-
Notifications
You must be signed in to change notification settings - Fork 5k
Return freed memory to the OS on a background thread instead of the JS thread #34181
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
+27
−6
Merged
Changes from all commits
Commits
Show all changes
14 commits
Select commit
Hold shift + click to select a range
34afcea
usockets: sweep mimalloc's heaps while the loop is parked, not before…
Jarred-Sumner c094bd9
usockets: hand the heaps over on every tick, and use mimalloc's header
Jarred-Sumner 38f5d78
deps: bump mimalloc for the interruptible collect phase
Jarred-Sumner 7a84c86
usockets: sweep inline only when there is no scavenger to hand off to
Jarred-Sumner c8abd3e
jsc: return the JS thread's mimalloc pages to the arena when the heap…
Jarred-Sumner a908c8a
Revert "jsc: return the JS thread's mimalloc pages to the arena when …
Jarred-Sumner edd3e53
Reapply "jsc: return the JS thread's mimalloc pages to the arena when…
Jarred-Sumner 3c8728e
jsc: collect the JS thread's theap when a collection shrinks the heap
Jarred-Sumner f745562
jsc: collect the JS thread's theap after a collection, when the loop …
Jarred-Sumner c015194
[autofix.ci] apply automated fixes
autofix-ci[bot] c0068c6
jsc: trim the theap-collect comments to the durable content
Jarred-Sumner 868c47b
jsc: drop the unused theap-collect counter
Jarred-Sumner 0d9ba34
Revert the GC-controller theap collect: measured, it does nothing
Jarred-Sumner d940463
deps: bump mimalloc to bun-dev3-v2 with the idle-theap handoff
Jarred-Sumner File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟡 Gating this block to
OS(WINDOWS)and deleting theif (nowNs == 0) nowNs = us_internal_monotonic_ns();fallback leaves stale artifacts: the comment's "(0 = take one)" clause now describes code that no longer exists (Windows always passes a reading, and if it passed 0 the sweep would be skipped, not "taken"), and theus_internal_monotonic_nsextern + its 3-line comment at the top of the file is now unreferenced (as ismi_on_thread_idleon non-Windows, and thenowNsparameter itself). Per the landing rules, dead code should be deleted in the same PR that makes it dead — drop the parenthetical, remove the#if !OS(WINDOWS)extern block, and gate themi_on_thread_idleextern toOS(WINDOWS).Extended reasoning...
What's stale
This PR made two changes to the
#if USE(MIMALLOC)block inBun__JSC_onBeforeWait:#if USE(MIMALLOC)to#if USE(MIMALLOC) && OS(WINDOWS).Two pieces of surrounding text/code described that fallback and were not updated.
(a) The "(0 = take one)" comment clause
The comment above the rate-limit check still reads:
"0 = take one" was accurate before this PR: on POSIX,
nowNs == 0triggerednowNs = us_internal_monotonic_ns(). That line is now deleted, and the block only compiles on Windows — where the fallback never existed in the first place (the extern comment at the top of the file explicitly says "Windows always passes a reading, so it needs no fallback", andlibuv.c:226confirms it:Bun__JSC_onBeforeWait(loop->data.jsc_vm, (uint64_t) uv_now(loop->uv_loop) * 1000000ULL)).Step-by-step: if Windows ever did pass
nowNs = 0, the check would be0 >= lastIdleSweepNs + 100000000. WithlastIdleSweepNsstarting at 0, that's0 >= 100000000→ false → sweep skipped. It would not "take one". The parenthetical now describes behavior that exists nowhere in the codebase. The PR touched this exact comment (added the "Windows only:" paragraph right below it), so this isn't pre-existing drift — the stale clause was left in a block the author edited.(b) Dead extern declarations at the top of the file
us_internal_monotonic_nsis guarded#if !OS(WINDOWS). Its only call site in this TU was the deleted fallback. The declaration and its 3-line explanatory comment are now completely unreferenced — the symbol is never mentioned again in this file.mi_on_thread_idleis declared under#if USE(MIMALLOC)(all platforms) but is now only called under#if USE(MIMALLOC) && OS(WINDOWS), so on Linux/macOS it's an unused extern declaration.nowNsfunction parameter is now unread on non-Windows builds.Impact
None at runtime. Unused extern declarations emit no code and create no link dependency; a stale comment doesn't execute. This is purely about the repo's landing rule (CLAUDE.md → "Delete dead code in the same PR that makes it dead (required scope)" and "Comments carry only durable non-obvious content") — the PR itself made these dead, so cleanup belongs in the same change.
Fix
#if !OS(WINDOWS)extern block forus_internal_monotonic_nsentirely (declaration + 3-line comment).mi_on_thread_idleextern guard to#if USE(MIMALLOC) && OS(WINDOWS)to match its only remaining call site.(void)nowNs;on non-Windows if the build treats unused parameters as errors here.