-
-
Notifications
You must be signed in to change notification settings - Fork 496
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
refactor(semantic): simplify getting AstNodeId
s for CFG
#5674
refactor(semantic): simplify getting AstNodeId
s for CFG
#5674
Conversation
Your org has enabled the Graphite merge queue for merging into mainAdd the label “0-merge” to the PR and Graphite will automatically add it to the merge queue when it’s ready to merge. Or use the label “hotfix” to add to the merge queue as a hot fix. You must have a Graphite account and log in to Graphite in order to use the merge queue. Sign up using this link. |
This stack of pull requests is managed by Graphite. Learn more about stacking. |
CodSpeed Performance ReportMerging #5674 will not alter performanceComparing Summary
|
3509f51
to
48dcdc1
Compare
4531601
to
c40dd01
Compare
a3987cf
to
d8ec781
Compare
c40dd01
to
147fd56
Compare
faf82fb
to
ab9a845
Compare
This is showing on benchmarks as a 1% perf regression. This is surprising, as the code is doing less, not more. Possibly reducing the amount of code in some functions has caused compiler to make different decisions about inlining, and those decisions aren't as good as before. 🤷 The benchmarks have CFG disabled. I strongly suspect this would be a speed-up when CFG is enabled. |
9b4374f
to
5b7e978
Compare
Close as stale. |
Reopening just to check benchmarks of this rebased on latest main. I found it surprising originally that this did not have a positive effect. |
5b7e978
to
e69dfb4
Compare
Nope. Still -1% on all semantic benchamrks. |
Remove
ast_node_records
, and the functions that push and pop it, fromSemanticBuilder
.The purpose of that apparatus is to do something quite simple - get the
AstNodeId
of anExpression
from its parent, for the control flow graph (CFG). As nodes are created in order, a simpler way to do this is to get what the nextAstNodeId
will be before visiting theExpression
.This implementation has to work around a current shortcoming with
IndexVec
- convertingNodes::len()
to anAstNodeId
involves a panicking bounds-check, which adds overhead even when CFG is disabled. So for nowget_next_node_id
returns the "id" as au32
, and we convert it toAstNodeId
only after the node has been created, which guarantees that theu32
is a validAstNodeId
(not out of bounds), becauseIndexVec::push
also performs that same bounds check.If we implement oxc-project/backlog#109,
get_next_node_index
could becomeget_next_node_id
and return anAstNodeId
, and the unsafe code could be removed.