refactor(mangler): use bump allocator for internal storage

[camchenry]

No description provided.

[camchenry]

• refactor(mangler): use bump allocator for internal storage #11409 👈 (View in Graphite)
• refactor(mangler): use external allocator #11408
• main

---

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.

[codspeed-hq]

CodSpeed Instrumentation Performance Report

Merging #11409 will improve performances by 3.27%

_{Comparing 05-30-refactor_mangler_use_bump_allocator_for_internal_storage (bfa05f2) with 05-30-refactor_mangler_use_external_allocator (fa4966d)}

Summary

⚡ 1 improvements
✅ 37 untouched benchmarks

Benchmarks breakdown

	Benchmark	BASE	HEAD	Change
⚡	mangler[RadixUIAdoptionSection.jsx]	14.8 µs	14.3 µs	+3.27%

[overlookmotel]

⚡ mangler[RadixUIAdoptionSection.jsx] 15.1 µs 14.2 µs +6.57%

Nice! I am guessing that this isn't just noise. RadixUIAdoptionSection.jsx is the smallest test fixture, so it makes sense it moves the needle most here, as the allocations are a larger slice of the overall workload. Personally I think it's the most important benchmark we have as it's most representative of real-world code. Most people don't write 50,000-line files like checker.ts!

It's a shame that FixedBitset doesn't support custom allocators, or we could put them into the temp arena too. They're ideal candidates for putting in an arena as they never grow (high cost of growth is the only perf downside of arena-allocated data structures).

Probably we could write our own version of FixedBitset which allocates in an arena. We don't use most of FixedBitset's APIs, and I don't think the APIs we do use benefit from SIMD-acceleration, so our version could be a lot simpler. That could be fun! But, to be honest, mangler is so fast anyway that it's not the first place we should be looking for optimizations.

[camchenry]

It's a shame that FixedBitset doesn't support custom allocators, or we could put them into the temp arena too. They're ideal candidates for putting in an arena as they never grow (high cost of growth is the only perf downside of arena-allocated data structures).

We could at least put this into an arena-owned Vec if our Vec implementation supported resize_with by the way: https://doc.rust-lang.org/std/vec/struct.Vec.html#method.resize_with. The mangler is currently using this method with the FixedBitSet Vec. I didn't want to implement it in this PR stack but that's a future improvement we can make here as well.

[camchenry]

Perf improvement appears to be fairly consistent after refactoring as well:

⚡ mangler[RadixUIAdoptionSection.jsx] 15 µs 14.2 µs +5.59%

[overlookmotel]

We could at least put this into an arena-owned Vec if our Vec implementation supported resize_with

I don't think we can, unfortunately. oxc_allocator::Vec does not allow storing types which own heap memory (are Drop), because Vec does nothing on drop. So it'd leak memory if it contained types which rely on drop being called to free the memory they own. This is by design - it makes dropping an AST very fast. But consequently, putting FixedBitSets in arena would require a new impl which stores its data in arena, instead of on the heap, so it doesn't require Drop.

That said, we should implement Vec::resize_with. I'm not sure why we don't have it.

[camchenry]

I don't think we can, unfortunately. oxc_allocator::Vec does not allow storing types which own heap memory (are Drop), because Vec does nothing on drop. So it'd leak memory if it contained types which rely on drop being called to free the memory they own. This is by design - it makes dropping an AST very fast. But consequently, putting FixedBitSets in arena would require a new impl which stores its data in arena, instead of on the heap, so it doesn't require Drop.

Makes sense, I thought it was simply the lack of FixedBitSet::with_capacity_in or a similar function, thanks for the explanation.

[camchenry]

Merge activity

• Jun 8, 10:39 PM UTC: The merge label '0-merge' was detected. This PR will be added to the Graphite merge queue once it meets the requirements.
• Jun 8, 10:49 PM UTC: graphite-app added this pull request to the Graphite merge queue.
• Jun 8, 10:58 PM UTC: Merged by the Graphite merge queue.