Skip to content

Fix mangler benchmarks and speed up the mangler #11347

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

Open
overlookmotel opened this issue May 28, 2025 · 1 comment
Open

Fix mangler benchmarks and speed up the mangler #11347

overlookmotel opened this issue May 28, 2025 · 1 comment
Labels
A-minifier Area - Minifier C-performance Category - Solution not expected to change functional behavior, only performance E-Help Wanted Experience level - For the experienced collaborators

Comments

@overlookmotel
Copy link
Contributor

overlookmotel commented May 28, 2025

The mangler benchmarks are extremely noisy, often reporting +/- 15% perf differences on PRs which don't touch the mangler at all. The mangler[cal.com.tsx] benchmark is the worst offender, but other mangler benchmarks are also displaying a lot of variance.

Presumably the source of this variance is unpredictable patterns of allocation.

The mangler already uses an Allocator internally for storing some temporary data. I suggest we try to put more of the temporary Vecs and HashMaps used in the mangler into this arena too. That should make the pattern of allocations more deterministic, and so reduce the variance in benchmarks. I would hope it may also be a performance gain, as it'll reduce allocations overall.

One note: When storing data in an arena, it becomes much more important to accurately predict upfront how large a Vec / HashMap is going to get, and reserve sufficient capacity when creating the Vec / HashMap.

Reserving sufficient capacity upfront is always a gain, because growing these structures is an expensive operation, especially if they're large. But when the Vec / HashMap is stored in an arena, it becomes even more important. When Vec / HashMap is stored on general heap, sometimes the OS can grow the allocation in place (very cheap), or resize it by altering the page table (somewhat cheap). But when the Vec / HashMap is in an arena, neither of these is an option, and growth always results in creating a new allocation and copying all the data from the old allocation to the new one (very expensive).

@overlookmotel overlookmotel added E-Help Wanted Experience level - For the experienced collaborators A-minifier Area - Minifier labels May 28, 2025
@overlookmotel
Copy link
Contributor Author

overlookmotel commented May 28, 2025

Actually, looking flame graphs on CodSpeed for PRs which report ~15% regression on mangler[cal.com.tsx] benchmarks, the main source of the variance is allocating memory for the Allocator used for temp allocations (bumpalo::Bump::alloc_layout), and this is really expensive in the context of the mangler.

e.g.: https://codspeed.io/oxc-project/oxc/branches/05-26-perf_parser_remove_branch_parsing_class_elements

What we should ideally do is: Instead of creating a fresh Allocator inside the mangler each time, create one outside and pass in &Allocator. After mangling one file, that Allocator can be reset and then re-used when mangling the next file. This would both remove the benchmark variance, and speed up the mangler a lot.

Note: This is the same pattern we use when parsing and transforming multiple files. The parser is provided an &Allocator which it stores the AST in. Once we're done with that file, that allocator is reset and reused for the next file, rather than creating a new Allocator each time.

The question is: How to make this ergonomic for the user? Having to create two allocators and pass the right one into various methods is confusing and annoying!

@overlookmotel overlookmotel changed the title Fix mangler benchmarks Fix mangler benchmarks and speed up the mangler May 28, 2025
@overlookmotel overlookmotel added the C-performance Category - Solution not expected to change functional behavior, only performance label May 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-minifier Area - Minifier C-performance Category - Solution not expected to change functional behavior, only performance E-Help Wanted Experience level - For the experienced collaborators
Projects
None yet
Development

No branches or pull requests

1 participant