Open overlookmotel opened 2 months ago
I can now see a way around this problem.
ScopeId
is just a wrapper aroundu32
, andscope_id
fields of AST nodes are unused at this point. So we could store the binding counts inscope_id
fields. InSemanticBuilder
's main AST pass, it'd retrieve the binding count fromscope_id
field, use that count to initializeBindings
for that scope with appropriate capacity, and then write actualScopeId
intoscope_id
field.A bit hacky to misuse
scope_id
fields in this way, but should work.What we don't know is whether the gain of pre-allocating
Bindings
hashmaps to required capacity will outweigh the cost of calculating the counts. Maybe it won't because most scopes contain less than 4 bindings anyway (which is probably default capacity ofHashMap
), and these hashmaps are usually fairly small so growth is not so costly. But probably worth trying and measuring it.NB: If we do try this, we may need to take into account the load factor of
HashMap
. To be able to store 8 items in a hashmap requires the hashmap to have space for more than 8 elements. Docs forHashMap::with_capacity
suggest it takes load factor into account, but we should make sure or we may misread benchmark results as it won't be doing quite what we think it is.
Clever! I'm looking forward to seeing you try this!
https://github.com/oxc-project/oxc/pull/4367 added an AST traversal at start of building
Semantic
, to count how many nodes, scopes, symbols, and references there are in the AST.It uses these counts to pre-allocate sufficient capacity in
AstNodes
,ScopeTree
andSymbolTable
before the main traversal which populates these structures with data. This avoids these structures needing to grow during the main traversal, with all the memory-copying involved in that. The result was a big boost for performance.@Dunqing suggested:
I replied:
I can now see a way around this problem.
ScopeId
is just a wrapper aroundu32
, andscope_id
fields of AST nodes are unused at this point. So we could store the binding counts inscope_id
fields. InSemanticBuilder
's main AST pass, it'd retrieve the binding count fromscope_id
field, use that count to initializeBindings
for that scope with appropriate capacity, and then write actualScopeId
intoscope_id
field.A bit hacky to misuse
scope_id
fields in this way, but should work.What we don't know is whether the gain of pre-allocating
Bindings
hashmaps to required capacity will outweigh the cost of calculating the counts. Maybe it won't because most scopes contain less than 4 bindings anyway (which is probably default capacity ofHashMap
), and these hashmaps are usually fairly small so growth is not so costly. But probably worth trying and measuring it.NB: If we do try this, we may need to take into account the load factor of
HashMap
. To be able to store 8 items in a hashmap requires the hashmap to have space for more than 8 elements. Docs forHashMap::with_capacity
suggest it takes load factor into account, but we should make sure or we may misread benchmark results as it won't be doing quite what we think it is.