Skip to content

perf(semantic): calculate number of nodes, scopes, symbols, references before visiting AST #4328

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.

Sign up for GitHub

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

Merged
Dunqing merged 20 commits into from
Jul 19, 2024
+124 −0
Merged

perf(semantic): calculate number of nodes, scopes, symbols, references before visiting AST #4328

Changes from 1 commit
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
c4fbba1
test: visit
Dunqing Jul 17, 2024
59e4c74
feat: reserve
Dunqing Jul 17, 2024
f573c34
feat: improve small file
Dunqing Jul 19, 2024
bbf8dc3
Merge remote-tracking branch 'origin/main' into test-visit
Dunqing Jul 19, 2024
a698196
fix
Dunqing Jul 19, 2024
dc6b249
Fix
Dunqing Jul 19, 2024
53bc5f5
Fix
Dunqing Jul 19, 2024
e68d3fd
avoid reserve for symbols
Dunqing Jul 19, 2024
221674f
U
Dunqing Jul 19, 2024
753c64d
Update
Dunqing Jul 19, 2024
ad6aa55
Revert
Dunqing Jul 19, 2024
88b4bdf
Correct
Dunqing Jul 19, 2024
b4a8d67
Update
Dunqing Jul 19, 2024
2992c54
fix: stack overflow
Dunqing Jul 19, 2024
9d58b6c
fix: stack overflow
Dunqing Jul 19, 2024
a615891
Merge remote-tracking branch 'origin/main' into test-visit
Dunqing Jul 19, 2024
d7b8072
fix: symbols less than collected by collector
Dunqing Jul 19, 2024
1cec151
chore: add debug assert
Dunqing Jul 19, 2024
062e3b3
Merge branch 'main' into test-visit
Dunqing Jul 19, 2024
8a2971b
Merge branch 'main' into test-visit
Dunqing Jul 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Update
  • Loading branch information
@Dunqing
Dunqing committed Jul 19, 2024
commit b4a8d675e0dd9999f2098000fecbb4330470d45e
44 changes: 32 additions & 12 deletions crates/oxc_semantic/src/builder.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,39 +117,59 @@ impl<'a> Visit<'a> for Collector {
fn enter_scope(&mut self, _: ScopeFlags, _: &Cell<Option<ScopeId>>) {
self.scope += 1;
}
#[inline]
fn leave_node(&mut self, _: AstKind<'a>) {}
#[inline]
fn leave_scope(&mut self) {}

#[inline]
fn visit_binding_identifier(&mut self, _: &BindingIdentifier<'a>) {
self.symbol += 1;
self.node += 1;
}

#[inline]
fn visit_identifier_reference(&mut self, _: &IdentifierReference<'a>) {
self.reference += 1;
self.node += 1;
}

#[inline]
fn visit_jsx_identifier(&mut self, _: &JSXIdentifier<'a>) {
self.reference += 1;
self.node += 1;
}

#[inline]
fn visit_jsx_member_expression_object(&mut self, it: &JSXMemberExpressionObject<'a>) {
self.node += 1;
if let JSXMemberExpressionObject::MemberExpression(expr) = &it {
self.visit_jsx_member_expression(expr);
} else {
self.node += 1;
Copy link
Member

@overlookmotel overlookmotel Jul 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since you're counting JSXIdentifier as a reference, shouldn't this branch also have self.reference += 1;?

Copy link
Member Author

@Dunqing Dunqing Jul 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oxc/crates/oxc_semantic/src/builder.rs

Lines 1966 to 1975 in b4a8d67

fn reference_jsx_identifier(&mut self, ident: &JSXIdentifier) {
match self.nodes.parent_kind(self.current_node_id) {
Some(AstKind::JSXElementName(_)) => {
if !ident.name.chars().next().is_some_and(char::is_uppercase) {
return;
}
}
Some(AstKind::JSXMemberExpressionObject(_)) => {}
_ => return,
}

JSXIdentifier is not always a reference. you can see my latest commit.

Copy link
Member

@overlookmotel overlookmotel Jul 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two things:

In that case shouldn't it call self.visit_jsx_identifier?

I don't think we necessarily need to make the counts 100% accurate. Over-estimating is fine. It may be that the cost of complicating Collector to make an accurate count (and so making it a bit slower) is worse than the cost of allocating too much space in the Vecs. It's under-estimating that we absolutely need to avoid.

Copy link
Member Author

@Dunqing Dunqing Jul 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two things:

In that case shouldn't it call self.visit_jsx_identifier?

If I call the visit_jsx_identifier, the reference count will increase. But it's not a reference

Copy link
Member

@overlookmotel overlookmotel Jul 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unless I'm reading reference_jsx_identifier wrong, it does create a reference for a JSXIdentifier whose parent is a JSXMemberExpressionObject. Some(AstKind::JSXMemberExpressionObject(_)) => {} lets execution fall through to next block of code which creates the reference.

That would be correct behavior - in <Foo.bar />, Foo is a reference.

But this isn't easy to reason about. #3528 would make it much simpler.

}
}
// fn visit_jsx_element_name(&mut self, it: &JSXElementName<'a>) {
// if let JSXElementName::Identifier(ident) = it {
// if !ident.name.chars().next().is_some_and(char::is_uppercase) {
// return;
// }
// }

// self.visit_jsx_element_name(it);
// }
#[inline]
fn visit_jsx_element_name(&mut self, it: &JSXElementName<'a>) {
self.node += 1;
if let JSXElementName::Identifier(ident) = it {
self.node += 1;
if ident.name.chars().next().is_some_and(char::is_uppercase) {
self.reference += 1;
}
} else {
self.visit_jsx_element_name(it);
}
}

#[inline]
fn visit_jsx_attribute_name(&mut self, it: &JSXAttributeName<'a>) {
// NOTE: AstKind doesn't exists!
// self.node += 1;

if let JSXAttributeName::Identifier(_) = it {
self.node += 1;
} else {
self.visit_jsx_attribute_name(it);
}
}
}

impl<'a> SemanticBuilder<'a> {
Expand Down