Fix mimalloc arena alignment assertion for structure heap #143
Conversation
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
Member
Author
|
/build-preview |
|
🔄 Preview build started Release tag: |
WalkthroughPasses the full structure-heap region to mimalloc registration (adjusting pointer/size calculations and offset logic) and enables mimalloc across Windows, macOS, Docker (including musl) and shared build flags; several CMake lists updated to conditionally link/include mimalloc and workflow trigger/commenting logic revised. Changes
Possibly related PRs
🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@Source/JavaScriptCore/heap/StructureAlignedMemoryAllocator.cpp`:
- Around line 142-145: The code computes offset = max(MarkedBlock::blockSize,
MI_ARENA_SLICE_SIZE) and then subtracts it from g_jscConfig.sizeOfStructureHeap
to get size, which can underflow if sizeOfStructureHeap <= offset; update the
allocation path around mi_manage_os_memory_ex to validate and clamp/abort when
g_jscConfig.sizeOfStructureHeap <= offset (or compute size as 0 and fail early),
e.g. check g_jscConfig.sizeOfStructureHeap > offset before performing size =
g_jscConfig.sizeOfStructureHeap - offset and only call mi_manage_os_memory_ex
when size > 0 (or return an error/RELEASE_ASSERT with a clear message),
referencing offset, g_jscConfig.sizeOfStructureHeap, MarkedBlock::blockSize,
MI_ARENA_SLICE_SIZE, mi_manage_os_memory_ex, and structureArena so the guard is
placed immediately before the current subtraction and call.
In `@windows-release.ps1`:
- Around line 383-384: The CMake flag -DUSE_LIBPAS=OFF is unused and should be
removed from the Windows release script; locate the cmake invocation in
windows-release.ps1 where the flags include -DUSE_MIMALLOC=ON and
-DUSE_LIBPAS=OFF and delete the -DUSE_LIBPAS=OFF entry so only effective/defined
options (e.g., -DUSE_MIMALLOC=ON) remain.
|
This may have been caused by a newer build being triggered for the same PR. |
Member
Author
|
/build-preview |
|
🔄 Preview build started Release tag: |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@Source/JavaScriptCore/heap/StructureAlignedMemoryAllocator.cpp`:
- Around line 142-146: The comment and call to mi_manage_os_memory_ex assume
mimalloc won't allocate at offset 0, which is false and risks allocating at
StructureID(0); update the code to explicitly reserve offset 0 by either
adjusting the managed range passed to mi_manage_os_memory_ex (advance
g_jscConfig.startOfStructureHeap by MI_ARENA_SLICE_SIZE and reduce
g_jscConfig.sizeOfStructureHeap accordingly) or implement a guard that blocks
allocations at offset 0 in structureArena, and fix the comment to accurately
describe mimalloc's mi_arena_meta_zalloc behavior; ensure changes reference
g_jscConfig.startOfStructureHeap, g_jscConfig.sizeOfStructureHeap,
mi_manage_os_memory_ex, structureArena and mention
StructureID(0)/MI_ARENA_SLICE_SIZE.
Member
Author
|
/build-preview |
|
This may have been caused by a newer build being triggered for the same PR. |
|
🔄 Preview build started Release tag: |
|
✅ Preview build completed Release: autobuild-preview-pr-143-ab2470a3 |
Member
Author
|
/build-preview |
|
🔄 Preview build started Release tag: |
Member
Author
|
/build-preview |
|
This may have been caused by a newer build being triggered for the same PR. |
|
🔄 Preview build started Release tag: |
|
This may have been caused by a newer build being triggered for the same PR. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@mac-release.bash`:
- Around line 50-52: Remove the duplicate CMake flag by deleting one of the
repeated -DCMAKE_EXPORT_COMPILE_COMMANDS=ON entries in the mac-release.bash
invocation; keep the single occurrence and retain the other new flags
(-DUSE_BUN_JSC_ADDITIONS=ON and -DUSE_MIMALLOC=ON) so the CMake arguments list
is deduplicated and unchanged otherwise.
Comment on lines
50
to
+52
| -DCMAKE_EXPORT_COMPILE_COMMANDS=ON \ | ||
| -DUSE_BUN_JSC_ADDITIONS=ON \ | ||
| -DUSE_MIMALLOC=ON \ |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Minor: Duplicate CMake flag.
-DCMAKE_EXPORT_COMPILE_COMMANDS=ON is specified twice (lines 45 and 50). Consider removing one occurrence.
The -DUSE_MIMALLOC=ON addition looks correct and aligns with the PR objective.
Suggested cleanup
-DENABLE_MALLOC_HEAP_BREAKDOWN=$ENABLE_MALLOC_HEAP_BREAKDOWN \
- -DCMAKE_EXPORT_COMPILE_COMMANDS=ON \
-DUSE_BUN_JSC_ADDITIONS=ON \
-DUSE_MIMALLOC=ON \🤖 Prompt for AI Agents
In `@mac-release.bash` around lines 50 - 52, Remove the duplicate CMake flag by
deleting one of the repeated -DCMAKE_EXPORT_COMPILE_COMMANDS=ON entries in the
mac-release.bash invocation; keep the single occurrence and retain the other new
flags (-DUSE_BUN_JSC_ADDITIONS=ON and -DUSE_MIMALLOC=ON) so the CMake arguments
list is deduplicated and unchanged otherwise.
|
This may have been caused by a newer commit being pushed to the same PR. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @.github/workflows/build-preview.yml:
- Around line 114-129: The current extraction of previous table rows uses
lines.filter(l => l.startsWith('| `')), which is fragile; update the logic
around the existing variable where previousRows is computed (the lines,
tableLines, and previousRows variables) to more robustly detect table rows
(e.g., match lines that start with '|' and have three pipe-separated columns or
use a regex like /^\|\s*[^|]+\|\s*[^|]+\|\s*[^|]+\|/), ensure you preserve any
leading newline behavior (previousRows assignment), and keep the rest of the
body assembly (marker, '## Preview Builds', table header, newEntry +
previousRows) unchanged so existing consumers of body/newEntry continue to work.
- Around line 105-112: The listing of PR comments via
github.rest.issues.listComments with per_page: 100 can miss the marker comment
on very active PRs; update the logic (the call to
github.rest.issues.listComments and the subsequent comments.find(c =>
c.body.includes(marker)) usage) to paginate through all comment pages or
increase the page size and aggregate results before searching for marker,
ensuring the search for existing (the existing variable) checks the full set of
comments rather than only the first page.
Comment on lines
+105
to
+112
| // Find existing comment | ||
| const { data: comments } = await github.rest.issues.listComments({ | ||
| owner: context.repo.owner, | ||
| repo: context.repo.repo, | ||
| issue_number: ${{ needs.trigger.outputs.pr_number }}, | ||
| body: body | ||
| issue_number: prNumber, | ||
| per_page: 100 | ||
| }); | ||
| const existing = comments.find(c => c.body.includes(marker)); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Pagination may miss marker comment in highly active PRs.
With per_page: 100, the marker comment won't be found if there are more than 100 comments on the PR. This is an edge case but could result in duplicate preview build comments.
Consider paginating or using a higher limit if PRs with many comments are expected:
♻️ Optional: Add pagination or increase limit
// Find existing comment
- const { data: comments } = await github.rest.issues.listComments({
- owner: context.repo.owner,
- repo: context.repo.repo,
- issue_number: prNumber,
- per_page: 100
- });
- const existing = comments.find(c => c.body.includes(marker));
+ let existing = null;
+ for await (const { data: comments } of github.paginate.iterator(
+ github.rest.issues.listComments,
+ { owner: context.repo.owner, repo: context.repo.repo, issue_number: prNumber, per_page: 100 }
+ )) {
+ existing = comments.find(c => c.body.includes(marker));
+ if (existing) break;
+ }🤖 Prompt for AI Agents
In @.github/workflows/build-preview.yml around lines 105 - 112, The listing of
PR comments via github.rest.issues.listComments with per_page: 100 can miss the
marker comment on very active PRs; update the logic (the call to
github.rest.issues.listComments and the subsequent comments.find(c =>
c.body.includes(marker)) usage) to paginate through all comment pages or
increase the page size and aggregate results before searching for marker,
ensuring the search for existing (the existing variable) checks the full set of
comments rather than only the first page.
Comment on lines
+114
to
+129
| // Build table | ||
| let previousRows = ''; | ||
| if (existing) { | ||
| const lines = existing.body.split('\n'); | ||
| const tableLines = lines.filter(l => l.startsWith('| `')); | ||
| previousRows = tableLines.length > 0 ? '\n' + tableLines.join('\n') : ''; | ||
| } | ||
| - name: React to failure | ||
| if: needs.build.result == 'failure' && github.event_name == 'issue_comment' | ||
| uses: actions/github-script@v7 | ||
| with: | ||
| script: | | ||
| await github.rest.reactions.createForIssueComment({ | ||
| owner: context.repo.owner, | ||
| repo: context.repo.repo, | ||
| comment_id: context.payload.comment.id, | ||
| content: '-1' | ||
| }); | ||
| - name: Post failure comment | ||
| if: needs.build.result == 'failure' | ||
| uses: actions/github-script@v7 | ||
| with: | ||
| script: | | ||
| const body = "❌ Preview build failed\n\n" + | ||
| "Check the [workflow run](" + context.serverUrl + "/" + context.repo.owner + "/" + context.repo.repo + "/actions/runs/" + context.runId + ") for details."; | ||
| await github.rest.issues.createComment({ | ||
| owner: context.repo.owner, | ||
| repo: context.repo.repo, | ||
| issue_number: ${{ needs.trigger.outputs.pr_number }}, | ||
| body: body | ||
| }); | ||
| - name: Post cancellation comment | ||
| if: needs.build.result == 'cancelled' | ||
| uses: actions/github-script@v7 | ||
| with: | ||
| script: | | ||
| const body = "⚠️ Preview build was cancelled\n\n" + | ||
| "This may have been caused by a newer build being triggered for the same PR."; | ||
| await github.rest.issues.createComment({ | ||
| owner: context.repo.owner, | ||
| repo: context.repo.repo, | ||
| issue_number: ${{ needs.trigger.outputs.pr_number }}, | ||
| body: body | ||
| }); | ||
| const body = [ | ||
| marker, | ||
| '## Preview Builds', | ||
| '', | ||
| '| Commit | Release | Date |', | ||
| '|--------|---------|------|', | ||
| newEntry + previousRows | ||
| ].join('\n'); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Table row extraction relies on specific markdown format.
The filter l.startsWith('| \')` works for the current format but is fragile. If the table format changes (e.g., different cell content or spacing), previously appended rows may be lost.
This is acceptable for now but worth documenting or making more robust if the format evolves.
🤖 Prompt for AI Agents
In @.github/workflows/build-preview.yml around lines 114 - 129, The current
extraction of previous table rows uses lines.filter(l => l.startsWith('| `')),
which is fragile; update the logic around the existing variable where
previousRows is computed (the lines, tableLines, and previousRows variables) to
more robustly detect table rows (e.g., match lines that start with '|' and have
three pipe-separated columns or use a regex like
/^\|\s*[^|]+\|\s*[^|]+\|\s*[^|]+\|/), ensure you preserve any leading newline
behavior (previousRows assignment), and keep the rest of the body assembly
(marker, '## Preview Builds', table header, newEntry + previousRows) unchanged
so existing consumers of body/newEntry continue to work.
Preview Builds
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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
mi_manage_os_memory_ex2assertion failure when using mimalloc for the structure heap on WindowsMarkedBlock::blockSize(16KB), but mimalloc v3 requiresMI_ARENA_SLICE_SIZE(64KB) alignment for arena memorystd::max(MarkedBlock::blockSize, MI_ARENA_SLICE_SIZE)as the offset to ensure proper alignment🤖 Generated with Claude Code