Skip to content

Comments

refactor(transformer/class-properties): defer re-parenting initializers scopes until all instance properties are transformed#10495

Merged
graphite-app[bot] merged 1 commit intomainfrom
04-18-refactor_transformer_class-properties_defer_reparent_initializers_scopes_after_all_instance_properties_are_collected
Apr 22, 2025
Merged

refactor(transformer/class-properties): defer re-parenting initializers scopes until all instance properties are transformed#10495
graphite-app[bot] merged 1 commit intomainfrom
04-18-refactor_transformer_class-properties_defer_reparent_initializers_scopes_after_all_instance_properties_are_collected

Conversation

@Dunqing
Copy link
Member

@Dunqing Dunqing commented Apr 18, 2025

Re-parenting the initializers' scope after all instance properties are transformed, which is preparing for #10493. And rename transform_instance_initializer to reparent_initializers_scope because the previous name confused me; the implementation only affects scope data.

Moreover, there is a drawback to this change. The reparent_initializers_scope method now processes initializers that have been transformed, so that it will visit many unrelated AST nodes. See the transformer/pdf.js benchmark, it shows 0.1ms slower than before. It may not matter; we will get it back in the follow-up changes.

Copy link
Member Author

Dunqing commented Apr 18, 2025


How to use the Graphite Merge Queue

Add either label to this PR to merge it via the merge queue:

  • 0-merge - adds this PR to the back of the merge queue
  • hotfix - for urgent hot fixes, skip the queue and merge this PR next

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.

@github-actions github-actions bot added A-transformer Area - Transformer / Transpiler C-cleanup Category - technical debt or refactoring. Solution not expected to change behavior labels Apr 18, 2025
@codspeed-hq
Copy link

codspeed-hq bot commented Apr 18, 2025

CodSpeed Instrumentation Performance Report

Merging #10495 will create unknown performance changes

Comparing 04-18-refactor_transformer_class-properties_defer_reparent_initializers_scopes_after_all_instance_properties_are_collected (f13fe0a) with main (c83dad6)

Summary

🆕 36 new benchmarks

Benchmarks breakdown

Benchmark BASE HEAD Change
🆕 codegen[checker.ts] N/A 22.2 ms N/A
🆕 codegen_sourcemap[checker.ts] N/A 65.1 ms N/A
🆕 formatter[antd.js] N/A 708.4 ms N/A
🆕 formatter[react.development.js] N/A 8 ms N/A
🆕 formatter[typescript.js] N/A 1.1 s N/A
🆕 isolated-declarations[vue-id.ts] N/A 58.3 ms N/A
🆕 lexer[RadixUIAdoptionSection.jsx] N/A 21.3 µs N/A
🆕 lexer[antd.js] N/A 24.8 ms N/A
🆕 lexer[cal.com.tsx] N/A 5.9 ms N/A
🆕 lexer[checker.ts] N/A 14.9 ms N/A
🆕 lexer[pdf.mjs] N/A 3.9 ms N/A
🆕 linter[RadixUIAdoptionSection.jsx] N/A 2.7 ms N/A
🆕 linter[cal.com.tsx] N/A 1.2 s N/A
🆕 linter[checker.ts] N/A 3 s N/A
🆕 mangler[antd.js] N/A 15.9 ms N/A
🆕 mangler[react.development.js] N/A 293.4 µs N/A
🆕 mangler[typescript.js] N/A 39.4 ms N/A
🆕 minifier[antd.js] N/A 163.6 ms N/A
🆕 minifier[react.development.js] N/A 1.8 ms N/A
🆕 minifier[typescript.js] N/A 287.9 ms N/A
... ... ... ... ...

ℹ️ Only the first 20 benchmarks are displayed. Go to the app to view all benchmarks.

@Dunqing Dunqing changed the title refactor(transformer/class-properties): defer reparent initializers scopes after all instance properties are collected refactor(transformer/class-properties): defer re-parenting initializers scopes until all instance properties are collected Apr 21, 2025
@Dunqing Dunqing changed the title refactor(transformer/class-properties): defer re-parenting initializers scopes until all instance properties are collected refactor(transformer/class-properties): defer re-parenting initializers scopes until all instance properties are transformed Apr 21, 2025
@Dunqing Dunqing marked this pull request as ready for review April 21, 2025 02:55
@Dunqing Dunqing requested a review from overlookmotel as a code owner April 21, 2025 02:55
@overlookmotel overlookmotel added the 0-merge Merge with Graphite Merge Queue label Apr 22, 2025
Copy link
Member

overlookmotel commented Apr 22, 2025

Merge activity

…rs scopes until all instance properties are transformed (#10495)

Re-parenting the initializers' scope after all instance properties are transformed, which is preparing for #10493. And rename `transform_instance_initializer` to `reparent_initializers_scope` because the previous name confused me; the implementation only affects scope data.

Moreover, there is a drawback to this change. The `reparent_initializers_scope` method now processes initializers that have been transformed, so that it will visit many unrelated AST nodes. See the transformer/pdf.js benchmark, it shows `0.1ms` slower than before. It may not matter; we will get it back in the follow-up changes.
graphite-app bot pushed a commit that referenced this pull request Apr 22, 2025
…instance property initializer (#10492)

Move the `instance_inits_scope_id` and `instance_inits_constructor_scope_id` fields from the `ClassProperties` struct to local variables.

Using struct-level fields as temporary variables is unnecessary within three function depths, and you also need to consider when to reset the states, otherwise, you may cause a bug due to using previous temporary information. Moreover, I think it also improves a bit readability.

NOTE: This is not only a refactor, but also along with #10495 and #10493 to prepare #10491.
@graphite-app graphite-app bot force-pushed the 04-18-refactor_transformer_class-properties_streamline_handling_scope_of_instance_property_initializer branch from f24b9e2 to c83dad6 Compare April 22, 2025 11:21
@graphite-app graphite-app bot force-pushed the 04-18-refactor_transformer_class-properties_defer_reparent_initializers_scopes_after_all_instance_properties_are_collected branch from 5ca39af to f13fe0a Compare April 22, 2025 11:21
Base automatically changed from 04-18-refactor_transformer_class-properties_streamline_handling_scope_of_instance_property_initializer to main April 22, 2025 11:27
@graphite-app graphite-app bot removed the 0-merge Merge with Graphite Merge Queue label Apr 22, 2025
@graphite-app graphite-app bot merged commit f13fe0a into main Apr 22, 2025
26 checks passed
@graphite-app graphite-app bot deleted the 04-18-refactor_transformer_class-properties_defer_reparent_initializers_scopes_after_all_instance_properties_are_collected branch April 22, 2025 11:29
graphite-app bot pushed a commit that referenced this pull request Apr 22, 2025
… method only once (#10493)

Benefit from #10495, we can move getting `instance_inits_scope_id` and `instance_inits_constructor_scope_id` part to after all instance properties transformed. So that we can avoid to get `constructor` twice due to borrow checker.
graphite-app bot pushed a commit that referenced this pull request Apr 22, 2025
… method only once (#10493)

Benefit from #10495, we can move getting `instance_inits_scope_id` and `instance_inits_constructor_scope_id` part to after all instance properties transformed. So that we can avoid to get `constructor` twice due to borrow checker.
graphite-app bot pushed a commit that referenced this pull request Apr 23, 2025
…r` (#10543)

Follow-on after #10495.

1. Branch on whether `instance_inits_constructor_scope_id` is `Some` only once, rather than on each turn of the loop.
2. Only create one `InstanceInitializerVisitor` for all the inits, rather than a new one for each init. `InstanceInitializerVisitor` contains a `Stack`, so ideal to re-use it, because `Stack` performs an allocation.

These are small optimizations. I doubt will register on benchmarks.
Boshen added a commit that referenced this pull request Apr 23, 2025
## [0.66.0] - 2025-04-23

### Features

- 7d5ad7d parser: Report error when `import type { type }` is used
(#10528) (camc314)
- 6e40fac parser: Report error when `export type { type }` is used
(#10524) (camc314)

### Bug Fixes

- 43ad4e9 ast: Box `this_param` in `TSCallSignatureDeclaration` (#10558)
(Yuji Sugiura)
- 8eb3c0a ast/estree: Fix raw deser for `FormalParameter` (#10548)
(overlookmotel)
- 3ebf220 codegen: Generate missing `type` in `export type {} from
'mod'` (#10539) (Boshen)
- f19b287 estree: Add `TSParameterProperty` (#10534) (Yuji Sugiura)
- 7284f16 isolated-declarations: Leading comments of
`ExportDefaultDeclaration` and `TSExportAssignment` appear in incorrect
places (#10559) (Dunqing)
- 4c316a5 isolated_declarations: Fix broken snapshot files (#10561)
(Boshen)
- 84742d8 transformer/module_runner: Fix export default live binding
(#10560) (hi-ogawa)
- 6c115c7 transformer/module_runner: Fix execution order of re-export
(#10362) (hi-ogawa)
- b9d84b0 transformer/typescript: Remove `StringLiteral::raw` when
rewriting extensions (#10554) (overlookmotel)

### Performance

- 6a045c8 codegen: Speed up printing `Directive`s (#10551)
(overlookmotel)
- 3418ae5 transformer/class-properties: Re-use
`InstanceInitializerVisitor` (#10543) (overlookmotel)
- 7fcf0ac transformer/typescript: Reduce allocations renaming extensions
(#10555) (overlookmotel)

### Documentation

- 1a9530c transformer/class-properties: Fix typo (#10542)
(overlookmotel)

### Refactor

- 7e71282 transformer/class-properties: Correct outdated comment
(#10546) (overlookmotel)
- 596978b transformer/class-properties: Simplify to get `constructor`
method only once (#10493) (Dunqing)
- f13fe0a transformer/class-properties: Defer re-parenting initializers
scopes until all instance properties are transformed (#10495) (Dunqing)
- c83dad6 transformer/class-properties: Streamline handling scope of
instance property initializer (#10492) (Dunqing)

### Styling

- e10dfc8 traverse: Remove excess whitespace (#10544) (overlookmotel)

### Testing

- 227febf allocator: Ignore a slow doc test (#10521) (Dunqing)
- 14bb2be codegen: Add more tests for strings (#10552) (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-transformer Area - Transformer / Transpiler C-cleanup Category - technical debt or refactoring. Solution not expected to change behavior

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants