Skip to content

Fix mimalloc arena alignment assertion for structure heap #143

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

Open
dylan-conway wants to merge 9 commits into
base: main
Choose a base branch
from
Open

Fix mimalloc arena alignment assertion for structure heap #143

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
fda7c35
fix alignment with MI_ARENA_SLICE_SIZE
dylan-conway Jan 23, 2026
ab2470a
update
dylan-conway Jan 23, 2026
93ccaf0
Merge branch 'main' into dylan/fix-mimalloc-assertion
dylan-conway Jan 23, 2026
309346a
enable mimalloc on all platforms
dylan-conway Jan 23, 2026
2da75ad
use oven-sh/mimalloc
dylan-conway Jan 23, 2026
631217f
fix build and auto build preview
dylan-conway Jan 23, 2026
e8d0a95
update
dylan-conway Jan 23, 2026
1e0c749
update
dylan-conway Jan 23, 2026
b31f5b3
update
dylan-conway Jan 24, 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
186 changes: 71 additions & 115 deletions .github/workflows/build-preview.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
name: Preview Build

on:
issue_comment:
types: [created]
pull_request:
types: [opened, synchronize, reopened]
workflow_dispatch:
inputs:
pr_number:
Expand All @@ -13,101 +13,62 @@ on:
permissions:
contents: write
pull-requests: write
issues: write

concurrency:
group: preview-build-pr-${{ github.event.issue.number || inputs.pr_number }}
group: preview-build-pr-${{ github.event.pull_request.number || inputs.pr_number }}
cancel-in-progress: true

jobs:
trigger:
name: Trigger Preview Build
runs-on: ubuntu-latest
if: |
github.event_name == 'workflow_dispatch' ||
(github.event.issue.pull_request && contains(github.event.comment.body, '/build-preview'))
outputs:
sha: ${{ steps.pr.outputs.sha }}
short_sha: ${{ steps.pr.outputs.short_sha }}
release_tag: ${{ steps.pr.outputs.release_tag }}
pr_number: ${{ steps.pr_number.outputs.number }}
pr_number: ${{ steps.pr.outputs.pr_number }}

steps:
- name: Set PR number
id: pr_number
run: |
if [ "${{ github.event_name }}" = "workflow_dispatch" ]; then
echo "number=${{ inputs.pr_number }}" >> $GITHUB_OUTPUT
else
echo "number=${{ github.event.issue.number }}" >> $GITHUB_OUTPUT
fi

- name: Check permissions
if: github.event_name == 'issue_comment'
if: github.event_name == 'pull_request'
uses: actions/github-script@v7
with:
script: |
const { data: permission } = await github.rest.repos.getCollaboratorPermissionLevel({
owner: context.repo.owner,
repo: context.repo.repo,
username: context.payload.comment.user.login
username: context.actor
});

if (!['admin', 'write'].includes(permission.permission)) {
await github.rest.reactions.createForIssueComment({
owner: context.repo.owner,
repo: context.repo.repo,
comment_id: context.payload.comment.id,
content: '-1'
});
core.setFailed('❌ Only collaborators with write access can trigger preview builds');
core.setFailed(`${context.actor} does not have write access. Preview builds must be triggered manually via workflow_dispatch.`);
}

- name: Get PR information
id: pr
uses: actions/github-script@v7
with:
script: |
const { data: pr } = await github.rest.pulls.get({
owner: context.repo.owner,
repo: context.repo.repo,
pull_number: ${{ steps.pr_number.outputs.number }}
});

const shortSha = pr.head.sha.substring(0, 8);
const releaseTag = `autobuild-preview-pr-${{ steps.pr_number.outputs.number }}-${shortSha}`;
let sha, prNumber;

core.setOutput('sha', pr.head.sha);
core.setOutput('short_sha', shortSha);
core.setOutput('release_tag', releaseTag);

- name: React to comment
if: 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: 'eyes'
});
if (context.eventName === 'pull_request') {
sha = context.payload.pull_request.head.sha;
prNumber = context.payload.pull_request.number;
} else {
const { data: pr } = await github.rest.pulls.get({
owner: context.repo.owner,
repo: context.repo.repo,
pull_number: ${{ inputs.pr_number || 0 }}
});
sha = pr.head.sha;
prNumber = ${{ inputs.pr_number || 0 }};
}

- name: Post starting message
if: github.event_name == 'issue_comment'
uses: actions/github-script@v7
with:
script: |
const body = "🔄 Preview build started\n\n" +
"**Release tag**: `${{ steps.pr.outputs.release_tag }}`\n" +
"**Workflow**: [View progress](" + context.serverUrl + "/" + context.repo.owner + "/" + context.repo.repo + "/actions/runs/" + context.runId + ")";
const shortSha = sha.substring(0, 8);
const releaseTag = `autobuild-preview-pr-${prNumber}-${shortSha}`;

await github.rest.issues.createComment({
owner: context.repo.owner,
repo: context.repo.repo,
issue_number: ${{ steps.pr_number.outputs.number }},
body: body
});
core.setOutput('sha', sha);
core.setOutput('release_tag', releaseTag);
core.setOutput('pr_number', prNumber);

build:
name: Build
Expand All @@ -122,68 +83,63 @@ jobs:
llvm_version: '19'

notify:
name: Notify Completion
name: Update PR Comment
needs: [trigger, build]
runs-on: ubuntu-latest
if: always()
if: needs.build.result == 'success'
permissions:
issues: write
pull-requests: write
steps:
- name: Post success comment
if: needs.build.result == 'success'
- name: Update preview build comment
uses: actions/github-script@v7
with:
script: |
const marker = '<!-- preview-build-comment -->';
const prNumber = ${{ needs.trigger.outputs.pr_number }};
const releaseTag = '${{ needs.trigger.outputs.release_tag }}';
const body = "✅ Preview build completed\n\n" +
"**Release**: [" + releaseTag + "](https://github.com/" + context.repo.owner + "/" + context.repo.repo + "/releases/tag/" + releaseTag + ")";
const sha = '${{ needs.trigger.outputs.sha }}';
const shortSha = sha.substring(0, 8);
const releaseUrl = `https://github.com/${context.repo.owner}/${context.repo.repo}/releases/tag/${releaseTag}`;
const newEntry = `| \`${shortSha}\` | [\`${releaseTag}\`](${releaseUrl}) | ${new Date().toISOString().replace('T', ' ').substring(0, 19)} UTC |`;

await github.rest.issues.createComment({
// 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));
Comment on lines +105 to +112
Copy link

@coderabbitai coderabbitai bot Jan 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 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.


// 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');
Comment on lines +114 to +129
Copy link

@coderabbitai coderabbitai bot Jan 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 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.


if (existing) {
await github.rest.issues.updateComment({
owner: context.repo.owner,
repo: context.repo.repo,
comment_id: existing.id,
body: body
});
} else {
await github.rest.issues.createComment({
owner: context.repo.owner,
repo: context.repo.repo,
issue_number: prNumber,
body: body
});
}
1 change: 1 addition & 0 deletions Dockerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -203,6 +203,7 @@ RUN --mount=type=tmpfs,target=/webkitbuild \
-DUSE_THIN_ARCHIVES=OFF \
-DUSE_BUN_JSC_ADDITIONS=ON \
-DUSE_BUN_EVENT_LOOP=ON \
-DUSE_MIMALLOC=ON \
-DENABLE_FTL_JIT=ON \
-DCMAKE_EXPORT_COMPILE_COMMANDS=ON \
-DALLOW_LINE_AND_COLUMN_NUMBER_IN_BUILTINS=ON \
Expand Down
1 change: 1 addition & 0 deletions Dockerfile.musl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,7 @@ RUN --mount=type=tmpfs,target=/webkitbuild \
-DUSE_THIN_ARCHIVES=OFF \
-DUSE_BUN_JSC_ADDITIONS=ON \
-DUSE_BUN_EVENT_LOOP=ON \
-DUSE_MIMALLOC=ON \
-DENABLE_FTL_JIT=ON \
-DCMAKE_EXPORT_COMPILE_COMMANDS=ON \
-DALLOW_LINE_AND_COLUMN_NUMBER_IN_BUILTINS=ON \
Expand Down
6 changes: 4 additions & 2 deletions Source/JavaScriptCore/heap/StructureAlignedMemoryAllocator.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ WTF_ALLOW_UNSAFE_BUFFER_USAGE_BEGIN
#include <bmalloc/pas_thread_local_cache.h>
#elif USE(MIMALLOC)
#include <bmalloc/mimalloc.h>
#include <mimalloc/types.h>
#endif
WTF_ALLOW_UNSAFE_BUFFER_USAGE_END
#endif
Expand Down Expand Up @@ -139,8 +140,9 @@ class StructureMemoryManager {
}
m_usedBlocks.set(0);
#elif USE(MIMALLOC)
void* memory = reinterpret_cast<void*>(reinterpret_cast<uintptr_t>(g_jscConfig.startOfStructureHeap) + MarkedBlock::blockSize);
size_t size = g_jscConfig.sizeOfStructureHeap - MarkedBlock::blockSize;
constexpr size_t offset = std::max(MarkedBlock::blockSize, static_cast<size_t>(MI_ARENA_SLICE_SIZE));
void* memory = reinterpret_cast<void*>(reinterpret_cast<uintptr_t>(g_jscConfig.startOfStructureHeap) + offset);
size_t size = g_jscConfig.sizeOfStructureHeap - offset;
RELEASE_ASSERT(mi_manage_os_memory_ex(memory, size, false, false, false, -1, true, &structureArena));
structureHeap = mi_heap_new_in_arena(structureArena);
#else
Expand Down
4 changes: 4 additions & 0 deletions Source/JavaScriptCore/shell/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,10 @@ if (WIN32)
list(APPEND jsc_LIBRARIES Winmm)
endif ()

if (USE_MIMALLOC AND USE_BUN_JSC_ADDITIONS)
list(APPEND jsc_LIBRARIES $<TARGET_OBJECTS:mimalloc-obj>)
endif ()

if (ENABLE_FUZZILLI)
list(APPEND jsc_SOURCES ../fuzzilli/Fuzzilli.cpp)
endif ()
Expand Down
5 changes: 4 additions & 1 deletion Source/bmalloc/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -649,7 +649,7 @@ if (ATOMICS_REQUIRE_LIBATOMIC)
list(APPEND bmalloc_LIBRARIES atomic)
endif ()

if (USE_MIMALLOC)
if (USE_MIMALLOC AND NOT USE_BUN_JSC_ADDITIONS)
list(APPEND bmalloc_LIBRARIES $<TARGET_OBJECTS:mimalloc-obj>)
endif ()

Expand Down Expand Up @@ -681,6 +681,9 @@ add_definitions(-DPAS_BMALLOC=1)

if (USE_MIMALLOC)
add_definitions(-DUSE_MIMALLOC=1)
list(APPEND bmalloc_INTERFACE_INCLUDE_DIRECTORIES
"${BMALLOC_DIR}/mimalloc/mimalloc/include"
)
endif ()

if (USE_SYSTEM_MALLOC)
Expand Down
10 changes: 2 additions & 8 deletions Source/bmalloc/bmalloc/VMAllocate.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -387,14 +387,8 @@ inline void vmZeroAndPurge(void* p, size_t vmSize, VMTag usage)
BUNUSED_PARAM(usage);

vmValidate(p, vmSize);
// DiscardVirtualMemory does not guarantee zeroing and fails on memory
// managed by libpas within reserved Gigacage regions. Use decommit+recommit
// which releases physical pages and provides demand-zeroed pages on next
// access, matching the behavior of mmap(MAP_FIXED|MAP_ANON) on Unix.
BOOL success = VirtualFree(p, vmSize, MEM_DECOMMIT);
RELEASE_BASSERT(success);
void* result = VirtualAlloc(p, vmSize, MEM_COMMIT, PAGE_READWRITE);
RELEASE_BASSERT(result == p);
DWORD result = DiscardVirtualMemory(p, vmSize);
RELEASE_BASSERT(result == ERROR_SUCCESS);
}

inline void vmDeallocatePhysicalPages(void* p, size_t vmSize)
Expand Down
4 changes: 4 additions & 0 deletions Source/bmalloc/mimalloc/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,10 @@ set(MI_BUILD_STATIC OFF CACHE BOOL "Build mimalloc static library" FORCE)
set(MI_BUILD_OBJECT ON CACHE BOOL "Build mimalloc object library" FORCE)
set(MI_BUILD_TESTS OFF CACHE BOOL "Build mimalloc tests" FORCE)

if (USE_BUN_JSC_ADDITIONS)
set(MI_USE_CXX ON CACHE BOOL "Use C++ compiler for mimalloc (avoids GCC stdatomic.h incompatibility with clang)" FORCE)
endif ()

WEBKIT_INCLUDE_CONFIG_FILES_IF_EXISTS()

add_subdirectory(mimalloc)
Expand Down
2 changes: 1 addition & 1 deletion build.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,7 @@ const getCommonFlags = (config: BuildConfig) => {
"-DUSE_BUN_JSC_ADDITIONS=ON",
"-DUSE_BUN_EVENT_LOOP=ON",
"-DENABLE_FTL_JIT=ON",
"-DUSE_MIMALLOC=ON",
"-G",
"Ninja",
];
Expand Down Expand Up @@ -149,7 +150,6 @@ const getCommonFlags = (config: BuildConfig) => {
flags.push(
"-DENABLE_REMOTE_INSPECTOR=ON",
"-DUSE_VISIBILITY_ATTRIBUTE=1",
"-DUSE_SYSTEM_MALLOC=ON",
`-DCMAKE_LINKER=${lldLink}`,
`-DICU_ROOT=${VCPKG_ROOT}`,
`-DICU_LIBRARY=${icuPaths.ICU_LIBRARY}`,
Expand Down
8 changes: 2 additions & 6 deletions mac-release.bash
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,12 +27,7 @@ if [ -n "$ENABLE_SANITIZERS" ]; then
export ENABLE_ASSERTS="ON";
fi

export DEFAULT_MALLOC_HEAP_BREAKDOWN=${DEFAULT_MALLOC_HEAP_BREAKDOWN:-"OFF"}
if [ "$CMAKE_BUILD_TYPE" == "Debug" ]; then
export DEFAULT_MALLOC_HEAP_BREAKDOWN="ON"
fi

ENABLE_MALLOC_HEAP_BREAKDOWN=${ENABLE_MALLOC_HEAP_BREAKDOWN:-$DEFAULT_MALLOC_HEAP_BREAKDOWN}
ENABLE_MALLOC_HEAP_BREAKDOWN=${ENABLE_MALLOC_HEAP_BREAKDOWN:-"OFF"}

echo "Building for mac: [ $ENABLE_MALLOC_HEAP_BREAKDOWN ] [ $ENABLE_SANITIZERS ]"

Expand All @@ -54,6 +49,7 @@ cmake \
-DENABLE_MALLOC_HEAP_BREAKDOWN=$ENABLE_MALLOC_HEAP_BREAKDOWN \
-DCMAKE_EXPORT_COMPILE_COMMANDS=ON \
-DUSE_BUN_JSC_ADDITIONS=ON \
-DUSE_MIMALLOC=ON \
Comment on lines 50 to +52
Copy link

@coderabbitai coderabbitai bot Jan 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 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.

-DENABLE_ASSERTS="$ENABLE_ASSERTS" \
-DUSE_BUN_EVENT_LOOP=ON \
-DCMAKE_AR="$AR" \
Expand Down
3 changes: 1 addition & 2 deletions windows-release.ps1
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -380,8 +380,7 @@ cmake -S . -B $WebKitBuild `
-DUSE_BUN_JSC_ADDITIONS=ON `
-DUSE_BUN_EVENT_LOOP=ON `
-DENABLE_BUN_SKIP_FAILING_ASSERTIONS=ON `
-DUSE_MIMALLOC=OFF `
-DUSE_LIBPAS=ON `
-DUSE_MIMALLOC=ON `
"-DICU_ROOT=${ICU_STATIC_ROOT}" `
"-DICU_LIBRARY=${ICU_STATIC_LIBRARY}" `
"-DICU_INCLUDE_DIR=${ICU_STATIC_INCLUDE_DIR}" `
Expand Down
Loading