perf(codegen): lower untranslated parser predicates as generatable Unknown templates - #218
Merged
Merged
Conversation
…known templates An untranslated predicate body (e.g. a bare this.IsNotIdentifierAssign() helper call) previously produced no PredicateTemplate, leaving its coordinate out of the generated set. compile_generated_parser_transition then refused to compile the containing rule, and with require_generated_callees active the drop cascaded through drop_rules_calling_disabled_rules to every calling rule — the untouched grammars-v4 JavaParser.g4 kept only 15 of 129 generated rule bodies and routed everything else through the interpreter, 5-6x slower than the predicate-stripped portable grammar (issue #209). Lower such coordinates as a new PredicateTemplate::Unknown instead, mirroring UnknownWithFailMessage: SemIR PExpr::Hook(0), so evaluation keeps the documented hook -> unknown-policy chain. Typed/closure hooks stay consulted, --sem-unknown dispositions and --require-full-semantics behave unchanged, the manifest still reports disposition assume-true with template null, and a dispose="error" coordinate override still lowers to no SemIR entry. Untouched JavaParser.g4 now generates all 129 rule dispatch bodies; parse times match the lit-true ({ true }?) build within noise: mojang-data-result.java 14.7 -> 3.1 ms, google-closure-property.java 4.1 -> 0.9 ms (portable baseline 2.2 / 0.6 ms; the small residual is allow_semantic_context adaptive prediction at the two predicate-bearing decisions). Fixes #209
Copy/Paste DetectionNo duplications found in 1 changed Rust file(s) (threshold: 100 tokens). |
📝 WalkthroughWalkthroughThe generator adds ChangesUnknown predicate codegen and benchmark routing
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant SemanticCollection
participant ParserSemIR
participant GeneratedParserRule
SemanticCollection->>ParserSemIR: collect PredicateTemplate::Unknown
ParserSemIR->>GeneratedParserRule: emit PExpr::Hook
GeneratedParserRule->>ParserSemIR: evaluate unknown predicate policy
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
📊 Source Code Metrics (this PR vs
|
| File | Cyclomatic | Cognitive | Functions | LLOC | MI |
|---|---|---|---|---|---|
| src/bin/antlr4-rust-gen.rs | 2261 (main: 2257) 🔴 | 1419 (main: 1418) 🔴 | 501 (main: 500) 🔴 | 3961 (main: 3953) 🔴 | 0 ⚪ |
Generated by mehen v1.7.0 — the code quality watcher.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
Claude Code review skipped — usage limit reached.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f883ab08c5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Swap the hand-written assert_eq! in untranslated_parser_predicate_keeps_generated_rule for a named insta snapshot per the repository snapshot guidance (pinning a collection's full contents is a value test, not a property test). Addresses the Codex review comment on PR #218.
…uting change The untouched-JavaParser.g4 lane previously ran the two predicate-bearing rules (and, via the caller cascade, most of the grammar) through the ATN interpreter. With untranslated predicates now lowering as generatable templates, that lane routes through generated rule bodies — a methodology change, not a runtime regression, and the per-fixture deltas are mixed by design (mojang 6.4->3.2 ms, google-closure 1.9->0.9 ms, but bazel-sky-value-retriever 10.4->19.0 ms on the CI runner, where the generated walker loses to the warmed interpreter DFA for that fixture's decision mix; the same shape exists on main between the portable and interpreted lanes). Bump JAVA_RUST_PREDICATE_VARIANT to v2 so the comparator skips the mismatched-variant rows and re-arms the 1.15x Java regression gate once both reports carry v2 — the same reset the v1 tag performed for the legacy-to-predicate transition in #175.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #209.
Root cause
The 5–6× slowdown on predicate-carrying grammars was never per-evaluation predicate cost — it was rule-compilation loss cascading through the call graph:
JavaParser.g4's bare helper calls{ this.IsNotIdentifierAssign() }?,{ this.DoLastRecordComponent() }?) parses to noPredicateTemplate, sostructural_predicate_templatespushed nothing for it.predicate_coordinates.allbut not.generated, andcompile_generated_parser_transitionrefuses to compile the containing rule.require_generated_calleesis true anddrop_rules_calling_disabled_rulescascades the kill upward: only 15 of 129 rules kept generated bodies. Everything else routed through the interpreter.Fix
Lower such coordinates as a new
PredicateTemplate::Unknownvariant instead of leaving them uncovered, mirroring the existingUnknownWithFailMessageprecedent:PExpr::Hook(HookId::new(0)), so evaluation keeps the documented hook → unknown-policy chain: an attached typed/closure hook is still consulted, and a declining hook falls through to the configured--sem-unknowndisposition. (A pure epsilon fold was rejected — it would silently stop consultingwith_typed_hooksusers, which work today via the interpreter.)assume-trueby default),templatestaysnull—Unknownis an internal lowering, not a translation.--sem-unknown=errorand--require-full-semanticsstill abort listing the coordinates; a per-coordinatedispose = "error"override still lowers to no SemIR entry (theparsed_bodygate) and stays fatal.ParserPredicatetable errors onUnknownexactly likeHook(SemIR is the active path).Results
3-way bench on
tools/parse-benchfixtures (min ms, generated parsers for portable/stripped vs untouched vs hand-edited{ true }?grammar):The untouched grammar now generates all 129 rule dispatch bodies and matches the lit-true build within noise. The residual gap to fully-stripped portable is
allow_semantic_contextadaptive prediction at the two predicate-bearing decisions; a per-coordinatedispose = "assume-true"override (literalTruetemplate) remains available for the last bit.Verification
untranslated_parser_predicate_keeps_generated_rulepins the generated-dispatch survival and thePExpr::Hooklowering.falsesteersannotationFieldValueto its second alternative on the generated path (verified against the regenerated untouched Java parser).assume-true/template: null;hook→hooked+ runtime Error policy installed;error/--require-full-semantics→ generation aborts naming both coordinates.CI parse-bench: benchmark variant bumped to v2
The
parse-benchjob's first run flaggedjava/bazel-sky-value-retriever.javaat 1.82× vs the base report. That is the methodology change this PR makes, not a runtime regression: the Java Rust lane benches the untouched predicate grammar, which previously ran through the ATN interpreter and now runs generated rule bodies. Per-fixture deltas are mixed by design — mojang 6.4→3.2 ms and google-closure 1.9→0.9 ms improve, while the bazel fixture's decision mix happens to favor the warmed interpreter DFA over the generated walker (the same shape exists on main between the portable-generated and interpreted lanes; nothing in the runtime changed).JAVA_RUST_PREDICATE_VARIANTis bumped tojava-upstream-parser-predicates-v2(304c1ac), the same baseline-reset mechanism the v1 tag used for the legacy-to-predicate transition in #175: the comparator skips mismatched-variant rows on this PR and re-arms the 1.15× Java gate once the base branch report also carries v2. Verified locally by running the real harness Java lane (report rows tagged v2) and feeding it tocompare.pyagainst the failed run's v1 numbers — rows skip cleanly and the job passes.Summary by CodeRabbit