Skip to content

refactor(allocator): add Arena::grow_fixed_size_chunk method#22123

Merged
graphite-app[bot] merged 1 commit into
mainfrom
om/05-03-refactor_allocator_add_arena_grow_fixed_size_chunk_method
May 14, 2026
Merged

refactor(allocator): add Arena::grow_fixed_size_chunk method#22123
graphite-app[bot] merged 1 commit into
mainfrom
om/05-03-refactor_allocator_add_arena_grow_fixed_size_chunk_method

Conversation

@overlookmotel

@overlookmotel overlookmotel commented May 5, 2026

Copy link
Copy Markdown
Member

Add a private method Arena::grow_fixed_size_chunk which attempts to grow a fixed-size Arena chunk in place.

Similar to #22122, there's multiple implementations for different platforms, but all are currently the same - all return None to indicate "could not grow". This is preparatory work for #22124, which adds a Windows-specific implementation.

overlookmotel commented May 5, 2026

Copy link
Copy Markdown
Member Author

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

codspeed-hq Bot commented May 5, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

✅ 48 untouched benchmarks
⏩ 3 skipped benchmarks1


Comparing om/05-03-refactor_allocator_add_arena_grow_fixed_size_chunk_method (e216a84) with main (6d42395)2

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.

  2. No successful run was found on main (e216a84) during the generation of this report, so 6d42395 was used instead as the comparison base. There might be some changes unrelated to this pull request in this report.

@overlookmotel overlookmotel self-assigned this May 5, 2026
@overlookmotel overlookmotel marked this pull request as ready for review May 5, 2026 01:28
Copilot AI review requested due to automatic review settings May 5, 2026 01:28

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Adds a new internal hook for fixed-size arenas to attempt in-place growth of the current chunk, and wires that hook into the allocation slow path. This is preparatory scaffolding for a future Windows-specific implementation.

Changes:

  • Introduce Arena::grow_fixed_size_chunk as a per-platform implementation stub (currently always returns None).
  • Call grow_fixed_size_chunk from try_alloc_layout_slow_impl when allocating from a fixed-size chunk, before failing the allocation.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

File Description
crates/oxc_allocator/src/arena/fixed_size/windows.rs Adds Windows stub for grow_fixed_size_chunk (currently no-op).
crates/oxc_allocator/src/arena/fixed_size/macos.rs Adds macOS stub for grow_fixed_size_chunk (currently no-op).
crates/oxc_allocator/src/arena/fixed_size/linux.rs Adds non-macOS/non-Windows stub for grow_fixed_size_chunk (currently no-op).
crates/oxc_allocator/src/arena/alloc_impl.rs Invokes grow_fixed_size_chunk in fixed-size allocation slow path before failing.

Comment thread crates/oxc_allocator/src/arena/fixed_size/linux.rs Outdated
Comment thread crates/oxc_allocator/src/arena/fixed_size/unix.rs
Comment thread crates/oxc_allocator/src/arena/fixed_size/windows.rs Outdated
@overlookmotel overlookmotel force-pushed the om/05-03-refactor_allocator_add_arena_grow_fixed_size_chunk_method branch from 91be1c5 to 2098199 Compare May 5, 2026 02:25
@overlookmotel overlookmotel force-pushed the om/05-03-refactor_allocator_per-platform_implementation_of_freeing_fixed-size_chunks branch from 8ad4c56 to a654d30 Compare May 5, 2026 02:25
@overlookmotel overlookmotel added C-cleanup Category - technical debt or refactoring. Solution not expected to change behavior A-allocator Area - Allocator labels May 5, 2026
@overlookmotel overlookmotel force-pushed the om/05-03-refactor_allocator_add_arena_grow_fixed_size_chunk_method branch from 2098199 to 01d8cdd Compare May 12, 2026 22:57
@overlookmotel overlookmotel force-pushed the om/05-03-refactor_allocator_per-platform_implementation_of_freeing_fixed-size_chunks branch 2 times, most recently from b6c9c93 to d5b9c75 Compare May 13, 2026 21:07
@overlookmotel overlookmotel force-pushed the om/05-03-refactor_allocator_add_arena_grow_fixed_size_chunk_method branch from 01d8cdd to cdc43b7 Compare May 13, 2026 21:07
@overlookmotel overlookmotel force-pushed the om/05-03-refactor_allocator_add_arena_grow_fixed_size_chunk_method branch from cdc43b7 to 9e53a66 Compare May 13, 2026 23:38
@overlookmotel overlookmotel force-pushed the om/05-03-refactor_allocator_per-platform_implementation_of_freeing_fixed-size_chunks branch from d5b9c75 to f4cbe3d Compare May 14, 2026 18:15
@overlookmotel overlookmotel force-pushed the om/05-03-refactor_allocator_add_arena_grow_fixed_size_chunk_method branch from 9e53a66 to c08e350 Compare May 14, 2026 18:15
@overlookmotel

Copy link
Copy Markdown
Member Author

Performance Changes

Mode Benchmark BASE HEAD Efficiency
❌ Simulation parser[RadixUIAdoptionSection.jsx] 80.9 µs 84.9 µs -4.68%
❌ Simulation semantic[cal.com.tsx] 21.1 ms 21.9 ms -3.41%

I need to investigate if this is spurious or not.

@overlookmotel overlookmotel marked this pull request as draft May 14, 2026 18:27
@overlookmotel overlookmotel force-pushed the om/05-03-refactor_allocator_add_arena_grow_fixed_size_chunk_method branch from c08e350 to dd5ead0 Compare May 14, 2026 18:28
@overlookmotel

Copy link
Copy Markdown
Member Author

Performance Changes

Mode Benchmark BASE HEAD Efficiency
❌ Simulation parser[RadixUIAdoptionSection.jsx] 80.9 µs 84.9 µs -4.68%
❌ Simulation semantic[cal.com.tsx] 21.1 ms 21.9 ms -3.41%

I need to investigate if this is spurious or not.

Pushed again, and the "regression" entirely disappeared. I think previous result was just a freak. Codspeed noted that the CI run with the poor perf was run on a machine with a different CPU - assume that was the cause.

@overlookmotel overlookmotel marked this pull request as ready for review May 14, 2026 20:34
@graphite-app

graphite-app Bot commented May 14, 2026

Copy link
Copy Markdown
Contributor

Merge activity

Add a private method `Arena::grow_fixed_size_chunk` which attempts to grow a fixed-size `Arena` chunk in place.

Similar to #22122, there's multiple implementations for different platforms, but all are currently the same - all return `None` to indicate "could not grow". This is preparatory work for #22124, which adds a Windows-specific implementation.
@graphite-app graphite-app Bot force-pushed the om/05-03-refactor_allocator_per-platform_implementation_of_freeing_fixed-size_chunks branch from f4cbe3d to 5086ddc Compare May 14, 2026 22:27
@graphite-app graphite-app Bot force-pushed the om/05-03-refactor_allocator_add_arena_grow_fixed_size_chunk_method branch from dd5ead0 to e216a84 Compare May 14, 2026 22:28
graphite-app Bot pushed a commit that referenced this pull request May 14, 2026
…22124)

Fixes #19395 - OOM errors in Oxlint JS plugins on Windows.

### The problem

Previously, on all platforms each fixed-size (raw transfer-enabled) allocator obtained a large chunk of memory from the `System` allocator (6 GiB in the case of Windows). This is fine on MacOS and most Linux distros, as they use overcommit allocation accounting, so these allocations consume only virtual memory (address space), not actual physical memory. The available address space is enormous (~128 TiB), so it's hard to exhaust it. Physical memory is only consumed when pages of the allocation are actually touched, which the vast majority never are.

Windows behaves differently. Allocations obtained from `System` allocator immediately count towards the memory limit. So, on memory-constrained machines, allocating 6 GiB can immediately fail - OOM just from linting the first file.

### This PR

[#22088](#22088), #22122, and #22123 introduced platform-specific code for handling allocation and deallocation of fixed-size `Arena`s, which are used in raw transfer/Oxlint JS plugins.

This PR switches to a different allocation method on Windows for fixed-size `Arena`s.

It switches to using [VirtualAlloc](https://learn.microsoft.com/en-us/windows/win32/api/memoryapi/nf-memoryapi-virtualalloc). This Windows-only API allows reserving address space and committing pages of that reservation as separate operations (similar to `mmap`).

It works like this:

- Fixed-size allocators now still reserve 6 GiB, but that only consumes address space, and does not count towards max memory limit.
- Of that 6 GiB reservation, a 2 GiB "container", aligned on a 4 GiB boundary, will be used as the active part of the memory for storing the AST and other data.
- Initially only the last 16 KiB of the container is committed (made into usable memory, with physical pages backing it).
- If AST does not fit in the committed region, the region grows downwards, committing further pages to accomodate allocation requests, up to the maximum of 2 GiB.

Essentially, this replicates something similar to Linux-style allocation, but on Windows. The only difference is that on Windows it's necessary to explicitly commit pages before touching them, whereas Linux/MacOS commit on demand automatically.

### Implementation details and testing

This PR uses the battle-tested [windows-sys](https://crates.io/crates/windows-sys) crate for `VirtualAlloc` and `VirtualFree`. The allocation implementation is written from scratch, but while I was researching to validate that I'd not missed any edge cases, I discovered that it's almost identical to [wasmtime's implementation](https://github.com/bytecodealliance/wasmtime/blob/386a3280dee61f5c4120ce7cde621c1039e383d5/crates/wasmtime/src/runtime/vm/sys/windows/mmap.rs) which uses the same APIs. Comments in the code go over in some detail considerations of Rust's memory model and pointer provenance. As far as I can see, the implementation is fairly watertight.

There are tests for the essential functionality and edge cases, and #22121 made these tests run under Miri on Windows.

However, I don't have access to a Windows machine to fully test this by running Oxlint on a massive repo. I am hoping members of the community who do can put this through its paces a bit before we merge.

### What's missing

In enormous projects, when linting with both JS plugins and `import` plugin, it's possible to end up with a huge number of ASTs in memory simultaneously. With each AST consuming 6 GiB of address space, it's possible to exhaust the entire 128 TiB usable address space, and get OOM on _virtual_ memory.

At present we have a workaround to prevent too many fixed-size allocators existing concurrently, so avoiding OOM, but the workaround is a drag on performance. Future work will aim to address this.
Base automatically changed from om/05-03-refactor_allocator_per-platform_implementation_of_freeing_fixed-size_chunks to main May 14, 2026 22:33
@graphite-app graphite-app Bot merged commit e216a84 into main May 14, 2026
40 checks passed
@graphite-app graphite-app Bot deleted the om/05-03-refactor_allocator_add_arena_grow_fixed_size_chunk_method branch May 14, 2026 22:34
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