libpas(win): release TLC commit charge on scavenge; harden VirtualQuery#185
Open
Jarred-Sumner wants to merge 1 commit intomainfrom
Open
libpas(win): release TLC commit charge on scavenge; harden VirtualQuery#185Jarred-Sumner wants to merge 1 commit intomainfrom
Jarred-Sumner wants to merge 1 commit intomainfrom
Conversation
pas_thread_local_cache used the *_without_mprotect commit/decommit variants. On Windows, decommit_impl(do_mprotect=false) maps to DiscardVirtualMemory, which frees physical pages but does not release commit charge, while commit_impl always issues VirtualAlloc(MEM_COMMIT). The TLC tracks pages_committed and recommits before reuse, so it does not need pages to stay accessible across the cycle; switch to pas_page_malloc_commit/decommit so the scavenger actually returns commit charge via VirtualFree(MEM_DECOMMIT). POSIX production builds are unchanged (PAS_MPROTECT_DECOMMITTED is testing-only there). Also: - Wrap the three unchecked VirtualQuery calls in commit_impl/decommit_impl in PAS_ASSERT so failure surfaces instead of operating on stack garbage. - StructureAlignedMemoryAllocator: guard OSAllocator::commit on Windows/PS when bmalloc_try_allocate_auxiliary returns null (would otherwise reserve+commit and leak a fresh 16 KB region at a kernel-chosen address). Refs oven-sh/bun#29412
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughPlatform-specific memory management improvements across JavaScript Core and libpas allocators. Changes include conditional commit logic on allocation success, strengthened Windows memory query error handling, and migration to standard mprotect-aware memory functions in thread-local cache operations. Changes
🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
Preview Builds
|
Jarred-Sumner
added a commit
to oven-sh/bun
that referenced
this pull request
Apr 17, 2026
Tests oven-sh/WebKit#185 (libpas Windows TLC commit-charge release).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
pas_thread_local_cache.c: switch the three TLC commit/decommit sites from the_without_mprotectvariants topas_page_malloc_commit/decommit. On Windows the_without_mprotectdecommit usesDiscardVirtualMemory(does not release commit charge) while the matching commit alwaysVirtualAlloc(MEM_COMMIT)s, so per-thread TLC commit charge ratcheted to high-water mark for the life of each thread. The TLC trackspages_committedand recommits before reuse, so it doesn't need the keep-accessible semantics —VirtualFree(MEM_DECOMMIT)is correct here. POSIX production unchanged (PAS_MPROTECT_DECOMMITTEDisPAS_ENABLE_TESTING).pas_page_malloc.c: wrap the three uncheckedVirtualQuerycalls in the per-region fallback loops withPAS_ASSERT; on failurememInfowas uninitialized stack and the subsequent asserts/calls used garbage.StructureAlignedMemoryAllocator.cpp: null-guardOSAllocator::commit(result, ...)on the Windows/PlayStation libpas path.bmalloc_try_allocate_auxiliary_with_alignment_inlinecan return null;VirtualAlloc(NULL, 16KB, MEM_COMMIT, ...)would then allocate and leak a fresh region.Found while investigating oven-sh/bun#29412 (Windows commit-limit exhaustion: 11.26 GB commit / 1.10 GB RSS). The TLC asymmetry is a confirmed contributor; the other two are hardening in the exact code path that crashed.
Test plan
bun build.ts debug→ 3058/3058,bin/jscsmoke test OK)$.agentworkers — exercises all three TLC sites, no assertsVirtualFree(MEM_DECOMMIT)path is verified by inspection only on this host)