Description
Let's not do this right now!
NB: I would prefer if we don't do this right now because I think it'd be best to get wallclock benchmarks working first (#5), so we can accurately measure perf impact. This change should remove a bunch of branch predictor misses, which CodSpeed I think does not measure. So this would be a good opportunity to test that's correct and find out how much impact branch prediction misses have on Oxc's performance vs what CodSpeed has been telling us.
So I'm writing this up now not as "let's do it!", but just to make a note before I forget.
Proposed change
SemanticBuilder::enter_kind is called when entering every AST node. It is a giant match on AstKind.
This seems like an anti-pattern to me. Visitors already branch on the type of AST node, and here we're generating a huge amount of branches (or at best a large unpredictable jump table) figuring out what type of AST node we're in, when the visitor function which called enter_kind (via enter_node) already knew this information.
Now:
impl<'a> Visit<'a> for SemanticBuilder<'a> {
fn visit_function(&mut self, func: &Function<'a>, flags: ScopeFlags) {
let kind = AstKind::Function(self.alloc(func));
self.enter_node(kind);
/* ... */
}
fn enter_node(&mut self, kind: AstKind<'a>) {
self.create_ast_node(kind);
self.enter_kind(kind);
}
}
impl<'a> SemanticBuilder<'a> {
fn enter_kind(&mut self, kind: AstKind<'a>) {
match kind {
AstKind::Function(_) => {
do_stuff_with_function(func);
}
AstKind::ExportDefaultDeclaration(decl) => { /* ... */ }
AstKind::ExportNamedDeclaration(decl) => { /* ... */ }
AstKind::ExportSpecifier(decl) => { /* ... */ }
/* etc... loads more match arms */
}
}
}Replace with:
impl<'a> Visit<'a> for SemanticBuilder<'a> {
fn visit_function(&mut self, func: &Function<'a>, flags: ScopeFlags) {
let kind = AstKind::Function(self.alloc(func));
self.create_ast_node(kind);
do_stuff_with_function(func);
/* ... */
}
}i.e. Inline what was in enter_node and enter_kind into specific visitor functions. Do this for all the various types, and for leave_node / leave_kind too. All 4 methods can then be removed entirely.
Likely impact
oxc-project/oxc#4288 was a quick test to see what perf impact this might have. That uses #[inline(always)], which is not the best way to do this, but it probably results in about the same effect - by inlining everything the compiler can then deduce which match arm in enter_kind is relevant in each visitor, and remove the rest, thus producing roughly what I'm suggesting above. That PR showed 3%-4% speed-up on CodSpeed (NB: which also isn't taking into account branch predictor misses being removed).