-
Couldn't load subscription status.
- Fork 638
Making Stable Syntax Pointers just a wrapper around syntax nodes #8501
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: eytan_graphite/tracking_syntaxnode_and_interning_through_a_tracked_function
Are you sure you want to change the base?
Conversation
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
1136962 to
ef2bd29
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@orizi reviewed 7 of 10 files at r1, all commit messages.
Reviewable status: 7 of 10 files reviewed, 3 unresolved discussions (waiting on @TomerStarkware)
crates/cairo-lang-defs/src/cache/mod.rs line 1569 at r1 (raw file):
} #[derive(Serialize, Deserialize, Clone, Copy, Hash, Eq, PartialEq, salsa::Update)]
Probably not required - just need to remove the rest.
Suggestion:
#[derive(Serialize, Deserialize, Clone, Copy, Hash, Eq, PartialEq)]crates/cairo-lang-defs/src/cache/mod.rs line 1576 at r1 (raw file):
green: GreenIdCached, offset: TextOffset, // TODO(eytan-starkware): Using result for serde implementation. Change this to Either.
add serde for it - or just map to another serde function.
Code quote:
// TODO(eytan-starkware): Using result for serde implementation. Change this to Either.crates/cairo-lang-syntax/src/node/db.rs line 22 at r1 (raw file):
offset: TextOffset, parent: Either<SyntaxNode<'db>, FileId<'db>>, index: usize,
f2f
Code quote:
parent: Either<SyntaxNode<'db>, FileId<'db>>,
index: usize,crates/cairo-lang-defs/src/ids.rs line 790 at r1 (raw file):
pub fn name(&self, db: &'db dyn Database) -> Option<SmolStrId<'db>> { let node = self.1.0.0; assert!(node.parent(db).is_some());
consider removing these - as these are extra queries not really needed.
Code quote:
assert!(node.parent(db).is_some());There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@orizi reviewed 2 of 10 files at r1.
Reviewable status: 9 of 10 files reviewed, 3 unresolved discussions (waiting on @eytan-starkware and @TomerStarkware)
3b0503b to
899e353
Compare
ef2bd29 to
5e44fa2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 9 of 10 files reviewed, 3 unresolved discussions (waiting on @orizi and @TomerStarkware)
crates/cairo-lang-defs/src/ids.rs line 790 at r1 (raw file):
Previously, orizi wrote…
consider removing these - as these are extra queries not really needed.
Will change to a non query accessor in the future. Added todo.
crates/cairo-lang-defs/src/cache/mod.rs line 1569 at r1 (raw file):
Previously, orizi wrote…
Probably not required - just need to remove the rest.
Some salsa::Update implementations are necessary for the loading data context to be salsa::Update. Removed what I could
crates/cairo-lang-defs/src/cache/mod.rs line 1576 at r1 (raw file):
Previously, orizi wrote…
add serde for it - or just map to another serde function.
Using my own enum now
crates/cairo-lang-syntax/src/node/db.rs line 22 at r1 (raw file):
Previously, orizi wrote…
f2f
Using a new enum for the unique identifier of the syntax node
899e353 to
d813a17
Compare
5e44fa2 to
148a496
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@TomerStarkware reviewed 10 of 10 files at r1, 10 of 10 files at r2, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @orizi)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@orizi reviewed 10 of 10 files at r2, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @eytan-starkware)
d813a17 to
a78806e
Compare
148a496 to
b8f0747
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@orizi reviewed 3 of 3 files at r3, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @eytan-starkware)
b8f0747 to
9dc334b
Compare
a78806e to
25b9606
Compare

Optimize Syntax Node Representation for Better Performance
TL;DR
Refactored the syntax node representation to eliminate the separate
SyntaxStablePtrstructure, reducing indirection and improving performance.What changed?
SyntaxStablePtrenum with a direct reference toSyntaxNodeSyntaxNodeDatastructureEither<SyntaxNode, FileId>for clearer root node handlingNOTE
I introduced a semantic change:
Root is now actually a Node of SyntaxFile. This means:
a. parent can return a node that usually would panic
b. kind can return SyntaxFile
c. SyntaxFile key fields can be called