feat: skip unchanged defs via CstNode physical_equal#36
Merged
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, 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 the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughAdds incremental flat-projection support and plumbing: introduces Changes
Sequence Diagram(s)sequenceDiagram
participant SyncEditor as SyncEditor
participant Memo as build_projection_memos
participant Inc as to_flat_proj_incremental
participant Fresh as to_flat_proj
participant Reconcile as reconcile_flat_proj
SyncEditor->>Memo: call(prev_flat_proj_ref, prev_syntax_root_ref, syntax_root, counter)
alt prev_syntax_root exists AND prev_flat_proj exists
Memo->>Inc: (syntax_root, prev_syntax_root, prev_flat_proj, counter)
Inc-->>Memo: incremental FlatProj (reused IDs)
else prev_syntax_root unavailable
Memo->>Fresh: (syntax_root, counter)
Fresh-->>Memo: fresh FlatProj
Memo->>Reconcile: (prev_flat_proj, fresh_flat_proj, counter)
Reconcile-->>Memo: reconciled FlatProj (preserves IDs)
end
Memo->>SyncEditor: store prev_flat_proj_ref and update prev_syntax_root_ref
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
lambda-editor | 9a86f5a | Commit Preview URL Branch Preview URL |
Mar 18 2026, 10:16 AM |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
projection/flat_proj.mbt (1)
64-71: Potential double reconciliation when entry is reused unchanged.When a LetDef is unchanged (line 67 reuses
old_fp.defs[old_def_idx]directly), line 119 still callsreconcile_flat_projwhich will invokereconcile_aston this already-unchanged entry. Since the old and new ProjNodes are identical,reconcile_astwill return the old node (no-op), but the hash lookups and traversal still occur.Consider tracking which entries were reused to skip reconciliation for them, or accept this as acceptable overhead for code simplicity.
💡 Sketch for avoiding redundant reconciliation
One approach: build
new_fpwith a marker (e.g., sentinel NodeId or separate tracking array) for reused entries, then skip them inreconcile_flat_proj. This adds complexity, so the current approach may be acceptable if the incremental path is already providing sufficient speedup.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@projection/flat_proj.mbt` around lines 64 - 71, The reuse branch for LetDef entries directly pushes old_fp.defs[old_def_idx] but later reconcile_flat_proj still processes that index, causing redundant reconciliation; to fix, record which entries were reused (e.g., add a reusable boolean vector or mark with a sentinel NodeId when pushing in the LetDef reuse branch where old_fp.defs[old_def_idx] is pushed) and then, inside reconcile_flat_proj (and any loop that calls reconcile_ast), skip calling reconcile_ast for indices flagged as reused so unchanged entries are not revisited during reconciliation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@projection/flat_proj.mbt`:
- Around line 64-71: The reuse branch for LetDef entries directly pushes
old_fp.defs[old_def_idx] but later reconcile_flat_proj still processes that
index, causing redundant reconciliation; to fix, record which entries were
reused (e.g., add a reusable boolean vector or mark with a sentinel NodeId when
pushing in the LetDef reuse branch where old_fp.defs[old_def_idx] is pushed) and
then, inside reconcile_flat_proj (and any loop that calls reconcile_ast), skip
calling reconcile_ast for indices flagged as reused so unchanged entries are not
revisited during reconciliation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 98e0c8a7-644e-482b-9179-ce8a6e4c2ba5
📒 Files selected for processing (4)
editor/projection_memo.mbteditor/sync_editor.mbtprojection/flat_proj.mbtprojection/pkg.generated.mbti
Benchmark Comparison ReportComparing PR branch against Main Module BenchmarksBase branch: PR branch: event-graph-walker BenchmarksBase branch: PR branch: Benchmarks run with |
dowdiness
force-pushed
the
feature/flatproj-opt
branch
from
d678387 to
129a960
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
editor/performance_benchmark.mbt (1)
5-47: Consider extracting a shared benchmark helper.The 20-def and 80-def tests are structurally identical except for
let_count; a small helper would reduce drift risk.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@editor/performance_benchmark.mbt` around lines 5 - 47, Extract a shared helper to avoid duplication between the two tests by moving the common setup and benchmarking logic into a function like run_projection_incremental_bench(let_count: Int, name: String) that calls parser_bench_source(let_count, "0"), creates SyncEditor::new(name), sets text, warms with get_proj_node(), positions the cursor using source.length() - 2, and runs the b.bench closure that toggles insert/backspace and calls b.keep(editor.get_proj_node()); then replace both test "projection pipeline - incremental keystroke (20 defs)" and test "projection pipeline - incremental keystroke (80 defs)" to call this helper with 20 and 80 respectively (preserve the existing editor.insert/backspace behavior and error handling).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@editor/performance_benchmark.mbt`:
- Around line 14-24: Replace the anonymous MoonBit callback functions currently
written using the legacy `fn() { ... }` form with arrow function syntax `() => {
... }` for the benchmark callbacks; locate the two `b.bench(fn() { ... })`
usages (the blocks that toggle `inserted`, call `editor.backspace()` or
`editor.insert("1")`, and call `b.keep(editor.get_proj_node())`) and rewrite
each callback as `() => { ... }` so they follow the MoonBit guideline.
In `@projection/flat_proj.mbt`:
- Around line 64-67: The reuse of old projection entries (when
new_child.cst_node() == old_child.cst_node() and
defs.push(old_fp.defs[old_def_idx])) preserves stale start() offsets and
corrupts source ranges after edits; instead, when reusing an entry reconstruct
or update its source-range fields from the current CST node (use
new_child.cst_node().start() / relevant new_child positions) or build a fresh
projection tuple for that node so the start()/end() offsets reflect the new AST
positions; update both reuse sites (the block around new_child.cst_node() ==
old_child.cst_node() and the similar block at lines 104-106) to derive offsets
from the new_child rather than copying the old tuple wholesale.
---
Nitpick comments:
In `@editor/performance_benchmark.mbt`:
- Around line 5-47: Extract a shared helper to avoid duplication between the two
tests by moving the common setup and benchmarking logic into a function like
run_projection_incremental_bench(let_count: Int, name: String) that calls
parser_bench_source(let_count, "0"), creates SyncEditor::new(name), sets text,
warms with get_proj_node(), positions the cursor using source.length() - 2, and
runs the b.bench closure that toggles insert/backspace and calls
b.keep(editor.get_proj_node()); then replace both test "projection pipeline -
incremental keystroke (20 defs)" and test "projection pipeline - incremental
keystroke (80 defs)" to call this helper with 20 and 80 respectively (preserve
the existing editor.insert/backspace behavior and error handling).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: dc604c2d-b589-4a76-be61-147ef0e48fa3
📒 Files selected for processing (5)
editor/performance_benchmark.mbteditor/projection_memo.mbteditor/sync_editor.mbtprojection/flat_proj.mbtprojection/pkg.generated.mbti
🚧 Files skipped from review as they are similar to previous changes (1)
- editor/sync_editor.mbt
dowdiness
force-pushed
the
feature/flatproj-opt
branch
from
129a960 to
637da9b
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
projection/flat_proj.mbt (1)
117-119: Consider whether reconciliation is needed for fully-reused entries.When all entries were reused via CST pointer equality (line 67),
reconcile_flat_projis still called unconditionally. For entries that were already reused unchanged, the reconciliation:
- Matches by name (finds the same entry)
- Calls
reconcile_ast(old_entry.1, new_init, counter)where both are the sameProjNodeThis is safe but performs redundant work. For fully-incremental scenarios where most entries are unchanged, this adds unnecessary overhead.
💡 Potential optimization
Track whether any entries were actually rebuilt, and skip reconciliation when all were reused:
+ let mut any_rebuilt = false for new_child in new_children { if `@parser.LetDefView`::cast(new_child) is Some(v) { // ... existing logic ... if not(reused) { + any_rebuilt = true // Changed or new: build fresh // ... existing logic ... } } else if final_proj is None { // ... existing logic ... if not(reused_final) { + any_rebuilt = true final_proj = Some(syntax_to_proj_node(new_child, counter)) } } } let new_fp : FlatProj = { defs, final_expr: final_proj } - reconcile_flat_proj(old_fp, new_fp, counter) + if any_rebuilt { + reconcile_flat_proj(old_fp, new_fp, counter) + } else { + new_fp + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@projection/flat_proj.mbt` around lines 117 - 119, The code always calls reconcile_flat_proj(old_fp, new_fp, counter) even when every entry was reused via CST pointer equality, causing redundant work; modify the builder that creates new_fp (the defs/final_expr construction around where pointer-equality checks occur) to track a boolean flag like any_rebuilt which is set true whenever you actually allocate/replace a ProjNode or rebuild an entry, and after creating let new_fp : FlatProj = { defs, final_expr: final_proj } only call reconcile_flat_proj(old_fp, new_fp, counter) when any_rebuilt is true; if no entries were rebuilt, skip reconciliation (or simply reuse old_fp/new_fp identity) to avoid the redundant reconcile_ast(old_entry.1, new_init, counter) calls.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@projection/flat_proj.mbt`:
- Around line 117-119: The code always calls reconcile_flat_proj(old_fp, new_fp,
counter) even when every entry was reused via CST pointer equality, causing
redundant work; modify the builder that creates new_fp (the defs/final_expr
construction around where pointer-equality checks occur) to track a boolean flag
like any_rebuilt which is set true whenever you actually allocate/replace a
ProjNode or rebuild an entry, and after creating let new_fp : FlatProj = { defs,
final_expr: final_proj } only call reconcile_flat_proj(old_fp, new_fp, counter)
when any_rebuilt is true; if no entries were rebuilt, skip reconciliation (or
simply reuse old_fp/new_fp identity) to avoid the redundant
reconcile_ast(old_entry.1, new_init, counter) calls.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fdfa6566-b5c4-4b0c-85a8-c426bd23d9a2
📒 Files selected for processing (5)
editor/performance_benchmark.mbteditor/projection_memo.mbteditor/sync_editor.mbtprojection/flat_proj.mbtprojection/pkg.generated.mbti
🚧 Files skipped from review as they are similar to previous changes (1)
- editor/performance_benchmark.mbt
dowdiness
force-pushed
the
feature/flatproj-opt
branch
2 times, most recently
from
e452f1d to
f936bcf
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
projection/flat_proj.mbt (1)
64-71:⚠️ Potential issue | 🟠 MajorPrevent stale source ranges when reusing old projection nodes.
Line 70 and Line 109 reuse old
ProjNodevalues even though offsets may shift (as noted at Line 67). That preserves stale ranges in reused trees.🛠️ Suggested guard to avoid unsafe reuse after positional shifts
- if new_child.cst_node() == old_child.cst_node() && + if new_child.cst_node() == old_child.cst_node() && + new_child.start() == old_child.start() && old_def_idx < old_fp.defs.length() {- if new_child.cst_node() == old_child.cst_node() { + if new_child.cst_node() == old_child.cst_node() && + new_child.start() == old_child.start() { final_proj = old_fp.final_expr reused_final = true }Also applies to: 108-109
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3f098c67-d299-4807-bb95-6ede836ff949
📒 Files selected for processing (5)
editor/performance_benchmark.mbteditor/projection_memo.mbteditor/sync_editor.mbtprojection/flat_proj.mbtprojection/pkg.generated.mbti
🚧 Files skipped from review as they are similar to previous changes (1)
- editor/sync_editor.mbt
dowdiness
force-pushed
the
feature/flatproj-opt
branch
from
f936bcf to
aaa6b93
Compare
Add to_flat_proj_incremental: compares old and new CST children via CstNode equality (physical_equal fast-path from NodeInterner). Unchanged LetDefs reuse old FlatProj entries directly — skipping both syntax_to_proj_node and reconcile_ast. Three memo paths: - Incremental: prev FlatProj + prev SyntaxNode → physical_equal skip - Reconcile-only: prev FlatProj, no prev SyntaxNode → full rebuild + reconcile - Fresh: no prev FlatProj → fresh build The benefit shows in incremental editing (single keystroke changes) where most CST children are unchanged. Full reparse benchmarks are unaffected since every child is new.
dowdiness
force-pushed
the
feature/flatproj-opt
branch
from
aaa6b93 to
9a86f5a
Compare
This was referenced Mar 18, 2026
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.
Summary
Add
to_flat_proj_incrementalthat compares old and new CST children via CstNode equality (physical_equalfast-path from NodeInterner). Unchanged LetDefs reuse old FlatProj entries directly — skipping bothsyntax_to_proj_nodeandreconcile_ast.How it works
Three memo paths:
Performance impact
Benefits incremental editing (single keystroke changes) where most CST children are unchanged. Full reparse benchmarks are unaffected since every child is new.
Test plan
seed_flat_projclears prev syntax root to avoid stale comparisons🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Performance
Bug Fixes