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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

BatchRenamer, 100x faster scope.rename when you have many identifiers to rename #16472

Open
1 task
remorses opened this issue May 4, 2024 · 6 comments
Open
1 task

Comments

@remorses
Copy link

remorses commented May 4, 2024

馃捇

  • Would you like to work on this feature?

What problem are you trying to solve?

scope.rename is slow when you want to rename many identifiers, the complexity grows with the number of identifiers

Describe the solution you'd like

BatchRenamer, an utility to rename many identifiers in one traversal

const renamer = new BatchRenamer(
    path.scope,
    new Map(['old', 'new'])
)
renamer.rename()

// or attach a method directly on scope
scope.batchRename(new Map(['old', 'new']))

Describe alternatives you've considered

call scope.rename N times, time complexity grows with N, you make N traversals of the full AST

Documentation, Adoption, Migration Strategy

I have already written an implementation i am already using, it adds minimal overhead compared to the already existing Renamer, in my case i had 100 identifiers to rename and it becomes 100x faster.

I am not familiar with the Babel codebase, if someone wants to open a PR to add the code feel free to do it

https://gist.github.com/remorses/9a11d96f9f00d3af1388a197be2a7878

@babel-bot
Copy link
Collaborator

Hey @remorses! We really appreciate you taking the time to report an issue. The collaborators on this project attempt to help as many people as possible, but we're a limited number of volunteers, so it's possible this won't be addressed swiftly.

If you need any help, or just have general Babel or JavaScript questions, we have a vibrant Slack community that typically always has someone willing to help. You can sign-up here for an invite.

@nicolo-ribaudo
Copy link
Member

nicolo-ribaudo commented May 4, 2024

Thanks for this!

Maybe we could make rename accept an array of string | string[]?

@remorses
Copy link
Author

remorses commented May 4, 2024

In my code I used a Map that goes from old names to new names. I am not sure if this feature would be used enough and if it would make sense to add to babel core

@nicolo-ribaudo
Copy link
Member

We would at least use it in the commonjs transform, where we rename multiple identifiers one after the other, and probably in the let/const transform where we can batch-rename variables when converting them to var.

@j4k0xb
Copy link

j4k0xb commented May 6, 2024

Why is a full traverse needed in the first place? Got a massive performance boost by looping over binding.referencePaths/constantViolations instead (without batching)

@liuxingbaoyu
Copy link
Member

Why is a full traverse needed in the first place? Got a massive performance boost by looping over binding.referencePaths/constantViolations instead (without batching)

Because binding.referencePaths/constantViolations may be inaccurate, using them may have some risks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants