Skip to content

UPSTREAM PR #17460: Typo in json.gbnf#302

Open
loci-dev wants to merge 1 commit intomainfrom
upstream-PR17460-branch_hi-cannon-patch-2
Open

UPSTREAM PR #17460: Typo in json.gbnf#302
loci-dev wants to merge 1 commit intomainfrom
upstream-PR17460-branch_hi-cannon-patch-2

Conversation

@loci-dev
Copy link
Copy Markdown

Mirrored from ggml-org/llama.cpp#17460

Small fix for typo in the number rule:

[eE] [-+]? [0-9] [1-9]{0,15}

This would allow one leading zero and no trailing zeros; e.g. forbidding e+10, e+100 etc. I assume the reverse was intended

Fix minor typo
Old grammar would allow exponents with a leading zero, and no trailing zeros
@loci-review
Copy link
Copy Markdown

loci-review bot commented Nov 24, 2025

Explore the complete analysis inside the Version Insights

Pull Request #302 - Performance Review Summary

PR Title: UPSTREAM PR #17460: Typo in json.gbnf
Change Scope: Grammar definition file correction


Assessment

No performance impact detected. This PR corrects a typo in the JSON grammar definition file (grammars/json.gbnf) that fixes scientific notation exponent parsing. The change is a single-character substitution in a declarative grammar rule with zero measurable performance impact across all binaries.

Performance Metrics:

  • Power consumption change: 0.0% across all 16 binaries
  • Response Time change: No function-level deltas detected
  • Throughput Time change: No function-level deltas detected
  • Tokens per second impact: None (no inference path modifications)

Code Change Analysis:

The fix corrects the exponent pattern from [0-9] [1-9]{0,15} to [1-9] [0-9]{0,15}, which:

  • Resolves incorrect rejection of valid exponents like e+10, e+100
  • Properly rejects malformed exponents with leading zeros like e+01
  • Aligns with JSON standard (RFC 8259)

Impact Scope:

  • Affects only grammar-based constrained generation (structured output validation)
  • Does not modify core inference functions: llama_decode, llama_encode, llama_tokenize
  • No changes to Model Processing, Token Processing, Memory Management, or Batch Processing modules
  • Grammar parsing occurs post-inference, outside the critical token generation path

Technical Correctness:

  • Bug fix improves JSON validation accuracy
  • No breaking changes for compliant JSON inputs
  • Pattern matching complexity unchanged (identical character class operations)

Recommendation

Approve and merge. This is a correctness improvement with no performance implications. The change fixes a documented grammar bug while maintaining performance parity across all metrics. No action items required.

@loci-review
Copy link
Copy Markdown

loci-review bot commented Nov 24, 2025

Explore the complete analysis inside the Version Insights

Performance Analysis Summary - PR #302

Overview

PR #302 introduces a single-character grammar specification fix in grammars/json.gbnf to correct scientific notation parsing for JSON numbers. The change modifies the exponent pattern from [0-9] [1-9]{0,15} to [1-9] [0-9]{0,15}, enabling proper validation of multi-digit exponents containing zeros (e.g., e+10, e+100).

Performance analysis across all 16 binaries shows zero measurable impact. Power consumption remains stable at 226822 nJ for libllama.so, 285287 nJ for llama-tts, and 240164 nJ for llama-run. No functions within the Performance-Critical Areas (model processing, token processing, memory management, batch processing) were modified. This is a correctness fix that aligns the grammar with JSON RFC 8259 without affecting runtime execution paths or inference performance.

@loci-dev loci-dev force-pushed the main branch 23 times, most recently from 41fee42 to 17a79cb Compare November 27, 2025 11:08
@loci-dev loci-dev force-pushed the main branch 30 times, most recently from 5ddab7d to 38683c7 Compare December 3, 2025 06:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants