Skip to content

UPSTREAM PR #17307: Fix json schema with '\' in literals#229

Open
DajanaV wants to merge 6 commits into
mainfrom
upstream-PR17307-branch_i-v-s-fix-json-schema-escape
Open

UPSTREAM PR #17307: Fix json schema with '\' in literals#229
DajanaV wants to merge 6 commits into
mainfrom
upstream-PR17307-branch_i-v-s-fix-json-schema-escape

Conversation

@DajanaV

@DajanaV DajanaV commented Nov 16, 2025

Copy link
Copy Markdown
Collaborator

@loci-review

loci-review Bot commented Nov 16, 2025

Copy link
Copy Markdown

Access the complete analysis in the LOCI Dashboard

Performance Analysis Summary: JSON Schema Backslash Escape Fix

Overview

PR #229 implements a targeted fix for JSON schema literal escaping, adding proper handling for backslash characters in common/json-schema-to-grammar.cpp. The changes modify two global objects: a regex pattern and an escape mapping table to correctly process backslash characters during grammar generation.

Key Findings

Performance Impact:

  • Highest Change: _ZNSt6vectorIjSaIjEE3endEv (std::vector::end) shows 217% Response Time increase (82ns → 261ns) and 299% Throughput increase (60ns → 239ns)
  • Absolute Impact: +179ns absolute change represents negligible real-world performance impact
  • Core Function Impact: No changes to critical inference functions (llama_decode, llama_encode, llama_tokenize) - tokens per second remains unaffected

Power Consumption Analysis:

  • Minor efficiency improvements across three binaries:
    • llama-run: -0.036% (103nJ reduction)
    • llama-tts: -0.02% (69nJ reduction)
    • llama-cvector-generator: -0.017% (57nJ reduction)
  • All other binaries show no measurable power consumption change

Technical Analysis:

  • Flame Graph: Reveals shallow execution pattern with 92% cost in function self-execution, indicating instruction-level rather than algorithmic issues
  • CFG Comparison: Identifies compiler-generated code reorganization as root cause - an unconditional branch was added, splitting linear instruction sequence and introducing pipeline overhead
  • Code Review: Changes are functionally correct and necessary for JSON schema compliance, addressing upstream issue #17306

Root Cause:
The performance regression stems from compilation artifacts rather than algorithmic changes. Static object modifications (regex and map) triggered different compiler optimization passes, resulting in suboptimal instruction layout for the vector::end function.

Conclusion:
While percentage changes appear significant, the absolute impact (179ns) is negligible for practical applications. The functional fix correctly addresses backslash escaping requirements without affecting core inference performance. The regression represents a compiler optimization boundary effect rather than a fundamental performance issue.

@DajanaV
DajanaV force-pushed the main branch 8 times, most recently from f333350 to 9c4623f Compare November 18, 2025 09:10
@loci-dev
loci-dev force-pushed the main branch 16 times, most recently from d403a40 to 1019d57 Compare November 23, 2025 11:07
@loci-review

loci-review Bot commented Nov 26, 2025

Copy link
Copy Markdown

Explore the complete analysis inside the Version Insights

Pull Request #229 Performance Review

PR Title: Fix json schema with '' in literals
Scope: JSON schema to grammar conversion - escape character handling


Summary

This PR addresses a bug in JSON schema literal escaping by adding backslash character handling to the escape regex and escape map. The changes affect 4 files (C++, Python, JavaScript implementations and test suite) with identical logic updates across all implementations.

Code Changes:

  • Modified GRAMMAR_LITERAL_ESCAPE_RE regex to include backslash: [\r\n\"][\r\n\"\\]
  • Added backslash escape mapping: {'\\', "\\\\"} to GRAMMAR_LITERAL_ESCAPES
  • Added test case validating escape sequences: \r, \n, \", \\

Performance Impact:
The modified code is in the JSON schema to grammar conversion module, which is not part of the inference hot path. The format_literal function processes schema definitions during grammar construction, not during token generation or model inference.

Analysis:

  • No changes to tokenization functions (llama_tokenize, llama_detokenize)
  • No changes to inference functions (llama_decode, llama_encode)
  • No changes to model processing or KV cache operations
  • The regex compilation happens once at module initialization
  • The escape map lookup adds one entry but maintains O(1) access time

Tokens per Second Impact: None. The changes affect schema preprocessing, which occurs before inference begins. Runtime token processing remains unchanged.

Power Consumption: No measurable change. The modified functions are not in the execution hot path and the binary-level analysis shows 0.0% change across all core libraries (libllama.so, libggml.so).

This is a correctness fix with no performance implications for model inference.

@loci-dev
loci-dev force-pushed the main branch 18 times, most recently from 5efc8b7 to f077805 Compare November 29, 2025 09:08
@loci-review

loci-review Bot commented Nov 29, 2025

Copy link
Copy Markdown

Explore the complete analysis inside the Version Insights

Performance Analysis Summary: PR #229

Overview

PR #229 introduces a correctness fix for JSON schema backslash escaping across 4 files (C++, Python, JavaScript implementations and tests). The changes add backslash character to escape regex patterns and lookup maps in json-schema-to-grammar modules.

Performance Impact

Analysis of version 2b31f40c-7083-41ef-bf94-93431405560c against base 350e6f1e-8cc6-43fd-8044-0514208078a2 reveals no performance impact from this PR. The observed performance variations are unrelated to the backslash escaping fix and stem from compiler optimization differences affecting STL template instantiations.

Modified Functions in PR:

  • format_literal() - adds one character to regex pattern, one entry to hash map
  • Estimated impact: <1 ns per literal processed (unmeasurable)

Inference Impact:
No tokenization or inference functions modified. Functions llama_decode, llama_encode, llama_tokenize remain unchanged. Tokens per second unaffected.

Power Consumption:

  • llama-run: +213 nJ (+0.111%)
  • llama-cvector-generator: +171 nJ (+0.078%)
  • llama-tts: -142 nJ (-0.063%)

Changes are within measurement noise and unrelated to this PR's code modifications.

Key Findings

Performance Variations Observed (Unrelated to PR):
The analysis identified significant performance changes in STL functions, but these are compilation artifacts, not caused by the JSON schema escaping fix:

  • std::vector::end functions show 135 ns increase (llama-tts, llama-cvector-generator) and 31 ns increase (llama-run)
  • std::function::operator() shows 27 ns increase in llama-cvector-generator
  • std::__detail::_Hash_code_base::_M_bucket_index shows 29 ns improvement in llama-run

These variations result from debug assertions or compiler flag differences between builds, not from the backslash escaping changes. The PR modifies only static regex patterns and escape maps used during schema conversion, which occurs once at initialization, not during inference.

Actual PR Impact:
The backslash escaping fix affects only format_literal() execution during JSON schema parsing. This function processes constant values in schemas and is not in any inference path. The regex now matches 4 characters instead of 3, and the hash map contains 6 entries instead of 5, resulting in negligible overhead during schema conversion operations.

@loci-dev
loci-dev force-pushed the main branch 3 times, most recently from 7475023 to fc0f51d Compare November 29, 2025 18:11
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.

3 participants