Skip to content
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

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

Merged
merged 20 commits into from
Jul 19, 2024
Merged
59 changes: 58 additions & 1 deletion crates/oxc_semantic/src/builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,58 @@ pub struct SemanticBuilderReturn<'a> {
pub errors: Vec<OxcDiagnostic>,
}

#[derive(Default, Debug)]
pub struct Collector {
Copy link
Member

Choose a reason for hiding this comment

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

We should move this to another file and have some doc comments explaning the why before merge.

node: usize,
scope: usize,
symbol: usize,
reference: usize,
}

impl<'a> Visit<'a> for Collector {
#[inline]
fn enter_node(&mut self, _: AstKind<'a>) {
self.node += 1;
}
#[inline]
fn enter_scope(&mut self, _: ScopeFlags, _: &Cell<Option<ScopeId>>) {
self.scope += 1;
}
#[inline]
fn leave_node(&mut self, _: AstKind<'a>) {}
Dunqing marked this conversation as resolved.
Show resolved Hide resolved
#[inline]
fn leave_scope(&mut self) {}

fn visit_binding_identifier(&mut self, _: &BindingIdentifier<'a>) {
self.symbol += 1;
self.node += 1;
}
fn visit_identifier_reference(&mut self, _: &IdentifierReference<'a>) {
self.reference += 1;
self.node += 1;
}
fn visit_jsx_identifier(&mut self, _: &JSXIdentifier<'a>) {
self.reference += 1;
self.node += 1;
}
fn visit_jsx_member_expression_object(&mut self, it: &JSXMemberExpressionObject<'a>) {
if let JSXMemberExpressionObject::MemberExpression(expr) = &it {
self.visit_jsx_member_expression(expr);
} else {
self.node += 1;
Copy link
Contributor

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

Choose a reason for hiding this comment

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

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
Contributor

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

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
Contributor

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);
// }
}

impl<'a> SemanticBuilder<'a> {
pub fn new(source_text: &'a str, source_type: SourceType) -> Self {
let scope = ScopeTree::default();
Expand Down Expand Up @@ -192,8 +244,13 @@ impl<'a> SemanticBuilder<'a> {
let scope_id = self.scope.add_scope(None, AstNodeId::DUMMY, ScopeFlags::Top);
program.scope_id.set(Some(scope_id));
} else {
self.visit_program(program);
let mut collector = Collector::default();
collector.visit_program(program);
self.nodes.reserve(collector.node);
self.scope.reserve(collector.scope);
self.symbols.reserve(collector.symbol, collector.reference);

self.visit_program(program);
// Checking syntax error on module record requires scope information from the previous AST pass
if self.check_syntax_error {
checker::check_module_record(&self);
Expand Down
5 changes: 5 additions & 0 deletions crates/oxc_semantic/src/node.rs
Original file line number Diff line number Diff line change
Expand Up @@ -162,6 +162,11 @@ impl<'a> AstNodes<'a> {
self.nodes.push(node);
ast_node_id
}

pub fn reserve(&mut self, additional: usize) {
self.nodes.reserve(additional);
self.parent_ids.reserve(additional);
}
}

#[derive(Debug)]
Expand Down
8 changes: 8 additions & 0 deletions crates/oxc_semantic/src/scope.rs
Original file line number Diff line number Diff line change
Expand Up @@ -218,4 +218,12 @@ impl ScopeTree {
pub fn remove_binding(&mut self, scope_id: ScopeId, name: &CompactStr) {
self.bindings[scope_id].shift_remove(name);
}

pub fn reserve(&mut self, additional: usize) {
self.parent_ids.reserve(additional);
self.child_ids.reserve(additional);
self.flags.reserve(additional);
self.bindings.reserve(additional);
self.node_ids.reserve(additional);
}
}
11 changes: 11 additions & 0 deletions crates/oxc_semantic/src/symbol.rs
Original file line number Diff line number Diff line change
Expand Up @@ -193,4 +193,15 @@ impl SymbolTable {
_ => false,
}
}

pub fn reserve(&mut self, additional: usize, reference: usize) {
self.spans.reserve(additional);
self.names.reserve(additional);
self.flags.reserve(additional);
self.scope_ids.reserve(additional);
self.declarations.reserve(additional);
self.resolved_references.reserve(additional);
self.redeclare_variables.reserve(additional);
self.references.reserve(reference);
}
}