Skip to content

refactor(allocator): use latest bumpalo as base for Bump#20963

Merged
overlookmotel merged 11 commits intomainfrom
om/04-01-refactor_allocator_use_latest_bumpalo_as_base_for_bump_
Apr 2, 2026
Merged

refactor(allocator): use latest bumpalo as base for Bump#20963
overlookmotel merged 11 commits intomainfrom
om/04-01-refactor_allocator_use_latest_bumpalo_as_base_for_bump_

Conversation

@overlookmotel
Copy link
Copy Markdown
Member

@overlookmotel overlookmotel commented Apr 2, 2026

#18168 copied bumpalo's code for Bump into oxc_allocator crate. #18172, #18181, and #18234 made a few improvements to the implementation.

However, #18168 removed various things, and altered others. Notably, it removed theMIN_ALIGN generic param, which we definitely want to keep.

I'm going to be working on the allocator, and would like to start with a clean slate, beginning with the "known good" implementation of bumpalo.

This PR removes the previous modified bumpalo implementation, and copies latest version of bumpalo from main branch into the repo. It then makes the absolute minimum changes necessary just to make CI pass.

Note: Unlike #18168, this PR keeps all the doctests for Bump, using a trick of adding the crate to it's own dev-dependencies with testing feature enabled, and exposing Bump only with that feature.

This PR is broken into multiple commits, so we have an exact "trail of breadcrumbs" of how we've diverged from bumpalo. I intend to manually merge this PR, so that record remains on Github (instead of Graphite squashing all the commits).

Later PRs in this stack reapply the changes made in #18172, #18181, and #18234 on top of the fresh base implementation. This PR is a small perf regression, but that will be won back once those other changes are reapplied.

Copy link
Copy Markdown
Member Author

overlookmotel commented Apr 2, 2026


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 changes, fast-track this PR to the front of the merge queue

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
Copy link
Copy Markdown

codspeed-hq bot commented Apr 2, 2026

Merging this PR will improve performance by 10.09%

⚡ 1 improved benchmark
✅ 47 untouched benchmarks
⏩ 3 skipped benchmarks1

Performance Changes

Mode Benchmark BASE HEAD Efficiency
Simulation mangler[RadixUIAdoptionSection.jsx_keep_names] 20.2 µs 18.4 µs +10.09%

Comparing om/04-01-refactor_allocator_use_latest_bumpalo_as_base_for_bump_ (d8d2d4e) with main (fc82a7c)

Open in CodSpeed

Footnotes

  1. 3 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

@overlookmotel overlookmotel marked this pull request as ready for review April 2, 2026 13:35
Copilot AI review requested due to automatic review settings April 2, 2026 13:35
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Refreshes oxc_allocator’s in-repo Bump implementation to closely match the latest upstream bumpalo baseline (including restoring the MIN_ALIGN const parameter), with minimal local adjustments to keep CI green and preserve an auditable divergence trail.

Changes:

  • Replace the existing modified Bump implementation with a newer upstream-derived version and reintroduce Bump<const MIN_ALIGN: usize = 1>.
  • Gate public exposure of the bump module behind a testing feature so doctests can reference oxc_allocator::bump::Bump without making it part of the public API in normal builds.
  • Update allocation tracking snapshot outputs to reflect the allocator behavior changes.

Reviewed changes

Copilot reviewed 6 out of 7 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
tasks/track_memory_allocations/allocs_semantic.snap Updates semantic allocation snapshot values/format after allocator changes.
tasks/track_memory_allocations/allocs_minifier.snap Updates minifier allocation snapshot values/format after allocator changes.
crates/oxc_allocator/src/lib.rs Makes bump private by default; exposes it only under feature = "testing" for doctests/tests.
crates/oxc_allocator/src/bumpalo_alloc.rs Syncs allocator API module with upstream-derived version + lint attribute reshuffling.
crates/oxc_allocator/src/bump.rs Major upstream refresh of Bump, adds MIN_ALIGN const generic, alignment-aware fast path adjustments, and new try_* helpers.
crates/oxc_allocator/Cargo.toml Adds testing feature and self dev-dependency to enable doctests/tests with testing.
Cargo.lock Reflects the new self dev-dependency edge for oxc_allocator.

@overlookmotel overlookmotel merged commit caa2dce into main Apr 2, 2026
44 checks passed
@overlookmotel overlookmotel deleted the om/04-01-refactor_allocator_use_latest_bumpalo_as_base_for_bump_ branch April 2, 2026 14:54
@overlookmotel overlookmotel self-assigned this Apr 2, 2026
@overlookmotel overlookmotel added the A-allocator Area - Allocator label Apr 2, 2026
graphite-app bot pushed a commit that referenced this pull request Apr 2, 2026
…set` (#20964)

Partial revert of #18168.

#18168 made a small change to `get_current_chunk_footer_field_offset` (used by `Allocator::from_raw_parts`).

That change was correct in the context of the other changes made to `Bump` in that PR, but now that #20963 has rolled back most of those changes, this change needs to be reverted too as it's a potential perf regression with current version of `Bump`.
graphite-app bot pushed a commit that referenced this pull request Apr 2, 2026
Repeat of #18181.

#20963 wiped all changes to `bumpalo_alloc.rs`, going back to a fresh copy of `bumpalo`. Re-apply changes from #18181 which were lost in the process - marking error handling functions as `#[cold]`.

This PR differs from #18181 in one minor respect. `new_layout_err` is not marked `#[inline(never)]` in this PR. The function is a no-op (unconditionally returns a ZST), so we do want it inlined, but with the`#[cold]`attribute to hint to compiler that it's a cold path.
graphite-app bot pushed a commit that referenced this pull request Apr 2, 2026
Repeat of #18234.

#20963 wiped all changes to `bump.rs`, going back to a fresh copy of `bumpalo`.

Re-apply changes from #18234 which were lost in the process - increasing default arena chunk size to 16 KiB, and altering `try_with_min_align_and_capacity` to respect the requested capacity in `Bump::with_capacity`.

Add a comment to `try_with_min_align_and_capacity` explaining why that change is required.
overlookmotel added a commit that referenced this pull request Apr 2, 2026
#20963 had a test failing on 32-bit platforms. Fix it.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-allocator Area - Allocator C-cleanup Category - technical debt or refactoring. Solution not expected to change behavior

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants