perf(semantic): store class parents in IndexVec#22659
Conversation
How to use the Graphite Merge QueueAdd either label to this PR to merge it via the merge queue:
You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. This stack of pull requests is managed by Graphite. Learn more about stacking. |
camc314
force-pushed
the
c/05-21-perf_semantic_store_class_parents_in_indexvec
branch
from
b54ad38 to
94ae935
Compare
Merging this PR will not alter performance
Comparing 
Footnotes
|
| binder.ts | 193.08 kB || 41 | 1 || 7076 | 824 | ||
|
|
||
| kitchen-sink.tsx | 732.90 kB || 3138 | 345 || 100206 | 17853 | ||
| kitchen-sink.tsx | 732.90 kB || 3138 | 357 || 100206 | 17853 |
There was a problem hiding this comment.
hmm i'm not sure about this change actually
There was a problem hiding this comment.
I think FxHashMap is good here, as nested classes are not very common in practice. This approach may reallocate a few times more for a lot of None, which may cause a regression because faster access to data does not outweigh these allocation overheads.
Maybe you can try a further optimization to save these unnecessary reallocations by calculating the exact class count in stats, so we can reserve enough space upfront.
There was a problem hiding this comment.
yeah I agree. i don't think this is a positive change - lets close
There was a problem hiding this comment.
Pull request overview
This PR optimizes class-parent lookups in oxc_semantic by replacing ClassTable’s parent storage from a hash map keyed by ClassId to a dense IndexVec<ClassId, Option<ClassId>>, aligning storage with the fact that ClassId is allocated densely by the same table.
Changes:
- Replace
ClassTable.parent_idsstorage fromFxHashMap<ClassId, ClassId>toIndexVec<ClassId, Option<ClassId>>. - Add
ClassTable::parent_id()and route parent traversals (ancestors,pop_class, andcheck_super) through it. - Ensure parent slot and declaration slot stay aligned by pushing one parent entry per declared class (with a debug assertion on index equality).
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| crates/oxc_semantic/src/class/table.rs | Switch parent storage to IndexVec and add parent_id() accessor; update ancestors and declare_class to keep vectors aligned. |
| crates/oxc_semantic/src/class/builder.rs | Update class pop logic to use ClassTable::parent_id() instead of direct map access. |
| crates/oxc_semantic/src/checker/javascript.rs | Update super syntax-check logic to use ClassTable::parent_id() for parent traversal. |
| #[derive(Debug, Default)] | ||
| pub struct ClassTable<'a> { | ||
| pub parent_ids: FxHashMap<ClassId, ClassId>, | ||
| pub parent_ids: IndexVec<ClassId, Option<ClassId>>, | ||
| pub declarations: IndexVec<ClassId, NodeId>, |
There was a problem hiding this comment.
I don't think it is correct. Can you double check this @camc314 ?
There was a problem hiding this comment.
graphite-app
Bot
force-pushed
the
c/05-21-perf_semantic_use_direct_grandparent_lookup_for_ts_type_parameters
branch
from
37ab523 to
0c22389
Compare
graphite-app
Bot
force-pushed
the
c/05-21-perf_semantic_store_class_parents_in_indexvec
branch
from
94ae935 to
7078e0d
Compare
graphite-app
Bot
changed the base branch from
c/05-21-perf_semantic_use_direct_grandparent_lookup_for_ts_type_parameters
to
graphite-base/22659
graphite-app
Bot
force-pushed
the
c/05-21-perf_semantic_store_class_parents_in_indexvec
branch
from
7078e0d to
749b0f9
Compare
graphite-app
Bot
force-pushed
the
graphite-base/22659
branch
from
0c22389 to
d721ad9
Compare
graphite-app
Bot
force-pushed
the
c/05-21-perf_semantic_store_class_parents_in_indexvec
branch
from
749b0f9 to
ab047bb
Compare
ClassTablestored parent class links in anFxHashMap<ClassId, ClassId>, butClassIdis dense and allocated by the same table. The parent lookups inpop_class,ancestors, and thesupersyntax-check path only need direct lookup byClassId.This changes parent storage to
IndexVec<ClassId, Option<ClassId>>, pushes one parent slot when declaring each class, and reads parents throughClassTable::parent_id.