Repository navigation
Add Linux mimalloc build matrix entries (amd64/arm64, release + LTO) #283
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
Merged
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
1d65722
Add Linux mimalloc build matrix entries (amd64/arm64, release + LTO)
robobun 22b4f7c
Dockerfile: unset C_INCLUDE_PATH for the WebKit build step
robobun f625cc5
StructureAlignedMemoryAllocator: fix mimalloc structure-heap arena un…
robobun 8e7b610
Enable USE_MIMALLOC/USE_EXTERNAL_MIMALLOC on all non-asan release tar…
robobun 4a486d1
Revert Heap::didFinishCollection mi_collect hook
robobun 342334e
bmalloc: add force param to scavengeThisThread; call it force=false a…
robobun 24c63e7
StructureAlignedMemoryAllocator: re-inherit the structure heap on mac…
robobun 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
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.
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.
🟡 The PR title/description are stale relative to commit 8e7b610: they say four new
-mimalloc-suffixed Linux entries are added and "existing matrix entries default to OFF so they are unchanged", but the final diff instead flipsUSE_MIMALLOC/USE_EXTERNAL_MIMALLOCtoONon the existing release labels across Linux glibc/musl, macOS, Windows, FreeBSD, and Android — no-mimalloclabels exist. Looks intentional per the commit title; would just update the title/description so the scope is clear.Extended reasoning...
What
The PR description says:
That accurately described the initial commit (1d65722), but the final commit on the branch — 8e7b610 "Enable USE_MIMALLOC/USE_EXTERNAL_MIMALLOC on all non-asan release targets" — replaced the additive approach with an in-place switch. In the final
.github/workflows/build-reusable.ymlthere are no-mimalloc-suffixed labels; insteadUSE_MIMALLOC: "ON"/USE_EXTERNAL_MIMALLOC: "ON"are added to the existing labels:bun-webkit-linux-{amd64,arm64}[-lto][-baseline], all six-muslrelease entries, all fourmacosrelease/LTO entries, all fivewindowsrelease/LTO/baseline entries, bothfreebsdrelease entries, and bothandroidrelease entries. Debug and ASAN entries are the only ones left on the default (libpas).Why the description no longer matches
-mimalloclabels named in the description do not appear anywhere in the workflow, thereleasejob's download/rename steps, or the release-asset lists.OFFso they are unchanged" is now false — every non-debug/non-asan existing label is changed to external-mimalloc.Dockerfile/release.sh, but the diff also touchesDockerfile.{musl,macos,windows,freebsd,android}and their wrapper scripts, and flips flags for those platforms in the matrix.Step-by-step
build-reusable.ymlformimallocin alabel:— no matches.bun-webkit-linux-amd64matrix entry (lines ~89-96): it now carriesUSE_MIMALLOC: "ON"andUSE_EXTERNAL_MIMALLOC: "ON".bun-webkit-macos-arm64,bun-webkit-windows-amd64,bun-webkit-freebsd-amd64,bun-webkit-linux-arm64-android, etc.Impact
This is a PR-metadata issue, not a code defect. The commit title (8e7b610) is explicit about the intent, artifacts publish under per-SHA
autobuild-{sha}tags so older Bun builds pinning a specific SHA are unaffected, and the timeline shows Bun's CI has already exercised these builds. So there's no downstream breakage risk from the description being stale — the concern is purely that a reviewer approving from the title/description would underestimate the scope (an allocator/ABI change to every existing release label across 6 platforms rather than 4 additive Linux entries).Fix
Update the PR title and description to reflect the final state, e.g. "Switch all non-debug/non-asan release artifacts to USE_MIMALLOC + USE_EXTERNAL_MIMALLOC", drop the list of
-mimalloclabels, and remove/revise the "existing matrix entries default to OFF so they are unchanged" sentence. No code change needed.