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.

Sign up for GitHub

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

Jump to bottom

transformer: performance regression from generated_uid #5488

Closed
Boshen opened this issue Sep 5, 2024 · 4 comments · Fixed by #5611
Closed

transformer: performance regression from generated_uid #5488

Boshen opened this issue Sep 5, 2024 · 4 comments · Fixed by #5611
Assignees
@overlookmotel
Labels
A-transformer Area - Transformer / Transpiler C-bug Category - Bug C-performance Category - Solution not expected to change functional behavior, only performance

Comments

@Boshen
Copy link
Member

Boshen commented Sep 5, 2024

image

It's inside OptionalCatchBinding -> enter_catch_clause.
@Boshen Boshen added the C-bug Category - Bug label Sep 5, 2024
@overlookmotel
Copy link
Contributor

Do you suspect this is the root cause of #5457?

@Boshen
Copy link
Member Author

Boshen commented Sep 5, 2024

Nope, I just checked cal.com.tsx doesn't have optional catch clause 🤔

Giving it a spin anyway: #5489

@Boshen
Copy link
Member Author

Boshen commented Sep 5, 2024

Nope, I just checked cal.com.tsx doesn't have optional catch clause 🤔

Giving it a spin anyway: #5489

⚡ | transformer[pdf.mjs] | 10.6 ms | 7.4 ms | +43.59%

@Boshen
Copy link
Member Author

Boshen commented Sep 5, 2024 •
edited
Loading

Why do I feel like we broke codspeed, this is the profile for transformer on cal.com.tsx, filtered on generated_uid. Surely all benchmarks should've improved.

image

@Boshen Boshen added A-transformer Area - Transformer / Transpiler C-performance Category - Solution not expected to change functional behavior, only performance labels Sep 5, 2024
@Boshen Boshen changed the title transformer: insane performance regression from generated_uid transformer: performance regression from generated_uid Sep 7, 2024
@overlookmotel overlookmotel mentioned this issue Sep 8, 2024
Merged
Boshen pushed a commit that referenced this issue Sep 10, 2024
@overlookmotel
perf(traverse): generate_uid cache available binding names (#5611)
4996874 
Close #5488.

`generate_uid` previously iterated through every symbol and unresolved reference in the AST to find a unique var name. If the first var name it tried was already in use, it'd iterate again.

Instead build a hash map recording existing var names in use for every name which could clash with a UID (any var name starting with `_`). Once built, use that hash map to generate UIDs without iterating through all symbols again.

I had hoped to make `generate_uid` cheaper still by just recording the highest digits postfix for each var name, and then incrementing that postfix for each UID. i.e. if AST contains vars `_foo1` and `_foo6`, create UIDs starting at one number higher - `_foo7`, `_foo8` etc. This method would be more efficient, but unfortunately it does not match Babel, and so causes some of Babel's tests to fail.
@Boshen Boshen closed this as completed Sep 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@overlookmotel overlookmotel

Labels
A-transformer Area - Transformer / Transpiler C-bug Category - Bug C-performance Category - Solution not expected to change functional behavior, only performance
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

perf(traverse): generate_uid cache available binding names oxc-project/oxc
2 participants
@overlookmotel @Boshen