perf(ast/cmp): avoid re-tokenizing when comparing token slices#325
perf(ast/cmp): avoid re-tokenizing when comparing token slices#325DonIsaac wants to merge 4 commits into
Conversation
WalkthroughRefactors AstComparator into a stateful comparator with an optional Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant LR as LinterRule:duplicate_case
participant AC as AstComparator
participant AST as AST
participant TOK as Tokens?
LR->>AC: construct { ast, tokens = ctx.tokens() }
LR->>AC: eql(nodeA, nodeB)
activate AC
AC->>AC: eqlInner(nodeA,nodeB)
alt tokens available
AC->>TOK: tokensEql(..., skip_tag_check=false)
TOK-->>AC: equal / unequal
else tokens absent
AC->>AST: source/token-slice based compare (fallback)
AST-->>AC: equal / unequal
end
AC-->>LR: bool result
deactivate AC
note over LR: If switch missing → branch-hinted early return
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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 |
![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/linter/rules/duplicate_case.zig (1)
100-101: Consider hoisting comparator creation outside the loop.The comparator is currently created for every pair of cases being compared. Since it's just a lightweight struct containing pointers and the tokens don't change, you could hoist the creation to line 94 (before the outer loop) for a minor efficiency gain:
const cases = switchStmt.ast.cases; +const comparator = AstComparator{ .ast = ast, .tokens = ctx.tokens() }; // check each case statement against each other for (cases, 0..) |case, i| { for ((i + 1)..cases.len) |j| { const other = cases[j]; const a = ast.fullSwitchCase(case) orelse unreachable; const b = ast.fullSwitchCase(other) orelse unreachable; - const comparator = AstComparator{ .ast = ast, .tokens = ctx.tokens() }; if (comparator.eql(a.ast.target_expr, b.ast.target_expr)) {src/visit/AstComparator.zig (1)
228-253: Excellent optimization! Consider updating the TODO comment.The refactored
tokensEqlfunction successfully implements the optimization described in the TODO comment when tokens are provided. The comptimeskip_tag_checkparameter is a nice touch that allows callers to avoid redundant tag comparisons.However, the TODO comment at line 229 now appears outdated since the optimization is implemented in the new code path (lines 230-243). Consider either:
- Removing the TODO entirely, or
- Moving it inside the else branch (line 245) to clarify it only applies to the fallback path
Apply this diff to remove the TODO:
fn tokensEql(self: *const AstComparator, a: Ast.TokenIndex, b: Ast.TokenIndex, comptime skip_tag_check: bool) bool { - // TODO: source tokens from TokenList.Slice to avoid re-parsing if (self.tokens) |tokens| {Or move it to the fallback branch:
fn tokensEql(self: *const AstComparator, a: Ast.TokenIndex, b: Ast.TokenIndex, comptime skip_tag_check: bool) bool { - // TODO: source tokens from TokenList.Slice to avoid re-parsing if (self.tokens) |tokens| { const left = tokens.get(a); const right = tokens.get(b); if (comptime !skip_tag_check) { if (left.tag != right.tag) return false; // we're comparing keywords or operators; no need to compare source text // (an `orelse` token will always be "orelse") if (left.tag.lexeme()) |_| return true; } const left_slice = self.ast.source[left.loc.start..left.loc.end]; const right_slice = self.ast.source[right.loc.start..right.loc.end]; return mem.eql(u8, left_slice, right_slice); } + // TODO: source tokens from TokenList.Slice to avoid re-parsing const tok_tags: []Token.Tag = self.ast.tokens.items(.tag);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/linter/rules/duplicate_case.zig(2 hunks)src/visit/AstComparator.zig(7 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Test (windows-latest)
🔇 Additional comments (3)
src/linter/rules/duplicate_case.zig (1)
88-91: LGTM! Branch hint is correctly applied.The
.likelyhint on the early return is correct. Since this rule runs on every AST node and most nodes are not switch statements, the common path is indeed returning early whenfullSwitchreturns null.src/visit/AstComparator.zig (2)
13-18: LGTM! Clean conversion to instance method.The refactor from a static function to an instance method with early-return optimization is well-structured and aligns with the new stateful design.
93-97: LGTM! Debug assertions appropriately verify token types.The debug assertions ensure that field access tokens are identifiers, and passing
skip_tag_check = truetotokensEqlavoids redundant checks since the assertions already validated the token types.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #325 +/- ##
==========================================
- Coverage 91.67% 91.61% -0.06%
==========================================
Files 91 91
Lines 7809 7816 +7
==========================================
+ Hits 7159 7161 +2
- Misses 650 655 +5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/visit/AstComparator.zig (1)
228-253: Excellent optimization! Remove outdated TODO.The token comparison optimization is well-implemented:
- Uses pre-tokenized token locations when available to avoid re-parsing
- Early return for lexemes (line 237) is correct since keywords/operators have fixed text
- Falls back gracefully when tokens are unavailable
- The
comptime skip_tag_checkparameter enables further optimization by callersHowever, the TODO comment at line 229 is now outdated since the implementation below does exactly what the TODO suggests.
Apply this diff to remove the outdated TODO:
- // TODO: source tokens from TokenList.Slice to avoid re-parsing if (self.tokens) |tokens| {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/visit/AstComparator.zig(7 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Collect and upload test coverage
🔇 Additional comments (4)
src/visit/AstComparator.zig (4)
5-11: LGTM! Well-documented performance optimization.The optional
tokensfield is a sound design choice that provides backwards compatibility while enabling the performance optimization described in the PR objectives. The documentation clearly explains the rationale.
13-13: LGTM! Breaking change aligns with PR objectives.Converting
eqlto an instance method is the correct design for a stateful comparator. This change also fixes what appears to be a type error in the previous signature where the second parameter was typed asboolinstead ofNode.Index.
93-97: LGTM! Good use of debug assertions.The debug assertions verify token types while the
skip_tag_check=trueparameter avoids redundant tag comparisons in production. This is a sound optimization strategy.
274-279: LGTM! Necessary imports for the new functionality.The added imports and type aliases support the token optimization feature and improve code readability.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/visit/AstComparator.zig (1)
5-11: LGTM: Performance optimization is well-documented.The optional
tokensfield and its documentation clearly explain the performance benefit of avoiding re-tokenization. The field is properly typed as optional with a sensible default.Note: The typo on line 8 ("presert" → "preserve") was already flagged in a previous review.
🧹 Nitpick comments (2)
src/visit/AstComparator.zig (2)
155-155: LGTM: Tag check is performed for variable identifiers.The call to
tokensEqlwithskip_tag_check: falseensures tag validation is performed. The function correctly identifies the variable name token atmain_token + 1(the token aftervar/const).For consistency with
eqlFieldAccess(lines 93-96), consider adding debug assertions to verify the tokens are identifiers:// Compare identifier names const left_main_token = self.ast.nodes.items(.main_token)[a]; const right_main_token = self.ast.nodes.items(.main_token)[b]; + if (util.IS_DEBUG) { + std.debug.assert(self.ast.tokens.items(.tag)[left_main_token + 1] == .identifier); + std.debug.assert(self.ast.tokens.items(.tag)[right_main_token + 1] == .identifier); + } if (!self.tokensEql(left_main_token + 1, right_main_token + 1, false)) return false;
228-253: LGTM: Correct implementation of optimized token comparison.The implementation correctly handles both the optimized path (when
tokensis provided) and the fallback path (when re-tokenization is needed). Key observations:
- Token-based path: Uses pre-computed token locations to avoid re-tokenization, includes lexeme optimization when tags are verified equal
- Fallback path: Uses
tokenSlicewith tag checks when tokens unavailable- Lexeme optimization: Correctly placed inside the
!skip_tag_checkblock (line 237), ensuring tags are verified equal before skipping text comparison for keyword/operator tokensThe
skip_tag_checkparameter allows callers to skip redundant tag verification when they've already validated token types (e.g., via debug assertions ineqlFieldAccess).Consider adding a doc comment to explain the
skip_tag_checkparameter:+/// Compares two tokens for equality. When `skip_tag_check` is true, the caller +/// must ensure both tokens are of compatible types (typically verified via debug assertions). fn tokensEql(self: *const AstComparator, a: Ast.TokenIndex, b: Ast.TokenIndex, comptime skip_tag_check: bool) bool {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/visit/AstComparator.zig(7 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Collect and upload test coverage
🔇 Additional comments (5)
src/visit/AstComparator.zig (5)
13-19: LGTM: Clean conversion to instance method.The conversion of
eqlto an instance method is straightforward and maintains the existing branch hint optimization for the early return path.
93-97: LGTM: Debug assertions ensure safe optimization.The debug assertions verify that both tokens are identifiers before calling
tokensEqlwithskip_tag_check: true. This ensures correctness in debug builds while enabling the performance optimization in release builds.
211-214: LGTM: Correct delegation to token comparison.The function correctly delegates to
tokensEqlwithskip_tag_check: false, which is appropriate since main tokens can represent various token types (identifiers, literals, etc.).
216-220: LGTM: Correct handling of optional tokens.The function properly handles the null cases and delegates to
tokensEqlwithskip_tag_check: falsefor non-null tokens.
273-280: LGTM: Imports properly support the new functionality.The import additions are appropriate:
utilprovidesIS_DEBUGfor conditional debug assertionsSemanticprovides the updatedAsttype andTokenList.Slice- Type aliases are correctly updated to reference
Semantic.Astand related typesAll imports are utilized in the implementation.
1 participant
Summary by CodeRabbit
Bug Fixes
Refactor