refactor(semantic): convert scope fields to IndexVecs #4208

lucab · 2024-07-11T16:16:18Z

Closes: #4269

graphite-app · 2024-07-11T16:16:24Z

Your org has enabled the Graphite merge queue for merging into main

Add the label “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.

lucab · 2024-07-11T16:16:35Z

refactor(semantic): convert scope fields to IndexVecs #4208 👈
main

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @lucab and the rest of your teammates on Graphite

codspeed-hq · 2024-07-11T16:21:49Z

CodSpeed Performance Report

Merging #4208 will not alter performance

_{Comparing ups/semantics-no-maps (81ed588) with main (92ee774)}

Summary

✅ 32 untouched benchmarks

crates/oxc_semantic/src/scope.rs

overlookmotel · 2024-07-11T22:59:34Z

Thanks for continuing your rampage through all the semantic optimization issues!

There's a modest perf improvement, and as Boshen said on another issue, any gain is worthwhile. A bunch of 1%/2% gains add up pretty quickly to something significant.

I think I see why it's not as much of a speed-up as I originally expected. I thought node_id was set for all nodes which have scopes, but I now see it's only set for functions (I don't know why! If knowing the AstNodeId of a scope is useful, we should record it for all scopes). But the result is that the node_ids IndexVec is much larger than the hash map it replaces. The greater size is partially counter-acting the gain of faster accesses.

Dunqing · 2024-07-12T01:13:56Z

I think I see why it's not as much of a speed-up as I originally expected. I thought node_id was set for all nodes which have scopes, but I now see it's only set for functions (I don't know why! If knowing the AstNodeId of a scope is useful, we should record it for all scopes). But the result is that the node_ids IndexVec is much larger than the hash map it replaces. The greater size is offsetting the gain of faster accesses.

We should set the node_id for all nodes that have a scope, initially I only needed to use it in functions, so I only set for functions

overlookmotel · 2024-07-12T09:28:12Z

#4203 is merged now, so we can move recording AstNodeId into add_scope (see #4208 (comment)).

Merge this PR now, and do that in a follow-up? Or amend this PR? Which do you prefer @lucab?

lucab · 2024-07-12T09:38:26Z

@overlookmotel I'm fine with merging this small one as is. I started having a look at the new scope/node coupling, but I still need to explore some corners and I doubt I'll find more time this week.

@lucab

@lucab answered Don's critique in #4208 (comment)

graphite-app · 2024-07-12T09:43:35Z

Merge activity

Jul 12, 5:43 AM EDT: The merge label 'merge' was detected. This PR will be added to the Graphite merge queue once it meets the requirements.
Jul 12, 5:43 AM EDT: overlookmotel added this pull request to the Graphite merge queue.
Jul 12, 5:47 AM EDT: overlookmotel merged this pull request with the Graphite merge queue.

Closes: https://github.com/oxc-project/backlog/issues/10

overlookmotel · 2024-07-12T09:44:45Z

@lucab OK great. Merging now. If you have time, please open an issue for moving recording AstNodeId into add_scope (#4208 (comment)), so we don't forget about it.

lucab · 2024-07-12T16:29:07Z

Tracked at #4234.

overlookmotel · 2024-07-12T16:46:24Z

Thank you!

## [0.21.0] - 2024-07-18 - d7ab0b8 semantic: [**BREAKING**] Simplify node creation (#4226) (lucab) ### Features - af4dc01 ast: Align ts ast scope with typescript (#4253) (Dunqing) - 83c2c62 codegen: Add option for choosing quotes; remove slow `choose_quot` method (#4219) (Boshen) - 5d17675 mangler: Add debug mode (#4314) (Boshen) - e3e663b mangler: Initialize crate and integrate into minifier (#4197) (Boshen) - c818472 minifier: Dce conditional expression `&&` or `||` (#4190) (Boshen) - 8a190eb oxc: Export `oxc_mangler` (Boshen) - 20cdb1f semantic: Align class scope with typescript (#4195) (Dunqing) - 92ee774 semantic: Add `ScopeFlags::CatchClause` for use in CatchClause (#4205) (Dunqing) - 205c259 sourcemap: Support SourceMapBuilder#token_chunks (#4220) (underfin) - 7eb960d transformer: Decode xml character entity `&#xhhhh` and `&#nnnn;` (#4235) (Boshen) ### Bug Fixes - bf3d8d3 codegen: Print annotation comment inside parens for new and call expressions (#4290) (Boshen) - 084ab76 codegen: Use `ryu-js` for f64 to string (Boshen) - e167ef7 codegen: Print parenthesis properly (#4245) (Boshen) - c65198f codegen: Choose the right quote for jsx attribute string (#4236) (Boshen) - be82c28 codegen: Print `JSXAttributeValue::StringLiteral` directly (#4231) (Boshen) - 3df9e69 mangler: No shorthand `BindingProperty`; handle var hoisting and export variables (#4319) (Boshen) - f144082 minifier: RemoveDeadCode should visit nested expression (#4268) (underfin) - 66b455a oxc_codegen: Avoid print same pure comments multiple time (#4230) (IWANABETHATGUY) - 9a87e41 parser: Avoid crashing on invalid const modifier (#4267) (lucab) - 641a78b parser: Fix tests for number parsing (#4254) (overlookmotel) - 9badac0 semantic: Avoid var hosting insert the var variable to the `CatchClause` scope (#4337) (Dunqing) - 95e15b6 semantic: Incorrect resolve references for `ExportSpecifier` (#4320) (Dunqing) - c362bf7 semantic: Incorrect resolve references for `TSInterfaceHeritage` (#4311) (Dunqing) - 351ecf2 semantic: Incorrect resolve references for `TSTypeQuery` (#4310) (Dunqing) - 1108f2a semantic: Resolve references to the incorrect symbol (#4280) (Dunqing) - 22d56bd semantic: Do not resolve references after `FormalParameters` in TS type (#4241) (overlookmotel)- 1c117eb Avoid print extra semicolon after accessor property (#4199) (IWANABETHATGUY) ### Performance - a8dc4f3 parser: Speed up parsing numbers with `_` separators (#4259) (overlookmotel) - b94540d parser: Speed up parsing octal literals (#4258) (overlookmotel) - a7b328c parser: Faster parsing decimal numbers (#4257) (overlookmotel) - f9d3f2e semantic: Inline ast record functions (#4272) (overlookmotel) - 8fad7db semantic: Reduce `AstNodeId` to `u32` (#4264) (overlookmotel) - 23743db semantic: Do not record ast nodes for cfg if cfg disabled (#4263) (overlookmotel) - da69076 semantic: Reduce overhead of cfg recording ast nodes (#4262) (overlookmotel) - cb15303 semantic: Reduce memory copies (#4216) (overlookmotel) - ef4c1f4 semantic: Reduce lookups (#4214) (overlookmotel) - f23e54f semantic: Recycle unresolved references hash maps (#4213) (overlookmotel) - 2602ce2 semantic: Reuse existing map of unresolved refs (#4206) (lucab) ### Refactor - 2c7bb9f ast: Pass final `ScopeFlags` into `visit_function` (#4283) (overlookmotel) - 3e099fe ast: Move `enter_scope` after `visit_binding_identifier` (#4246) (Dunqing) - aab7aaa ast/visit: Fire node events as the outermost one. (#4203) (rzvxa) - d1c4be0 codegen: Clean up annotation_comment (Boshen) - 06197b8 codegen: Separate tests (Boshen) - aa22073 codegen: Improve print API (#4196) (Boshen) - c5731a5 semantic: Remove defunct code setting ScopeFlags twice (#4286) (overlookmotel) - 16698bc semantic: Move function/class-specific code into specific visitors (#4278) (overlookmotel) - ee16668 semantic: Rename function param (#4277) (overlookmotel) - 25f0771 semantic: Alter syntax of `control_flow!` macro (#4275) (overlookmotel) - 639fd48 semantic: Comment why extra CFG enabled check (#4274) (overlookmotel) - c418bf5 semantic: Directly record `current_node_id` when adding a scope (#4265) (Dunqing) - ace4f1f semantic: Update the order of `visit_function` and `Visit` fields in the builder to be consistent (#4248) (Dunqing) - 8bfeabf semantic: Simplify adding `SymbolFlags::Export` (#4249) (Dunqing) - dc2b3c4 semantic: Add strict mode in scope flags for class definitions (#4156) (Dunqing) - 81ed588 semantic: Convert scope fields to IndexVecs (#4208) (lucab) - bbe5ded semantic: Set `current_scope_id` to `scope_id` in `enter_scope` (#4193) (Dunqing) - 7f1addd semantic: Correct scope in CatchClause (#4192) (Dunqing) - fc0b17d syntax: Turn the `AstNodeId::dummy` into a constant field. (#4308) (rzvxa) - a197e01 transformer/typescript: Remove unnecessary code (#4321) (Dunqing) - 1458d81 visit: Add `#[inline]` to empty functions (#4330) (overlookmotel) Co-authored-by: Boshen <[email protected]>

github-actions bot added the A-semantic Area - Semantic label Jul 11, 2024

lucab marked this pull request as ready for review July 11, 2024 16:23

DonIsaac previously requested changes Jul 11, 2024

View reviewed changes

crates/oxc_semantic/src/scope.rs Show resolved Hide resolved

overlookmotel requested changes Jul 11, 2024

View reviewed changes

crates/oxc_semantic/src/scope.rs Outdated Show resolved Hide resolved

crates/oxc_semantic/src/scope.rs Show resolved Hide resolved

overlookmotel mentioned this pull request Jul 24, 2024

Record AstNodeId for all scopes in Semantic #4444

Closed

lucab force-pushed the ups/semantics-no-maps branch from f30973e to 5b24511 Compare July 12, 2024 07:10

lucab requested review from overlookmotel and DonIsaac July 12, 2024 08:51

overlookmotel approved these changes Jul 12, 2024

View reviewed changes

overlookmotel added the 0-merge Merge with Graphite Merge Queue label Jul 12, 2024

refactor(semantic): convert scope fields to IndexVecs (#4208)

81ed588

Closes: https://github.com/oxc-project/backlog/issues/10

overlookmotel force-pushed the ups/semantics-no-maps branch from 5b24511 to 81ed588 Compare July 12, 2024 09:44

graphite-app bot merged commit 81ed588 into main Jul 12, 2024
26 checks passed

graphite-app bot deleted the ups/semantics-no-maps branch July 12, 2024 09:47

lucab mentioned this pull request Jul 12, 2024

semantic: directly record real node ID when adding a scope to the scope-tree #4234

Closed

This was referenced Jul 15, 2024

Use IndexVec not FxHashMap for all fields of ScopeTree #4269

Closed

Optimize Semantic's structs-of-arrays and make API simpler oxc-project/backlog#11

Open

github-actions bot mentioned this pull request Jul 17, 2024

Release crates v0.21.0 #4325

Closed

github-actions bot mentioned this pull request Jul 18, 2024

Release crates v0.21.0 #4338

Merged

github-actions bot mentioned this pull request Jul 18, 2024

Release crates v0.21.0 #4344

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(semantic): convert scope fields to IndexVecs #4208

refactor(semantic): convert scope fields to IndexVecs #4208

lucab commented Jul 11, 2024 •

edited

Loading

graphite-app bot commented Jul 11, 2024

lucab commented Jul 11, 2024

codspeed-hq bot commented Jul 11, 2024 •

edited

Loading

overlookmotel commented Jul 11, 2024 •

edited

Loading

Dunqing commented Jul 12, 2024 •

edited

Loading

overlookmotel commented Jul 12, 2024

lucab commented Jul 12, 2024

graphite-app bot commented Jul 12, 2024 •

edited

Loading

overlookmotel commented Jul 12, 2024

lucab commented Jul 12, 2024

overlookmotel commented Jul 12, 2024

refactor(semantic): convert scope fields to IndexVecs #4208

refactor(semantic): convert scope fields to IndexVecs #4208

Conversation

lucab commented Jul 11, 2024 • edited Loading

graphite-app bot commented Jul 11, 2024

Your org has enabled the Graphite merge queue for merging into main

lucab commented Jul 11, 2024

codspeed-hq bot commented Jul 11, 2024 • edited Loading

CodSpeed Performance Report

Merging #4208 will not alter performance

Summary

overlookmotel commented Jul 11, 2024 • edited Loading

Dunqing commented Jul 12, 2024 • edited Loading

overlookmotel commented Jul 12, 2024

lucab commented Jul 12, 2024

graphite-app bot commented Jul 12, 2024 • edited Loading

Merge activity

overlookmotel commented Jul 12, 2024

lucab commented Jul 12, 2024

overlookmotel commented Jul 12, 2024

lucab commented Jul 11, 2024 •

edited

Loading

codspeed-hq bot commented Jul 11, 2024 •

edited

Loading

overlookmotel commented Jul 11, 2024 •

edited

Loading

Dunqing commented Jul 12, 2024 •

edited

Loading

graphite-app bot commented Jul 12, 2024 •

edited

Loading