[codex] Optimize lexer input and position hot paths - #100
Conversation
Copy/Paste DetectionFound 8 duplication(s) across 6 changed Rust file(s) (threshold: 100 tokens). Show duplicationsFound a 54 line (320 tokens) duplication in the following files:
atn: &LexerAtn,
hooks: &mut H,
mut generated_action: A,
mut generated_predicate: P,
unknown_policy: UnknownSemanticPolicy,
mut accept_adjuster: E,
) -> Result<TokenId, TokenStoreError>
where
I: CharStream,
H: SemanticHooks,
A: FnMut(&mut BaseLexer<I>, LexerCustomAction) -> bool,
P: FnMut(&BaseLexer<I>, LexerPredicate) -> Option<bool>,
E: FnMut(&mut BaseLexer<I>, i32, usize),
{
let hooks = RefCell::new(hooks);
let token = next_token_with_hooks_impl(
lexer,
sink,
atn,
&mut |lexer, action| {
if !generated_action(lexer, action)
&& !dispatch_lexer_action_hook(&hooks, lexer, action)
&& unknown_policy == UnknownSemanticPolicy::Error
&& let (Ok(rule), Ok(index)) = (
usize::try_from(action.rule_index()),
usize::try_from(action.action_index()),
)
{
lexer.record_semantic_error(true, rule, index);
}
},
&mut |lexer, predicate| {
generated_predicate(lexer, predicate)
.or_else(|| dispatch_lexer_predicate_hook(&hooks, lexer, predicate))
.unwrap_or_else(|| match unknown_policy {
UnknownSemanticPolicy::AssumeTrue => true,
UnknownSemanticPolicy::AssumeFalse => false,
UnknownSemanticPolicy::Error => {
lexer.record_semantic_error(
false,
predicate.rule_index(),
predicate.pred_index(),
);
false
}
})
},
&mut |lexer| dispatch_lexer_before_token_hook(&hooks, lexer),
&mut accept_adjuster,
&mut |lexer, accept_position| {
dispatch_lexer_after_accept_hook(&hooks, lexer, accept_position);
},
LexerMatchStrategy {
compiled: None,Found a 26 line (153 tokens) duplication in the following files:
pub fn next_token_with_hooks<I, A, P, E>(
lexer: &mut BaseLexer<I>,
sink: &mut TokenSink<'_>,
atn: &LexerAtn,
mut custom_action: A,
mut semantic_predicate: P,
mut accept_adjuster: E,
) -> Result<TokenId, TokenStoreError>
where
I: CharStream,
A: FnMut(&mut BaseLexer<I>, LexerCustomAction),
P: FnMut(&BaseLexer<I>, LexerPredicate) -> bool,
E: FnMut(&mut BaseLexer<I>, i32, usize),
{
next_token_with_hooks_impl(
lexer,
sink,
atn,
&mut custom_action,
&mut semantic_predicate,
&mut |_| {},
&mut accept_adjuster,
&mut |_, _| {},
LexerMatchStrategy {
compiled: None,
use_cache: false,Found a 25 line (142 tokens) duplication in the following files:
atn: &LexerAtn,
hooks: &mut H,
) -> Result<TokenId, TokenStoreError>
where
I: CharStream,
H: SemanticHooks,
{
let hooks = RefCell::new(hooks);
let token = next_token_with_hooks_impl(
lexer,
sink,
atn,
&mut |lexer, action| {
let _ = dispatch_lexer_action_hook(&hooks, lexer, action);
},
&mut |lexer, predicate| {
dispatch_lexer_predicate_hook(&hooks, lexer, predicate).unwrap_or(true)
},
&mut |lexer| dispatch_lexer_before_token_hook(&hooks, lexer),
&mut |_, _, _| {},
&mut |lexer, accept_position| {
dispatch_lexer_after_accept_hook(&hooks, lexer, accept_position);
},
LexerMatchStrategy {
compiled: None,Found a 24 line (134 tokens) duplication in the following files:
let atn = AtnDeserializer::new(&SerializedAtn::from_i32(&[
4, 0, 2, // version, lexer, max token type
9, // states
6, -1, // 0 token start
2, 0, // 1 rule 0 start
1, 0, // 2
1, 0, // 3
7, 0, // 4 rule 0 stop
2, 1, // 5 rule 1 start
1, 1, // 6
1, 1, // 7
7, 1, // 8 rule 1 stop
0, // non-greedy
0, // precedence
2, // rules
1, 1, // rule 0 starts at 1, token type 1
5, 2, // rule 1 starts at 5, token type 2
1, // modes
0, // default mode starts at 0
0, // sets
8, // edges
0, 1, 1, 0, 0, 0, // start -> rule 0
0, 5, 1, 0, 0, 0, // start -> rule 1
1, 2, 5, 'a' as i32, 0, 0, 2, 3, 5, 'b' as i32, 0, 0, 3, 4, 1, 0, 0, 0, 5, 6, 5,Found a 22 line (130 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(atn, next, &mut |predicate| {
semantic_predicate(lexer, predicate)
});
let target_has_semantic_context = closure.has_semantic_context;Found a 28 line (119 tokens) duplication in the following files:
impl IntStream for FallbackInput {
fn consume(&mut self) {
self.0.consume();
}
fn la(&mut self, offset: isize) -> i32 {
self.0.la(offset)
}
fn index(&self) -> usize {
self.0.index()
}
fn seek(&mut self, index: usize) {
self.0.seek(index);
}
fn size(&self) -> usize {
self.0.size()
}
fn source_name(&self) -> &str {
self.0.source_name()
}
}
// Deliberately implements none of the optional fast paths.
impl CharStream for FallbackInput {Found a 22 line (117 tokens) duplication in the following files:
atn: &LexerAtn,
mut custom_action: A,
mut semantic_predicate: P,
mut accept_adjuster: E,
) -> Result<TokenId, TokenStoreError>
where
I: CharStream,
A: FnMut(&mut BaseLexer<I>, LexerCustomAction),
P: FnMut(&BaseLexer<I>, LexerPredicate) -> bool,
E: FnMut(&mut BaseLexer<I>, i32, usize),
{
next_token_with_hooks_impl(
lexer,
sink,
atn,
&mut custom_action,
&mut semantic_predicate,
&mut |_| {},
&mut accept_adjuster,
&mut |_, _| {},
LexerMatchStrategy {
compiled: None,Found a 13 line (107 tokens) duplication in the following files:
let mut hooks = LifecycleRecordingHooks::default();
let mut store = TokenStore::new(lexer.source_text(), lexer.source_name());
let mut sink = TokenSink::new(&mut store);
let mut ids = Vec::new();
for _ in 0..3 {
let id = if compiled {
next_token_compiled_with_semantic_hooks(
&mut lexer, &mut sink, &atn, &dfa, &mut hooks,
)
} else {
next_token_with_semantic_hooks(&mut lexer, &mut sink, &atn, &mut hooks)
}
.expect("lifecycle token should fit"); |
|
Warning Review limit reached
Next review available in: 27 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughThe lexer adds optional Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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 |
|
@codex review |
![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 performance optimizations for compiled lexers, allowing them to read in-memory ASCII directly from static DFA tables and commit accepted spans in bulk. It adds optional fast-path methods to CharStream to preserve scalar fallback behavior for custom streams and Unicode input, along with benchmarks and synthetic fixtures to measure the improvements. A review comment identifies a bug in src/char_stream.rs where checking start > end after clamping them to the stream length can incorrectly return a default position summary instead of None for invalid out-of-bounds ranges; moving the check before clamping is suggested.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| let len = self.data.len(&self.source); | ||
| let start = start.min(len); | ||
| let end = end.min(len); | ||
| if start > end { | ||
| return None; | ||
| } |
There was a problem hiding this comment.
Checking start > end after clamping start and end to len can lead to incorrect behavior. If both start and end are out of bounds and start > end (e.g., start = 7, end = 6 with len = 5), they will both be clamped to len (becoming 5 and 5). The check start > end will then evaluate to false, incorrectly returning Some(PositionSummary::default()) instead of None for an invalid range.
Checking start > end before clamping resolves this issue.
| let len = self.data.len(&self.source); | |
| let start = start.min(len); | |
| let end = end.min(len); | |
| if start > end { | |
| return None; | |
| } | |
| if start > end { | |
| return None; | |
| } | |
| let len = self.data.len(&self.source); | |
| let start = start.min(len); | |
| let end = end.min(len); |
There was a problem hiding this comment.
Fixed in 1c03f6e6b: range order is now validated before either bound is clamped, with an explicit position_summary(7, 6) == None regression assertion. Validation passed with the focused all-features test and cargo clippy --locked --all-targets --all-features -- -D warnings.
|
Codex Review: Didn't find any major issues. Another round soon, please! Reviewed commit: ℹ️ 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". |
|
@codex review |
![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: 1c03f6e6b0
ℹ️ 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".
| rust_runner = write_rust_runner( | ||
| work_dir, | ||
| specs, | ||
| args.runtime_root, | ||
| args.rust_thin_lto, |
There was a problem hiding this comment.
When --work-dir is the selected --runtime-root or any ancestor of it (for example, using /tmp as a shared benchmark workspace for the documented detached checkout), prepare_work calls clear_work_dir recursively before any check relates the two paths. This deletes the runtime checkout that this newly added option is meant to benchmark, then later commands fail because its Cargo.toml and sources are gone. Reject overlapping work/runtime paths before cleanup.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in cbf5d46a2: benchmark cleanup now receives the selected runtime root and rejects a work directory that equals or contains that checkout before rmtree can run. Added regression coverage for equal, ancestor, disjoint sibling, and safe nested work paths. Validation passed with python3 -m unittest discover -s tools/parse-bench -p 'test_*.py', python3 -m py_compile tools/parse-bench/run.py tools/parse-bench/test_run.py, and cargo clippy --locked --all-targets --all-features -- -D warnings.
|
@codex review |
|
Codex Review: Didn't find any major issues. Another round soon, please! Reviewed commit: ℹ️ 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
Closes #78
Summary
CharStream, with compatible defaults for custom streamsMORE, recovery, and custom-stream behavior with focused tests and perf countersInvestigation
Ahead-of-time DFA compilation already removed ATN closure and config work from ordinary lexer matching, but current
mainstill performedseek(position) + la(1)for each compiled-DFA symbol and replayed each accepted character throughconsume_char()to reconstruct line/column. Recent lexer lifecycle and dynamic-emission work made a shared commit primitive necessary, but did not remove those costs.Performance
Same-machine, interleaved measurements on an Apple M3 Pro show:
0.8853xvsmain(11.5% faster)0.8642x0.7725xvsmain0.9958x; all 17 fixtures remained below the 2% regression thresholdInputStreamThe full methodology and per-language results are in
docs/issue-78-lexer-benchmark.md.Validation
cargo fmt --checkcargo check --locked --all-targetscargo check --locked --all-targets --all-featurescargo test --locked --all-targets --all-features(191 library, 2 harness, 163 generator, 7 CLI tests)cargo clippy --locked --all-targets --all-features -- -D warningsRUSTDOCFLAGS='-D warnings' cargo doc --locked --no-deps --all-features