[codex] Optimize C# parse benchmark runtime - #8
Conversation
|
Warning Review limit reached
Your plan currently allows 1 review/hour. Refill in 35 minutes and 52 seconds. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more review capacity refills, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than trial, open-source, and free plans. In all cases, review capacity refills continuously over time. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR optimizes the ANTLR Rust runtime lexer, parser, and token stream. The lexer now caches DFA states with relative-position keying. The parser's fast recognizer is refactored with bitset-backed token sets, persistent node structures, and conditional recovery logic. The token stream caches visibility relationships between buffered tokens. ChangesLexer DFA Caching and Parser Fast Recognizer Optimization
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
Copy/Paste DetectionFound 11 duplication(s) across 4 changed Rust file(s) (threshold: 100 tokens). Show duplicationsFound a 20 line (125 tokens) duplication in the following files:
pub fn next_token_with_hooks<I, F, A, P, E>(
lexer: &mut BaseLexer<I, F>,
atn: &Atn,
mut custom_action: A,
mut semantic_predicate: P,
mut accept_adjuster: E,
) -> CommonToken
where
I: CharStream,
F: TokenFactory,
A: FnMut(&mut BaseLexer<I, F>, LexerCustomAction),
P: FnMut(&BaseLexer<I, F>, LexerPredicate) -> bool,
E: FnMut(&mut BaseLexer<I, F>, i32, usize),
{
next_token_with_hooks_impl(
lexer,
atn,
&mut custom_action,
&mut semantic_predicate,
&mut accept_adjuster,Found a 31 line (121 tokens) duplication in the following files:
| Transition::Action { target, .. } => {
let boundary = left_recursive_boundary(atn, state, *target);
outcomes.extend(
self.recognize_state_fast(
atn,
FastRecognizeRequest {
state_number: *target,
stop_state,
index,
rule_start_index,
decision_start_index: next_decision_start_index,
precedence,
depth: depth + 1,
recovery_symbols: Rc::clone(&epsilon_recovery_symbols),
recovery_state: epsilon_recovery_state,
},
visiting,
memo,
expected,
)
.into_iter()
.map(|mut outcome| {
if let Some(rule_index) = boundary {
outcome.nodes.prepend(Rc::new(
FastRecognizedNode::LeftRecursiveBoundary { rule_index },
));
}
outcome
}),
);
}Found a 20 line (120 tokens) duplication in the following files:
let Some(state) = atn.state(config.state) else {
continue;
};
for transition in &state.transitions {
if !transition.matches(symbol, MIN_CHAR_VALUE, MAX_CHAR_VALUE) {
continue;
}
let mut advanced = config.clone();
set_config_state(atn, &mut advanced, transition.target());
if symbol == EOF {
advanced.consumed_eof = true;
} else {
advanced.position += 1;
}
next.push(advanced);
}
}
let closure = epsilon_closure(lexer, atn, next, semantic_predicate);
let target_has_semantic_context = closure.has_semantic_context;Found a 22 line (115 tokens) duplication in the following files:
recovery_symbols_id: Rc::as_ptr(&recovery_symbols) as usize,
recovery_state,
};
if let Some(outcomes) = memo.get(&key) {
return outcomes.clone();
}
let visit_key = key.clone();
if !visiting.insert(visit_key.clone()) {
return Vec::new();
}
let Some(state) = atn.state(state_number) else {
visiting.remove(&visit_key);
return Vec::new();
};
let next_decision_start_index = if starts_prediction_decision(state) {
Some(index)
} else {
decision_start_index
};
let (epsilon_recovery_symbols, epsilon_recovery_state) = if self.fast_recovery_enabled {Found a 15 line (114 tokens) duplication in the following files:
fn parser_matches_token_and_reports_mismatch() {
let source = Source {
tokens: vec![
CommonToken::new(1).with_text("x"),
CommonToken::eof("parser-test", 1, 1, 1),
],
index: 0,
};
let data = RecognizerData::new(
"Mini.g4",
Vocabulary::new([None, Some("'x'")], [None, Some("X")], [None::<&str>, None]),
);
let mut parser = BaseParser::new(CommonTokenStream::new(source), data);
assert_eq!(
parser.match_token(1).expect("token 1 should match").text(),Found a 32 line (113 tokens) duplication in the following files:
outcomes.extend(
self.recognize_state(
atn,
RecognizeRequest {
state_number: *target,
stop_state,
index,
rule_start_index,
decision_start_index: next_decision_start_index,
init_action_rules,
predicates,
rule_args,
member_actions,
return_actions,
local_int_arg,
member_values: member_values.clone(),
return_values: return_values.clone(),
rule_alt_number: next_alt_number,
track_alt_numbers,
consumed_eof,
precedence,
depth: depth + 1,
recovery_symbols: epsilon_recovery_symbols.clone(),
recovery_state: epsilon_recovery_state,
},
visiting,
memo,
expected,
)
.into_iter()
.map(|mut outcome| {
prepend_decision(&mut outcome, decision);Found a 14 line (110 tokens) duplication in the following files:
fn outcome_ties_keep_later_non_recursive_alternative() {
let first = RecognizeOutcome {
index: 1,
consumed_eof: false,
alt_number: 0,
member_values: BTreeMap::new(),
return_values: BTreeMap::new(),
diagnostics: Vec::new(),
decisions: Vec::new(),
actions: vec![ParserAction::new(1, 0, 0, None)],
nodes: vec![RecognizedNode::Token { index: 0 }],
};
let second = RecognizeOutcome {
actions: vec![ParserAction::new(2, 0, 0, None)],Found a 13 line (109 tokens) duplication in the following files:
fn parser_matches_token_and_reports_mismatch() {
let source = Source {
tokens: vec![
CommonToken::new(1).with_text("x"),
CommonToken::eof("parser-test", 1, 1, 1),
],
index: 0,
};
let data = RecognizerData::new(
"Mini.g4",
Vocabulary::new([None, Some("'x'")], [None, Some("X")], [None::<&str>, None]),
);
let mut parser = BaseParser::new(CommonTokenStream::new(source), data);Found a 19 line (106 tokens) duplication in the following files:
let start_state = atn
.rule_to_start_state()
.get(rule_index)
.copied()
.ok_or_else(|| {
AntlrError::Unsupported(format!("rule {rule_index} has no start state"))
})?;
let stop_state = atn
.rule_to_stop_state()
.get(rule_index)
.copied()
.filter(|state| *state != usize::MAX)
.ok_or_else(|| {
AntlrError::Unsupported(format!("rule {rule_index} has no stop state"))
})?;
let start_index = self.current_visible_index();
self.clear_prediction_diagnostics();
self.reset_per_parse_caches();Found a 16 line (104 tokens) duplication in the following files:
FastRecognizedNode::Rule {
rule_index,
invoking_state,
start_index,
stop_index,
children,
} => {
let mut context = ParserRuleContext::new(*rule_index, *invoking_state);
if let Some(token) = self.token_at(*start_index) {
context.set_start(token);
}
if let Some(token) = stop_index.and_then(|index| self.token_at(index)) {
context.set_stop(token);
}
if children.has_left_recursive_boundary() {
let folded = fold_fast_left_recursive_boundaries(children.to_vec());Found a 15 line (103 tokens) duplication in the following files:
FastRecognizedNode::MissingToken {
token_type,
at_index,
text,
} => {
let current = self.token_at(*at_index);
let token = CommonToken::new(*token_type)
.with_text(text)
.with_span(usize::MAX, usize::MAX)
.with_position(
current.as_ref().map(Token::line).unwrap_or_default(),
current.as_ref().map(Token::column).unwrap_or_default(),
);
Ok(ParseTree::Error(ErrorNode::new(token)))
} |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces significant performance optimizations to the lexer and parser, including a lexer DFA cache, a bitset-based FIRST set implementation, and a lookahead cache in the token stream. Feedback highlights a critical issue where lexer action positions are lost during caching, which could break position-dependent logic. Reviewers also suggested optimizing bitset range operations, avoiding eager tokenization of the entire input to preserve lazy-lexing, and using ordered collections in the fast recognizer to ensure deterministic behavior and stable traces.
| actions: accept | ||
| .actions | ||
| .iter() | ||
| .map(|action| LexerActionTrace { | ||
| action_index: action.action_index, | ||
| position: token_start, | ||
| rule_index: action.rule_index, | ||
| }) | ||
| .collect(), | ||
| } |
There was a problem hiding this comment.
The cached accept state logic currently loses the precise input position of lexer actions. In close_config, LexerActionTrace records the exact config.position where an action transition was reached. However, LexerDfaCachedAccept only stores the position_delta for the final accept boundary, and cached_accept_state applies the same token_start to all actions. This will break lexer actions that rely on Text() or other position-dependent logic during replay. LexerDfaActionKey should be updated to store its own position_delta relative to the token start to ensure stable traces and correct action execution.
References
- In performance-critical hot paths, use structured sortable types for cache keys instead of string formatting and joining to avoid unnecessary allocations and ensure stable traces.
There was a problem hiding this comment.
Fixed in 7639fca. LexerDfaActionKey now carries a per-action position_delta relative to the token start, and cached configs/accept states reconstruct LexerActionTrace.position as token_start + position_delta instead of collapsing every action to the token start. I also passed the token start through the DFA key/cache conversion path so replayed configs keep stable position-sensitive action traces. Validated with cargo test --quiet, strict clippy, and targeted runtime descriptor LexerExec/PositionAdjustingLexer.
| fn extend_range(&mut self, start: i32, stop: i32) { | ||
| let (start, stop) = if start <= stop { | ||
| (start, stop) | ||
| } else { | ||
| (stop, start) | ||
| }; | ||
| for symbol in start..=stop { | ||
| self.insert(symbol); | ||
| } | ||
| } |
There was a problem hiding this comment.
The extend_range implementation for TokenBitSet is suboptimal for large ranges (like Wildcard transitions) because it iterates through every symbol and calls insert, which includes redundant bounds checks and potential Vec resizes. Since this is a bitset, you can optimize this using bit-parallel operations to fill entire words at once.
fn extend_range(&mut self, start: i32, stop: i32) {
let (start, stop) = if start <= stop { (start, stop) } else { (stop, start) };
let start_slot = if start == TOKEN_EOF { 0 } else if start > 0 { start as usize } else if stop >= TOKEN_EOF { 0 } else if stop > 0 { 1 } else { return; };
let stop_slot = if stop == TOKEN_EOF { 0 } else if stop > 0 { stop as usize } else { return; };
if start_slot > stop_slot { return; }
let start_word = start_slot / 64;
let end_word = stop_slot / 64;
if end_word >= self.words.len() {
self.words.resize(end_word + 1, 0);
}
if start_word == end_word {
let mask = (!0u64 << (start_slot % 64)) & (!0u64 >> (63 - (stop_slot % 64)));
self.words[start_word] |= mask;
} else {
self.words[start_word] |= !0u64 << (start_slot % 64);
for i in (start_word + 1)..end_word {
self.words[i] = !0u64;
}
self.words[end_word] |= !0u64 >> (63 - (stop_slot % 64));
}
}There was a problem hiding this comment.
Fixed in 7639fca. TokenBitSet::extend_range now fills whole u64 words through extend_slot_range instead of looping through every symbol and repeatedly calling insert. EOF is handled separately before the positive token range is mapped to bit slots. Validated with cargo test --quiet, strict clippy, and the C# parse benchmark rerun.
| })?; | ||
|
|
||
| let start_index = self.current_visible_index(); | ||
| self.input.fill(); |
There was a problem hiding this comment.
Calling self.input.fill() at the start of every rule parse forces the lexer to tokenize the entire input stream immediately. While this might improve benchmark stability by removing lexing overhead from the measurement, it breaks the lazy-lexing design of CommonTokenStream. For very large files, this could lead to significant latency spikes or memory exhaustion before parsing even begins. Consider if this can be made optional or deferred.
There was a problem hiding this comment.
Fixed in 7639fca. The up-front self.input.fill() in parse_atn_rule is removed, so rule parsing no longer forces complete tokenization before parsing starts. To avoid regressing large valid parses into per-token sync overhead, token_type_at now performs a deferred fill only after the fast recognizer has actually advanced into the stream (index >= 64). This keeps startup lazy while preserving the benchmark behavior for large files. Validated with cargo test --quiet, strict clippy, targeted runtime descriptors, and the C# benchmark rerun.
| let memo_capacity = self.input.size().saturating_mul(4).min(1_000_000); | ||
| let mut visiting = FxHashSet::with_capacity_and_hasher(256, FxBuildHasher::default()); | ||
| let mut memo = FxHashMap::with_capacity_and_hasher(memo_capacity, FxBuildHasher::default()); |
There was a problem hiding this comment.
The memo_capacity heuristic is quite aggressive and can lead to high memory usage. Furthermore, to ensure a deterministic state index and preserve stable traces during parsing, use BTreeMap and BTreeSet instead of hash-based collections.
| let memo_capacity = self.input.size().saturating_mul(4).min(1_000_000); | |
| let mut visiting = FxHashSet::with_capacity_and_hasher(256, FxBuildHasher::default()); | |
| let mut memo = FxHashMap::with_capacity_and_hasher(memo_capacity, FxBuildHasher::default()); | |
| let mut visiting = BTreeSet::new(); | |
| let mut memo = BTreeMap::new(); |
References
- Use ordered maps (e.g., BTreeMap) instead of hash maps when a deterministic state index is required to preserve stable behavior or traces.
There was a problem hiding this comment.
Partially fixed in 7639fca. I reduced the aggressive sizing by removing the eager full-input fill that made input.size() equal to the whole token stream before the first pass, and capped the initial memo reserve at 262_144 instead of 1_000_000. I intentionally kept FxHashMap/FxHashSet for the fast recognizer memo and visiting sets: these caches are parser-internal, not externally iterated, and do not define DFA state numbering or emitted traces. Switching this hot path to BTreeMap/BTreeSet would trade away the benchmark improvement without adding observable determinism; the externally visible lexer DFA trace storage remains ordered.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![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: 7639fca2cf
ℹ️ 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".
| let inserted_visit = if self.fast_recovery_enabled { | ||
| if !visiting.insert(visit_key.clone()) { | ||
| return Vec::new(); | ||
| } | ||
| true | ||
| } else { | ||
| false |
There was a problem hiding this comment.
Disabling visiting whenever fast_recovery_enabled is false allows recognize_state_fast to recurse back into the same (state_number, stop_state, index, ...) key through epsilon/nullable cycles during the first pass, because the only remaining brake is the depth limit. Combined with single-outcome memo suppression later in the function, this can force the first pass to hit RECOGNITION_DEPTH_LIMIT and fail on otherwise valid inputs, triggering an expensive full retry every time for grammars with nullable recursion/cycles.
Useful? React with 👍 / 👎.
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
@codex review |
|
@greptile-apps Addressed the two latest parser follow-ups in c483bac.
Validated locally with |
|
@codex review |
|
Codex Review: Didn't find any major issues. You're on a roll. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/token_stream.rs (2)
15-15: 💤 Low valueConsider adding a doc comment for the sentinel constant.
A brief doc comment explaining why
usize::MAXwas chosen as the sentinel (e.g., "Sentinel value for uncached entries; valid token indices never reach usize::MAX") would improve code clarity.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/token_stream.rs` at line 15, Add a short doc comment above the constant UNKNOWN_NEXT_VISIBLE explaining that it is a sentinel for uncached/unknown next-visible token indices and that usize::MAX is used because valid token indices never reach that value (making it an unambiguous sentinel); update the comment to mention the symbol name UNKNOWN_NEXT_VISIBLE and that it represents "uncached entries" or "no next visible token" for clarity.
282-284: ⚡ Quick winAdd
#[must_use]attribute tois_filled().Pure getters should have the
#[must_use]attribute to signal that ignoring the return value is likely a mistake. This aligns with Clippy'smust_use_candidatepedantic lint. As per coding guidelines, Clippy pedantic lints should be treated as errors in CI.📝 Proposed fix
+ #[must_use] pub const fn is_filled(&self) -> bool { self.fetched_eof }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/token_stream.rs` around lines 282 - 284, The getter method is_filled() should be annotated with #[must_use] so callers are warned if they ignore its boolean result; add #[must_use] immediately above the pub const fn is_filled(&self) -> bool definition (preserving pub/const) so Clippy's must_use_candidate pedantic lint is satisfied.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/atn/lexer.rs`:
- Around line 643-670: The cached DFA key currently built by
lexer_dfa_key(active, token_start) omits the predicate-sensitive flag
has_semantic_context, allowing predicate-crossing closures to collide; update
the cache identity so predicate-sensitivity is part of the key (e.g., include
has_semantic_context when computing lexer_dfa_key or add a new
lexer_dfa_key_with_semantic flag) and then call
lexer.cache_lexer_dfa_state(state, ...) as before, or alternatively skip caching
entirely when has_semantic_context is true; ensure references to lexer_dfa_key,
has_semantic_context, cache_lexer_dfa_state and LexerDfaCachedState are updated
consistently so predicate-sensitive states are not reused without predicate
re-evaluation.
---
Nitpick comments:
In `@src/token_stream.rs`:
- Line 15: Add a short doc comment above the constant UNKNOWN_NEXT_VISIBLE
explaining that it is a sentinel for uncached/unknown next-visible token indices
and that usize::MAX is used because valid token indices never reach that value
(making it an unambiguous sentinel); update the comment to mention the symbol
name UNKNOWN_NEXT_VISIBLE and that it represents "uncached entries" or "no next
visible token" for clarity.
- Around line 282-284: The getter method is_filled() should be annotated with
#[must_use] so callers are warned if they ignore its boolean result; add
#[must_use] immediately above the pub const fn is_filled(&self) -> bool
definition (preserving pub/const) so Clippy's must_use_candidate pedantic lint
is satisfied.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 982f45a5-712d-4d8e-ab3c-ffa2798bad75
📒 Files selected for processing (4)
src/atn/lexer.rssrc/lexer.rssrc/parser.rssrc/token_stream.rs
|
@greptile-apps Addressed the interner-invariant follow-up in 7a2f829.
Validated locally with |
|
@codex review |
|
The documentation update in 7a2f829 looks good — the doc comment on LGTM. Thanks for the clean follow-up. Tip: You can customize Greptile's behavior for this repo with |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7a2f82929d
ℹ️ 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".
| .entry(state) | ||
| .or_insert(cached_state); |
There was a problem hiding this comment.
The cached lexer state is inserted with or_insert, so if a DFA state is first seen on a predicate-free path (has_semantic_context = false) and later reached through a path that does touch predicates, the stored flag never upgrades to true. match_token_cached uses this flag to decide whether transition replay is safe; a stale false allows replay across predicate-dependent edges and can skip required predicate re-evaluation, changing tokenization for grammars where predicate and non-predicate paths converge to the same config set.
Useful? React with 👍 / 👎.
|
@codex review |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
1 participant
Summary
Impact
The C# Rust benchmark mean is now around the target 200 ms while preserving runtime conformance. The largest C# fixture remains slower than Go, but the Rust runtime is much closer than the prior multi-second baseline.
10-iteration C# comparison:
Rust mean across the four C# fixtures: 203.5 ms.
Kotlin quick benchmark remains healthy, with Rust still faster than Go on the checked Kotlin fixtures.
Validation
git diff --checkcargo test --quietcargo clippy --locked --all-targets --all-features -- -D warningspython3 tools/parse-bench/run.py --languages csharp --runtimes rust-antlr,go-antlr --iters 10 --warmups 2 --json target/parse-bench/csharp-rust-go-final-10iters.json --markdown target/parse-bench/csharp-rust-go-final-10iters.mdpython3 tools/parse-bench/run.py --quick --languages kotlin --runtimes rust-antlr,go-antlr --json target/parse-bench/kotlin-rust-go-final-quick.json --markdown target/parse-bench/kotlin-rust-go-final-quick.mdcargo run --release --quiet --bin antlr4-runtime-testsuite->summary: 357 passed, 0 failed, 0 skipped, 357 runSummary by CodeRabbit