Skip to content

Start mimalloc's scavenger thread explicitly at startup #34502

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.

Sign up for GitHub

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
dylan-conway merged 3 commits into from
Jul 18, 2026
Merged

Start mimalloc's scavenger thread explicitly at startup #34502

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f82b780
Start mimalloc's scavenger thread explicitly at startup
dylan-conway Jul 17, 2026
1cb1f7a
Reduce to a plain mimalloc bump
dylan-conway Jul 18, 2026
733bfac
Pin mimalloc to the merged commit
dylan-conway Jul 18, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion scripts/build/deps/mimalloc.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@

import type { Dependency, DirectBuild } from "../source.ts";

const MIMALLOC_COMMIT = "24211c6e7610ae7c4ec06040758ec90bd21a1c83";
const MIMALLOC_COMMIT = "475188181e4fc2816ec1caf3375a34fd9c0dd890";

Check warning on line 15 in scripts/build/deps/mimalloc.ts

View check run for this annotation

Claude / Claude Code Review

PR title/description are stale after 'Reduce to a plain mimalloc bump' commit 

The PR title and description still say this "binds the new `mi_scavenger_start()`, and calls it early in `main`", but commit 1cb1f7a8 ("Reduce to a plain mimalloc bump") removed both the binding and the startup call — the net diff is now just the mimalloc commit-hash bump and the matching `process.versions` expectation. Since the description becomes the permanent squash-commit message (per `.claude/docs/landing-prs.md`, PR process: "Re-sync title/description whenever review reworks the change"),

@claude claude Bot Jul 18, 2026

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.

🟡 The PR title and description still say this "binds the new mi_scavenger_start(), and calls it early in main", but commit 1cb1f7a ("Reduce to a plain mimalloc bump") removed both the binding and the startup call — the net diff is now just the mimalloc commit-hash bump and the matching process.versions expectation. Since the description becomes the permanent squash-commit message (per .claude/docs/landing-prs.md, PR process: "Re-sync title/description whenever review reworks the change"), the title/description should be updated to reflect that this is now a plain mimalloc pin bump before merging.

Extended reasoning...

What the finding is

The PR title is "Start mimalloc's scavenger thread explicitly at startup" and the description says it "bumps the mimalloc pin, binds the new mi_scavenger_start(), and calls it early in main — right after the signal setup". However, the second commit on this branch — 1cb1f7a "Reduce to a plain mimalloc bump" — removed both the mi_scavenger_start() FFI binding and the startup call, so the PR title and description no longer describe what the PR actually does.

Step-by-step proof

  1. Commit f82b780 ("Start mimalloc's scavenger thread explicitly at startup") added:
    • the mi_scavenger_start() extern declaration in src/mimalloc_sys/mimalloc.rs
    • a call to it in src/bun_bin/lib.rs after signal setup
    • the MIMALLOC_COMMIT bump in scripts/build/deps/mimalloc.ts
    • the process.versions.mimalloc expectation update in test/js/node/process/process.test.js
  2. Commit 1cb1f7a ("Reduce to a plain mimalloc bump") reverted the binding and the startup call. Its commit message says: "mimalloc keeps starting its scavenger during process init … so bun needs no startup call after all; drop the binding and the call."
  3. Net PR diff (git diff a2152850..HEAD --stat): 
     scripts/build/deps/mimalloc.ts       | 2 +-
 test/js/node/process/process.test.js | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)
    src/bun_bin/lib.rs and src/mimalloc_sys/mimalloc.rs are not touched.
  4. git grep mi_scavenger_start src/ returns no matches — the binding is gone from the tree.

So the delivered change is exactly: bump the pinned mimalloc commit from 24211c6… to 4751881… and update the corresponding test expectation. Nothing about scavenger startup is changed on the Bun side; per the 1cb1f7a commit message, mimalloc at the new pin still auto-starts its scavenger during process init.

Why this matters

The repo's own PR-process rules (.claude/docs/landing-prs.md, referenced from REVIEW.md) state:

The PR description is the permanent squash-commit message — keep it true. … Re-sync title/description whenever review reworks the change.

If this merges as-is, the squash commit on main will be titled "Start mimalloc's scavenger thread explicitly at startup" and its body will describe binding mi_scavenger_start() and calling it in main — none of which is present in the merged diff. Anyone later bisecting a scavenger-related regression, or reading git log to understand when/why explicit scavenger startup was added, would be misled: this commit does not add an explicit startup call. The description's second paragraph ("Without the call, purging quietly falls back to being allocation-driven…") is also now factually wrong for this PR, since there is no call and — per the 1cb1f7a message — none is needed.

What existing safeguards missed

CodeRabbit's "Title check" and "Description check" both passed, but they were run against commit f82b780 — before 1cb1f7a landed and dropped the change the title/description describe. Nothing re-validated the metadata after the rework commit.

Impact

No runtime, correctness, or build impact — the code that ships is fine. The only consequence is a misleading permanent squash-commit message in main's history. That's why this is filed as a nit, not a merge blocker.

Fix

Retitle to something like "Bump mimalloc to 4751881" (or similar) and rewrite the description to say what the net diff does: bump the pinned oven-sh/mimalloc commit and update the process.versions.mimalloc test expectation. If it's useful context, note that the new pin keeps the scavenger auto-starting during process init so no Bun-side change is needed.


export const mimalloc: Dependency = {
name: "mimalloc",
Expand Down
2 changes: 1 addition & 1 deletion test/js/node/process/process.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -333,7 +333,7 @@ it("process.versions", () => {
const expectedVersions = {
boringssl: "1a41b9025c2c0a37edd07ff10f6944f03e028522",
libarchive: "ded82291ab41d5e355831b96b0e1ff49e24d8939",
mimalloc: "24211c6e7610ae7c4ec06040758ec90bd21a1c83",
mimalloc: "475188181e4fc2816ec1caf3375a34fd9c0dd890",
picohttpparser: "066d2b1e9ab820703db0837a7255d92d30f0c9f5",
zlib: "12731092979c6d07f42da27da673a9f6c7b13586",
tinycc: "12882eee073cfe5c7621bcfadf679e1372d4537b",
Expand Down
Loading