Open
Conversation
apply_rules_inner used to handle the "child was rewritten, so the parent needs new field IDs" case by cloning the parent node, swapping in the new fields, pushing the clone onto the arena, and returning the new Id. Every ancestor on the path from the rewrite up to the root was duplicated this way, with the originals retained as garbage in the arena. Switch to in-place mutation: assign `ast.nodes[id].fields = new_fields` and return the same Id. Rule firings still produce genuinely new nodes via BuildCtx (their structure differs from the input), but the ancestor-rebuild spine no longer copies anything. This is safe because apply_rules_inner already works entirely by Id: the field snapshot is cloned out before recursing, no &Node references are held across mutations of the arena, and captures are scoped to a single rule firing so the now-stable Ids do not break anything. Memory effect: a desugaring pass that rewrites R leaves of a tree of average depth d previously appended R*d ancestor clones to the arena. Now appends 0. With Ids stable for the lifetime of an Ast, the Node::id field becomes truly redundant and is removed (along with the Node::id() accessor). AstCursor switches from caching `node: &Node` to tracking `node_id: Id` and looking the node up via the arena on each access; ChildrenIter now yields Ids directly. A new AstCursor::node_id() method gives callers access to the cursor position by Id.
Previously, apply_rules_inner snapshotted a node's fields by cloning the BTreeMap into a Vec<(FieldId, Vec<Id>)>, then built a fresh BTreeMap of new_fields for the rewritten Ids. For a node with N fields, this allocated 2N+1 things per visit (the snapshot Vec, N cloned children Vecs, the new BTreeMap entries) — even when nothing in the subtree was rewritten. Use std::mem::take to swap the parent's fields out by ownership: the recursion can mutate the AST (including pushing new nodes from rule firings) without any conflict, since we hold the owned BTreeMap locally. Iterate values_mut() and only allocate a fresh children Vec on the first divergence (lazy alloc): unchanged children stay in the existing slot. When done, swap the fields back. For a subtree with no rewrites, this is now zero allocations per node (modulo the recursion itself). For nodes with rewrites, it's one Vec allocation per field that contains a rewritten child, instead of two plus the BTreeMap rebuild.
Contributor
Author
Rerun has been triggered: 3 restarted 🚀 |
Contributor
Author
Rerun has been triggered: 2 restarted 🚀 |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR optimizes the shared/yeast Rust crate’s AST construction and desugaring pass by removing redundant node IDs and reducing cloning during rewrites, aiming to improve runtime and memory usage.
Changes:
- Remove
Node::idand update cursor/test code to work directly with arena indices viaAstCursor::node_id(). - Adjust the tree-sitter visitor to treat the root as index
0rather than reading anidfield. - Optimize desugaring traversal by taking ownership of
fields(mem::take) and mutating child vectors only when rewrites actually change them.
Show a summary per file
| File | Description |
|---|---|
| shared/yeast/tests/test.rs | Updates tests to use AstCursor::node_id() after removing Node::id. |
| shared/yeast/src/visitor.rs | Removes Node::id initialization and sets AST root to index 0. |
| shared/yeast/src/lib.rs | Refactors AstCursor/ChildrenIter to use node IDs and rewrites apply_rules_inner to avoid cloning via mem::take. |
Copilot's findings
- Files reviewed: 3/3 changed files
- Comments generated: 1
| for children in fields.values_mut() { | ||
| let mut new_children: Option<Vec<Id>> = None; | ||
| for (i, &child_id) in children.iter().enumerate() { | ||
| let result = apply_rules_inner(index, ast, child_id, fresh, rewrite_depth, None)?; |
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
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.
Node::idredundant, so we get rid of it.mem::take.