Skip to content
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

perf(semantic): remove span field from Reference #4464

Merged

Conversation

overlookmotel
Copy link
Collaborator

@overlookmotel overlookmotel commented Jul 25, 2024

Remove span field from Reference. Span can instead be got via looking up span on AstKind via reference.node_id.

This shrinks Reference from 20 bytes to 12 bytes.

It does make getting span of a Reference a bit slower, as there's an extra table lookup involved, but the only places this is needed is in linter, and almost always when generating a diagnostic (i.e. cold path).

@github-actions github-actions bot added A-linter Area - Linter A-semantic Area - Semantic labels Jul 25, 2024
@overlookmotel overlookmotel force-pushed the 07-25-perf_semantic_remove_span_field_from_reference_ branch from 66abf34 to 4497fa3 Compare July 25, 2024 16:26
Copy link

graphite-app bot commented Jul 25, 2024

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.

Copy link

codspeed-hq bot commented Jul 25, 2024

CodSpeed Performance Report

Merging #4464 will not alter performance

Comparing 07-25-perf_semantic_remove_span_field_from_reference_ (348c1ad) with main (a49f491)

Summary

✅ 32 untouched benchmarks

Copy link

graphite-app bot commented Jul 26, 2024

Merge activity

Remove `span` field from `Reference`. Span can instead be got via looking up span on `AstKind` via `reference.node_id`.

This shrinks `Reference` from 20 bytes to 12 bytes.

It does make getting span of a `Reference` a bit slower, as there's an extra table lookup involved, but the only places this is needed is in linter, and almost always when generating a diagnostic (i.e. cold path).
@Boshen Boshen force-pushed the 07-25-perf_semantic_reduce_storage_size_for_symbol_redeclarations branch from 3d07015 to 6a9f4db Compare July 26, 2024 00:16
@Boshen Boshen force-pushed the 07-25-perf_semantic_remove_span_field_from_reference_ branch from 3f2041f to 348c1ad Compare July 26, 2024 00:17
Boshen pushed a commit that referenced this pull request Jul 26, 2024
…le (#4465)

In `nextjs:no_duplicate_head` rule, avoid allocating a `Vec` unless more than one `<Head>` is found (uncommon case).

Also, #4464 made getting span of a `Reference` a little more expensive. Avoid this work unless it's needed (which it generally won't be).
@Boshen Boshen changed the base branch from 07-25-perf_semantic_reduce_storage_size_for_symbol_redeclarations to main July 26, 2024 00:27
@graphite-app graphite-app bot merged commit 348c1ad into main Jul 26, 2024
25 checks passed
@graphite-app graphite-app bot deleted the 07-25-perf_semantic_remove_span_field_from_reference_ branch July 26, 2024 00:29
@Dunqing
Copy link
Member

Dunqing commented Jul 26, 2024

This PR has breaking changes for the downstream. Rolldown only uses ScopeTree and SymbolTable. Removing the span field from the Reference causes Rolldown to not get the reference span. Of course, ideally the rolldown should use Semantic rather than get a portion of Semantic to use

@hyf0 cc

@Boshen
Copy link
Member

Boshen commented Jul 26, 2024

Revert or fix?

@Dunqing
Copy link
Member

Dunqing commented Jul 26, 2024

Revert or fix?

Fixed in Rolldown

@overlookmotel
Copy link
Collaborator Author

@Dunqing Oh dear, I didn't consider that this could break downstream. How did you fix in Rolldown?

@Dunqing
Copy link
Member

Dunqing commented Jul 27, 2024

@Dunqing Oh dear, I didn't consider that this could break downstream. How did you fix in Rolldown?

@hyf0 fixed this. rolldown/rolldown@35a7f9b.

@oxc-bot oxc-bot mentioned this pull request Jul 27, 2024
Dunqing pushed a commit that referenced this pull request Jul 28, 2024
## [0.22.1] - 2024-07-27

### Features

- 2477330 ast: Add `AstKind::TSExportAssignment` (#4501) (Dunqing)
- aaee07e ast: Add `AstKind::AssignmentTargetPattern`,
`AstKind::ArrayAssignmentTarget` and `AstKind::ObjectAssignmentTarget`
(#4456) (Dunqing)
- fd363d1 ast: Add AstKind::get_container_scope_id (#4450) (DonIsaac)
- e2735ca span: Add `contains_inclusive` method (#4491) (DonIsaac)

### Bug Fixes

- 368112c ast: Remove `#[visit(ignore)]` from
`ExportDefaultDeclarationKind`'s `TSInterfaceDeclaration` (#4497)
(Dunqing)
- 36bb680 semantic: `TSExportAssignment` cannot reference type binding
(#4502) (Dunqing)
- cb2fa49 semantic: `typeof` operator cannot reference type-only import
(#4500) (Dunqing)
- ef0e953 semantic: Generic passed to typeof not counted as a reference
(#4499) (Dunqing)
- 40cafb8 semantic: Params in `export default (function() {})` flagged
as `SymbolFlags::Export` (#4480) (Dunqing)
- 2e01a45 semantic: Non-exported namespace member symbols flagged as
exported (#4493) (Don Isaac)
- e4ca06a semantic: Incorrect symbol’s scope_id after var hoisting
(#4458) (Dunqing)
- 77bd5f1 semantic: Use correct span for namespace symbols (#4448) (Don
Isaac)
- 5db7bed sourcemap: Fix pre-calculation of required segments for
building JSON (#4490) (overlookmotel)
- 1667491 syntax: Correct `is_reserved_keyword_or_global_object`'s
incorrect function calling. (#4484) (Ethan Goh)
- 82ba2a0 syntax: Fix unsound use of `NonZeroU32` (#4466)
(overlookmotel)
- c04b9aa transformer: Add to `SymbolTable::declarations` for all
symbols (#4460) (overlookmotel)
- ecdee88 transformer/typescript: Incorrect eliminate exports when the
referenced symbol is both value and type (#4507) (Dunqing)

### Performance

- 963a2d1 mangler: Reduce unnecessary allocation (#4498) (Dunqing)
- 868fc87 parser: Optimize conditional advance on ASCII values (#4298)
(lucab)
- 24beaeb semantic: Give `AstNodeId` a niche (#4469) (overlookmotel)
- 348c1ad semantic: Remove `span` field from `Reference` (#4464)
(overlookmotel)
- 6a9f4db semantic: Reduce storage size for symbol redeclarations
(#4463) (overlookmotel)
- 705e19f sourcemap: Reduce memory copies encoding JSON (#4489)
(overlookmotel)
- 4d10c6c sourcemap: Pre allocate String buf while encoding (#4476)
(Brooooooklyn)

### Documentation

- f5f0ba8 ast: Add doc comments to more AST nodes (#4413) (Don Isaac)
- 871b3d6 semantic: Add doc comments for SymbolTester and SemanticTester
(#4433) (DonIsaac)

### Refactor

- 9c5d2f9 ast/builder: Use `Box::new_in` over `.into_in` (#4428)
(overlookmotel)
- ccb1835 semantic: Methods take `Span` as param, not `&Span` (#4470)
(overlookmotel)
- f17254a semantic: Populate `declarations` field in
`SymbolTable::create_symbol` (#4461) (overlookmotel)
- a49f491 semantic: Re-order `SymbolTable` fields (#4459)
(overlookmotel)
- 7cd53f3 semantic: Var hoisting (#4379) (Dunqing)
- 4f5a7cb semantic: Mark SemanticTester and SymbolTester as must_use
(#4430) (DonIsaac)
- c958a55 sourcemap: `push_list` method for building JSON (#4486)
(overlookmotel)
- c99b3eb syntax: Give `ScopeId` a niche (#4468) (overlookmotel)
- 96fc94f syntax: Use `NonMaxU32` for IDs (#4467) (overlookmotel)

### Testing

- 4b274a8 semantic: Add more test cases for symbol references (#4429)
(DonIsaac)

Co-authored-by: Boshen <[email protected]>
@oxc-bot oxc-bot mentioned this pull request Aug 5, 2024
Boshen added a commit that referenced this pull request Aug 5, 2024
## [0.7.0] - 2024-08-05

- 85a7cea semantic: [**BREAKING**] Remove name from `reference` (#4329)
(Dunqing)

### Features

- aaee07e ast: Add `AstKind::AssignmentTargetPattern`,
`AstKind::ArrayAssignmentTarget` and `AstKind::ObjectAssignmentTarget`
(#4456) (Dunqing)
- 9df7b56 jsx-a11y/no-autofocus: Implement fixer support (#4171) (Jelle
van der Waa)
- b87bf70 linter: Add fix capabilties to existing lint rules (#4560)
(DonIsaac)
- ddd8b27 linter: Support conditional fix capabilities (#4559)
(DonIsaac)
- b952942 linter: Add eslint/no-unused-vars (⭐ attempt 3.2) (#4445)
(DonIsaac)
- 6543958 linter: Add auto-fix metadata to RuleMeta (#4557) (Don Isaac)
- 85e8418 linter: Add react/jsx-curly-brace-presence (#3949) (Don Isaac)
- 4c4da56 linter: Add typescript-eslint/prefer-keyword-namespce (#4438)
(Aza Walker)
- d8c2a83 linter: Eslint-plugin-vitest/no-import-node-test (#4440)
(cinchen)
- e3b0c40 linter: Eslint-plugin-vitest/no-identical-title (#4422)
(cinchen)
- c936782 linter: Eslint-plugin-vitest/no-conditional-expect (#4425)
(cinchen)
- 27fdd69 linter: Eslint-plugin-vitest/no-commented-out-tests (#4424)
(cinchen)
- 51f5025 linter: Add fixer for unicorn/prefer-string-starts-ends-with
(#4378) (DonIsaac)
- 3c0c709 linter: Add typescript-eslint/no-extraneous-class (#4357)
(Jaden Rodriguez)
- 7afa1f0 linter: Support suggestions and dangerous fixes (#4223)
(DonIsaac)
- acc5729 linter: Eslint-plugin-vitest/expect-expect (#4299) (cinchen)
- 2213f93 linter: Eslint-plugin-vitest/no-alias-methods (#4301)
(cinchen)
- c296bc3 linter/eslint: Implement func-names (#4618) (Alexander S.)
- e116ae0 linter/eslint: Implement fixer for prefer-numeric-literals
(#4591) (Jelle van der Waa)
- eaf834f linter/eslint: Implement prefer-numeric-literals (#4109)
(Jelle van der Waa)
- db2fd70 linter/eslint-plugin-promise: Implement
no-webpack-loader-syntax (#4331) (Jelle van der Waa)
- 5f1e070 linter/eslint-plugin-unicorn: Add fixer for prefer-code-point
(#4353) (Jelle van der Waa)
- ed49e16 linter/eslint-plugin-unicorn: Implement fixer for
prefer-dom-node-append (#4306) (Jelle van der Waa)
- e2b15ac linter/react: Implement react-jsx-boolean-value (#4613) (Jelle
van der Waa)
- 68efcd4 linter/react-perf: Handle new objects and arrays in prop
assignment patterns (#4396) (DonIsaac)

### Bug Fixes

- 368112c ast: Remove `#[visit(ignore)]` from
`ExportDefaultDeclarationKind`'s `TSInterfaceDeclaration` (#4497)
(Dunqing)
- d384f60 ci: Remove unused(?) .html file (#4545) (Yuji Sugiura)
- 06aec77 linter: Invalid binary expression with overflow (#4647)
(DonIsaac)
- b2da22b linter: Invalid tags in rule docs (#4646) (DonIsaac)
- 94440ad linter: Panic on invalid lang in `a11y/lang`. (#4630) (rzvxa)
- e0b03f8 linter: Improve the boundary for eslint/for-direction (#4590)
(heygsc)
- 70b8cfa linter: Missing return in no-obj-calls recursion (#4594)
(DonIsaac)
- fe1356d linter: Change no-unused-vars to nursery (#4588) (DonIsaac)
- 72337b1 linter: Change typescript-eslint/no-namespace to restriction
(#4539) (Don Isaac)
- 732f4e2 linter: Fix `oxlint` allocator cfg (#4527) (overlookmotel)
- 289dc39 linter: Overflow in no-obj-calls (#4397) (DonIsaac)
- a664715 linter/eslint: Fix invalid regexp in no_regex_spaces test
(#4605) (Yuji Sugiura)
- 74fa75a linter/eslint: Drop quotes around max-params lint warning
(#4608) (Jelle van der Waa)
- 9fcd9ae linter/eslint: Fix invalid regexp in no_control_regex test
(#4544) (leaysgur)
- ac08de8 linter/react_perf: Allow new objects, array, fns, etc in top
scope (#4395) (DonIsaac)
- 0fba738 npm: SyntaxError caused by optional chaining in low version
node (#4650) (heygsc)
- 73d2558 oxlint: Fix oxlint failed to build due to missing feature
(Boshen)

### Performance

- 6ff200d linter: Change react rules and utils to use `Cow` and
`CompactStr` instead of `String` (#4603) (DonIsaac)
- f259df0 linter: Make img-redundant-alt only build a regex once (#4604)
(DonIsaac)
- 7585e16 linter: Remove allocations for string comparisons (#4570)
(DonIsaac)
- b60bdf1 linter: `no_shadow_restricted_names` only look up name in
hashmap once (#4472) (overlookmotel)
- 81384f5 linter: Avoid unnecessary work in `nextjs:no_duplicate_head`
rule (#4465) (overlookmotel)
- f7da22d linter: Disable lint rules by file type (#4380) (DonIsaac)
- 348c1ad semantic: Remove `span` field from `Reference` (#4464)
(overlookmotel)
- 6a9f4db semantic: Reduce storage size for symbol redeclarations
(#4463) (overlookmotel)- a207923 Replace some CompactStr usages with
Cows (#4377) (DonIsaac)

### Refactor

- 7a75e0f linter: Use diagnostic codes in lint rules (#4349) (DonIsaac)
- ccb1835 semantic: Methods take `Span` as param, not `&Span` (#4470)
(overlookmotel)
- 7cd53f3 semantic: Var hoisting (#4379) (Dunqing)
- c99b3eb syntax: Give `ScopeId` a niche (#4468) (overlookmotel)

Co-authored-by: Boshen <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-linter Area - Linter A-semantic Area - Semantic
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants