purge-holes: keep sweep state off __thread statics (per-page guard, counters on the tld) - #18
Closed
dylan-conway wants to merge 2 commits into
Closed
purge-holes: keep sweep state off __thread statics (per-page guard, counters on the tld)#18dylan-conway wants to merge 2 commits into
dylan-conway wants to merge 2 commits into
Conversation
…ad statics `mi_purging_holes` (the sweep re-entrancy guard) is read from `mi_page_free_collect_ex`, i.e. on the allocation slow path, and the three `mi_holes_sweep_*` counters/flags sat next to it as `mi_decl_thread` statics. On targets with emulated TLS (Android before API 29, which is what Bun ships) the first access to a `__thread` variable calls `malloc` to allocate the thread's TLS block. A fresh thread whose first slow-path allocation was also its first touch of `mi_purging_holes` therefore went malloc -> mi_page_free_collect_ex -> __emutls_get_address -> malloc -> ... until it ran out of stack (seen as silent SIGSEGVs on JSC's Wasm compiler threads). Upstream already keeps compiler TLS off the allocation path on such platforms (MI_TLS_MODEL_PTHREADS on Android); follow that: the four values now live at the tail of `mi_tld_t` and are reached through `_mi_theap_default()`, the accessor the fast path itself uses. They are state of the thread running the sweep, so `_sweep_begin` writes the sweeper's tld while still pacing the sequence off the tld being swept.
Reaching the state through `_mi_theap_default()` initialized a theap on mimalloc's own scavenger thread the first time it swept a parked thread, which then tripped `mi_page_is_valid_init`'s ownership assertion while it validated that thread's pages. The scavenger is meant to stay theap-less, so instead of "state of the sweeping thread" make it "state of the tld being swept": the sweeper holds that tld exclusively (its own, or a parked one it claimed), every sweep entry point already has it in hand, and `_mi_page_purge_holes`/`_walk` now take it as a parameter. The allocation-path reader `_mi_page_purge_holes_in_progress` takes the page and consults the page owner's tld: a nested allocation on the sweeping thread can only be served from a page under the sweep when the sweeper is that page's owner, and abandoned pages (theap == NULL) are not allocated from at all. No thread-local access of any kind remains.
Collaborator
dylan-conway
changed the title
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
2 participants
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.
The hole-sweep re-entrancy guard
mi_purging_holesis read frommi_page_free_collect_ex— the allocation slow path — and it plus the threemi_holes_sweep_*values weremi_decl_threadstatics. On emulated-TLS targets (Android below API 29, which is what Bun currently ships) the first access to a__threadvariable callsmallocto allocate the thread's TLS block, so a fresh thread whose first slow-path allocation was also its first touch of that variable recursedmalloc → mi_page_free_collect_ex → __emutls_get_address → malloc → …until it exhausted its stack. In Bun this showed up as silent SIGSEGVs on JSC's Wasm (OMG) compiler threads on Android x64; the core shows exactly that cycle. Upstream keeps compiler TLS off the allocation path on such platforms (MI_TLS_MODEL_PTHREADSon Android); this removes it here too.MI_PARK_SWEEPING, or claimed the abandoned page)._mi_page_purge_holessets/clearspage->purging_holesaround the walk andmi_page_free_collect_exreads it directly — no thread-local, nopage->theapchase (which would be unsound: abandoned pages keep a non-NULL theap). The sweep's own pre-walk collects have nothing to serve and now use_mi_page_free_collect_no_unpurge, as heap inspection already did;_mi_page_purge_holes_begingoes away.holes_sweep_full→ tail ofmi_tld_t, as state of the tld being swept, passed explicitly (_mi_page_purge_holes(page, tld),_end(tld)); positional initializers unaffected, zero is the right initial value.History: v1 reached the state via
mi_theap_get_default(), which initialized a theap on mimalloc's own scavenger thread and trippedmi_page_is_valid_init's ownership assertion in Bun's debug build; v2 read it viapage->theap->tld, which review correctly flagged as unsound for abandoned pages.Verified: standalone clang Debug/
MI_DEBUG_FULLand Release builds; ctest andmimalloc-test-purge-holesresults identical to the base commit (same pre-existing failures). Bun Linux debug build (allocator asserts on) with this pin: multi-Worker allocation snippets,worker.test.ts,worker_threads.test.ts(122/122) run without allocator assertions. Android end-to-end (Wasm OMG stress tests, previously 100% segfault) is checked from the Bun PR's CI artifact.