Skip to content

perf(semantic): optimize Scoping::rename_symbol method#10612

Merged
Boshen merged 1 commit intomainfrom
04-25-perf_semantic_optimize_scoping_rename_symbol_method
Apr 27, 2025
Merged

perf(semantic): optimize Scoping::rename_symbol method#10612
Boshen merged 1 commit intomainfrom
04-25-perf_semantic_optimize_scoping_rename_symbol_method

Conversation

@Dunqing
Copy link
Member

@Dunqing Dunqing commented Apr 25, 2025 •
edited
Loading

Optimize Scoping::rename_symbol to avoid calling to_string, since the borrow checker is getting in the way, so the set_symbol_name and rename_binding method implementations need to be combined.

This was referenced Apr 25, 2025
Merged
Merged
Merged
@Dunqing Graphite App
Copy link
Member Author

Dunqing commented Apr 25, 2025 •
edited
Loading

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.

@Dunqing Dunqing mentioned this pull request Apr 25, 2025
Merged
@github-actions github-actions bot added A-semantic Area - Semantic C-performance Category - Solution not expected to change functional behavior, only performance labels Apr 25, 2025
@Dunqing Dunqing mentioned this pull request Apr 25, 2025
Merged
@codspeed-hq
Copy link

codspeed-hq bot commented Apr 25, 2025 •
edited
Loading

CodSpeed Instrumentation Performance Report

Merging #10612 will not alter performance

Comparing 04-25-perf_semantic_optimize_scoping_rename_symbol_method (cdd25c5) with main (f35efd3)

Summary

✅ 36 untouched benchmarks

@Dunqing Dunqing force-pushed the 04-25-refactor_traverse_semantic_move_rename_symbol_from_traversescoping_to_scoping branch from 43a0e3a to 1578ef4 Compare April 25, 2025 02:52
@Dunqing Dunqing force-pushed the 04-25-perf_semantic_optimize_scoping_rename_symbol_method branch from 9244388 to bb70f36 Compare April 25, 2025 02:52
@Dunqing Dunqing marked this pull request as ready for review April 25, 2025 02:58
@Dunqing Dunqing requested a review from Boshen as a code owner April 25, 2025 02:58
@Dunqing Dunqing mentioned this pull request Apr 25, 2025
Closed
@graphite-app graphite-app bot force-pushed the 04-25-refactor_traverse_semantic_move_rename_symbol_from_traversescoping_to_scoping branch from 1578ef4 to 0013165 Compare April 27, 2025 02:50
@graphite-app graphite-app bot requested a review from overlookmotel as a code owner April 27, 2025 02:50
@graphite-app graphite-app bot force-pushed the 04-25-perf_semantic_optimize_scoping_rename_symbol_method branch from bb70f36 to 03a4344 Compare April 27, 2025 02:50
@graphite-app graphite-app bot added the 0-merge Merge with Graphite Merge Queue label Apr 27, 2025
@graphite-app
Copy link
Contributor

graphite-app bot commented Apr 27, 2025 •
edited
Loading

Merge activity

  • Apr 27, 3:16 AM UTC: The merge label '0-merge' was detected. This PR will be added to the Graphite merge queue once it meets the requirements.
  • Apr 27, 3:35 AM UTC: graphite-app added this pull request to the Graphite merge queue.
  • Apr 27, 3:50 AM UTC: The Graphite merge queue removed this pull request due to downstack failures on PR #10610.

@Dunqing Dunqing force-pushed the 04-25-refactor_traverse_semantic_move_rename_symbol_from_traversescoping_to_scoping branch from 0013165 to e0bab74 Compare April 27, 2025 03:23
@Dunqing Dunqing force-pushed the 04-25-perf_semantic_optimize_scoping_rename_symbol_method branch from 03a4344 to d6170cd Compare April 27, 2025 03:23
@graphite-app graphite-app bot removed the 0-merge Merge with Graphite Merge Queue label Apr 27, 2025
@Boshen Boshen force-pushed the 04-25-perf_semantic_optimize_scoping_rename_symbol_method branch from d6170cd to 0189d98 Compare April 27, 2025 04:50
@Boshen Boshen force-pushed the 04-25-refactor_traverse_semantic_move_rename_symbol_from_traversescoping_to_scoping branch 2 times, most recently from e76a059 to ac31f1f Compare April 27, 2025 05:10
@Boshen Boshen force-pushed the 04-25-perf_semantic_optimize_scoping_rename_symbol_method branch from 0189d98 to 94b5642 Compare April 27, 2025 05:10
@Dunqing Dunqing force-pushed the 04-25-perf_semantic_optimize_scoping_rename_symbol_method branch from 94b5642 to b7ef8b1 Compare April 27, 2025 05:30
@Boshen Boshen added the 0-merge Merge with Graphite Merge Queue label Apr 27, 2025
@Boshen Boshen force-pushed the 04-25-perf_semantic_optimize_scoping_rename_symbol_method branch from b7ef8b1 to 52706e1 Compare April 27, 2025 11:57
@Boshen Boshen force-pushed the 04-25-refactor_traverse_semantic_move_rename_symbol_from_traversescoping_to_scoping branch from ac31f1f to c82ef6b Compare April 27, 2025 11:57
Boshen pushed a commit that referenced this pull request Apr 27, 2025
@Dunqing
refactor(traverse, semantic): move rename_symbol from `TraverseScop…
f35efd3 
…ing` to `Scoping` (#10611)

Part of oxc-project/backlog#163

Prepare #10612. I just found that this method has a `to_string`
allocation that can be avoided. However, it needs to combine
`set_symbol_name` and `rename_binding`, so move to `Semantic`.
Base automatically changed from 04-25-refactor_traverse_semantic_move_rename_symbol_from_traversescoping_to_scoping to main April 27, 2025 12:48
@github-actions github-actions bot added the A-transformer Area - Transformer / Transpiler label Apr 27, 2025
@Boshen Boshen force-pushed the 04-25-perf_semantic_optimize_scoping_rename_symbol_method branch from 52706e1 to cdd25c5 Compare April 27, 2025 13:18
@Boshen Boshen merged commit 075d4b5 into main Apr 27, 2025
25 checks passed
@Boshen Boshen deleted the 04-25-perf_semantic_optimize_scoping_rename_symbol_method branch April 27, 2025 13:26
Boshen pushed a commit that referenced this pull request Apr 27, 2025
@Dunqing
feat(semantic)!: do not need to return old name in `Scoping::set_symb…
a2b7e9d 
…ol_name` (#10613)

The only usage that needs the old name has been refactored in #10612.
Remove the returning old name logic, then the implementation is simpler.
@oxc-bot oxc-bot mentioned this pull request Apr 27, 2025
Merged
@overlookmotel overlookmotel mentioned this pull request May 2, 2025
Merged
graphite-app bot pushed a commit that referenced this pull request May 3, 2025
@overlookmotel
perf(semantic): reuse Atom (#10771)
3de1b31 
Follow-on after #10612. Small optimization. We already allocate `new_name` into `SemanticInner`'s arena. No need to allocate it again - just use the same string.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Boshen Boshen Awaiting requested review from Boshen

@overlookmotel overlookmotel Awaiting requested review from overlookmotel

Assignees

No one assigned

Labels

0-merge Merge with Graphite Merge Queue A-semantic Area - Semantic A-transformer Area - Transformer / Transpiler C-performance Category - Solution not expected to change functional behavior, only performance

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@Dunqing @Boshen