feat(ast): add a dummy `node_id` field to all visitable AST types. #5775

rzvxa · 2024-09-14T21:33:12Z

Part of #5689

A few places are creating these nodes without AstBuilder; For now I've assigned them directly but should be taken care of as part of making types non_exhaustive.

graphite-app · 2024-09-14T21:33:17Z

Your org has enabled the Graphite merge queue for merging into main

Add the label “0-merge” to the PR and Graphite will automatically add it to the merge queue when it’s ready to merge. Or use the label “hotfix” to add to the merge queue as a hot fix.

You must have a Graphite account and log in to Graphite in order to use the merge queue. Sign up using this link.

rzvxa · 2024-09-14T21:33:26Z

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.
Learn more

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @rzvxa and the rest of your teammates on Graphite

codspeed-hq · 2024-09-14T21:40:08Z

CodSpeed Performance Report

Merging #5775 will degrade performances by 3.87%

_{Comparing 09-15-feat_ast_add_a_dummy_node_id_field_to_all_visitable_ast_types (f5344f9) with 09-15-fix_ast_tools_fix_miscalculation_of_enum_layouts (ea1c733)}

Summary

❌ 4 regressions
✅ 25 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

	Benchmark	`09-15-fix_ast_tools_fix_miscalculation_of_enum_layouts`	`09-15-feat_ast_add_a_dummy_node_id_field_to_all_visitable_ast_types`	Change
❌	`codegen[checker.ts]`	20.8 ms	21.7 ms	-3.87%
❌	`isolated-declarations[vue-id.ts]`	391.8 ms	404.8 ms	-3.19%
❌	`parser[RadixUIAdoptionSection.jsx]`	77.3 µs	79.7 µs	-3.04%
❌	`parser[cal.com.tsx]`	24.5 ms	25.2 ms	-3.08%

Boshen · 2024-09-15T03:10:22Z

CodSpeed Performance Report

Merging #5775 will degrade performances by 3.87%

Comparing 09-15-feat_ast_add_a_dummy_node_id_field_to_all_visitable_ast_types (00ce823) with 09-15-fix_ast_tools_fix_miscalculation_of_enum_layouts (ea1c733)

Summary

❌ 3 regressions ✅ 26 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

Benchmark 09-15-fix_ast_tools_fix_miscalculation_of_enum_layouts 09-15-feat_ast_add_a_dummy_node_id_field_to_all_visitable_ast_types Change
❌ codegen[checker.ts] 20.8 ms 21.7 ms -3.87%
❌ isolated-declarations[vue-id.ts] 391.8 ms 404.7 ms -3.17%
❌ parser[cal.com.tsx] 24.5 ms 25.2 ms -3.07%

Not bad!!

Boshen · 2024-09-15T03:11:16Z

A few places are creating these nodes without AstBuilder; For now I've assigned them directly but should be taken care of as part of making types non_exhaustive.

I cleaned up the parser a few days, let me ask @Dunqing to refactor the transformer 😀

Let's do non_exhaustive first #5778

overlookmotel · 2024-09-16T11:16:28Z

crates/oxc_ast_macros/src/lib.rs

        syn::Item::Enum(enum_) => (enum_repr(enum_), assert_generated_derives(&enum_.attrs)),
        syn::Item::Struct(struct_) => {
+            // HACK: temperorary measure to speed up the initial implementation of `node_id`.


Typo:

Suggested change

// HACK: temperorary measure to speed up the initial implementation of `node_id`.

// HACK: temporary measure to speed up the initial implementation of `node_id`.

overlookmotel · 2024-09-16T11:38:13Z

crates/oxc_ast_macros/src/lib.rs

+                    if let syn::Fields::Named(fields) = &mut struct_.fields {
+                        fields
+                            .named
+                            .insert(0, parse_quote!(pub node_id: ::oxc_syntax::node::NodeId));


I suggest we don't make node_id field public. We can make it harder to end up with the same NodeId appearing in AST twice, by making it impossible to set node_id for a node, except when creating a new node via AstBuilder.

(that would also remove the need for #[non_exhaustive])

We'll need to transition all AST node creation to via AstBuilder first, though.

The field needs to be Cell<NodeId>. Semantic will be setting these fields, and it only has an immutable AST.

We're going to need to do #5888 first (or make node_id fields pub initially), so as not to break Rolldown.

overlookmotel · 2024-09-16T11:45:33Z

crates/oxc_ast_macros/src/lib.rs

+            {
+                let args = TokenStream2::from(args);
+                if args.into_iter().next().is_some_and(
+                    |tk| matches!(tk, proc_macro2::TokenTree::Ident(id) if id == "visit" ),


We're missing out some nodes which I think should have IDs. e.g. JSXOpeningFragment and JSXClosingFragment. On the other hand, we're correctly not adding NodeIds for TemplateElementValue, RegExp and EmptyObject.

I think criteria for whether a struct should have a node_id field is whether it has a span field or not. But that may be incorrect too.

Maybe we should indicate to macro/ast tools when a type should not have a NodeId with #[ast(no_id)]?

I think criteria for whether a struct should have a node_id field is whether it has a span field or not. But that may be incorrect too.

Maybe we should indicate to macro/ast tools when a type should not have a NodeId with #[ast(no_id)]?

Let's get rid of this macro shenanigans, I was going to use it to make the process easier but this seems like a lot of work.
We should write them manually anyway, So let's start with that.

What a pain! But, yes, I'd forgotten that we'll need to do it manually in the end anyway.

We should also add #[serde(skip)] to all node_id fields.

overlookmotel · 2024-09-16T11:47:26Z

crates/oxc_syntax/src/node.rs

 #[cfg(feature = "serialize")]
 use serde::{Serialize, Serializer};

+#[ast]


Why does this get an #[ast] attr? ScopeId, SymbolId and ReferenceId don't have one.

They should be, Previously they were defined via the define_index_type macro and we couldn't add them to our schema because we don't support macro expansions - other than inherit ones.

Sorry I wasn't aware of this stack of PRs #4468

overlookmotel · 2024-09-16T11:51:25Z

crates/oxc_ast/src/generated/derive_clone_in.rs

@@ -21,6 +21,7 @@ impl<'alloc> CloneIn<'alloc> for BooleanLiteral {
    type Cloned = BooleanLiteral;
    fn clone_in(&self, allocator: &'alloc Allocator) -> Self::Cloned {
        BooleanLiteral {
+            node_id: CloneIn::clone_in(&self.node_id, allocator),


Hmm. We'll need to work out what to do with CloneIn. We can leave that question until later, but just flagging that it's something we need to consider.

crates/oxc_ast/src/generated/derive_content_eq.rs

crates/oxc_syntax/src/node.rs

overlookmotel · 2024-09-16T12:11:38Z

Merging #5775 will degrade performances by 3.87%

Not bad!!

We can win most of this perf back if we optimize field layouts later on.

rzvxa added 2 commits September 15, 2024 00:20

fix(ast_tools): fix miscalculation of enum layouts.

ea1c733

feat(ast): add a dummy node_id field to all visitable AST types.

ada0292

rzvxa mentioned this pull request Sep 14, 2024

fix(ast_tools): fix miscalculation of enum layouts. #5774

Merged

github-actions bot added A-ast Area - AST A-transformer Area - Transformer / Transpiler A-isolated-declarations Isolated Declarations A-ast-tools Area - AST tools labels Sep 14, 2024

chore: cleanup.

4226ba5

update docs.

00ce823

rzvxa marked this pull request as ready for review September 14, 2024 21:50

rzvxa requested review from overlookmotel and Boshen September 14, 2024 21:50

rzvxa mentioned this pull request Sep 14, 2024

feat(ast, syntax, ast_tools): Introduce GetNodeId trait. #5776

Draft

Boshen marked this pull request as draft September 15, 2024 03:11

overlookmotel reviewed Sep 16, 2024

View reviewed changes

chore: cleanup

f5344f9

overlookmotel force-pushed the 09-15-fix_ast_tools_fix_miscalculation_of_enum_layouts branch from ea1c733 to e8f8409 Compare October 21, 2024 12:19

Base automatically changed from 09-15-fix_ast_tools_fix_miscalculation_of_enum_layouts to main October 21, 2024 12:25

github-actions bot added the C-enhancement Category - New feature or request label Oct 21, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(ast): add a dummy `node_id` field to all visitable AST types. #5775

feat(ast): add a dummy `node_id` field to all visitable AST types. #5775

rzvxa commented Sep 14, 2024 •

edited by overlookmotel

Loading

graphite-app bot commented Sep 14, 2024

rzvxa commented Sep 14, 2024 •

edited

Loading

codspeed-hq bot commented Sep 14, 2024 •

edited

Loading

Boshen commented Sep 15, 2024

CodSpeed Performance Report

Merging #5775 will degrade performances by 3.87%

Summary

Benchmarks breakdown

Boshen commented Sep 15, 2024 •

edited by overlookmotel

Loading

overlookmotel Sep 16, 2024

overlookmotel Sep 16, 2024

overlookmotel Sep 16, 2024

overlookmotel Sep 19, 2024

overlookmotel Sep 16, 2024

rzvxa Sep 16, 2024 •

edited

Loading

overlookmotel Sep 19, 2024

overlookmotel Sep 16, 2024

rzvxa Sep 16, 2024

overlookmotel Sep 16, 2024

overlookmotel commented Sep 16, 2024

Merging #5775 will degrade performances by 3.87%

	// HACK: temperorary measure to speed up the initial implementation of `node_id`.
	// HACK: temporary measure to speed up the initial implementation of `node_id`.

feat(ast): add a dummy node_id field to all visitable AST types. #5775

Are you sure you want to change the base?

feat(ast): add a dummy node_id field to all visitable AST types. #5775

Conversation

rzvxa commented Sep 14, 2024 • edited by overlookmotel Loading

graphite-app bot commented Sep 14, 2024

Your org has enabled the Graphite merge queue for merging into main

rzvxa commented Sep 14, 2024 • edited Loading

codspeed-hq bot commented Sep 14, 2024 • edited Loading

Merging #5775 will degrade performances by 3.87%

Summary

Benchmarks breakdown

Boshen commented Sep 15, 2024

Merging #5775 will degrade performances by 3.87%

Summary

Benchmarks breakdown

Boshen commented Sep 15, 2024 • edited by overlookmotel Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rzvxa Sep 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

overlookmotel commented Sep 16, 2024

Merging #5775 will degrade performances by 3.87%

feat(ast): add a dummy `node_id` field to all visitable AST types. #5775

feat(ast): add a dummy `node_id` field to all visitable AST types. #5775

rzvxa commented Sep 14, 2024 •

edited by overlookmotel

Loading

rzvxa commented Sep 14, 2024 •

edited

Loading

codspeed-hq bot commented Sep 14, 2024 •

edited

Loading

Boshen commented Sep 15, 2024 •

edited by overlookmotel

Loading

rzvxa Sep 16, 2024 •

edited

Loading