Linux ARM64 64KB page support (v2 — fix PAS_MAX_OBJECTS_PER_PAGE)#173
Open
dylan-conway wants to merge 3 commits intomainfrom
Open
Linux ARM64 64KB page support (v2 — fix PAS_MAX_OBJECTS_PER_PAGE)#173dylan-conway wants to merge 3 commits intomainfrom
dylan-conway wants to merge 3 commits intomainfrom
Conversation
Linux ARM64 kernels can be configured with 4KB, 16KB, or 64KB page sizes. Previously, the default CeilingOnPageSize for Linux ARM64 was 16KB, which caused an immediate RELEASE_ASSERT crash on systems with 64KB pages (e.g., RHEL aarch64, Oracle Linux, Ubuntu generic-64k). The existing USE(64KB_PAGE_BLOCK) workaround disables both JIT and bmalloc/libpas, which is a significant performance regression. This change raises CeilingOnPageSize to 64KB for all Linux ARM64 builds, and correspondingly updates libpas page/granule sizes to be at least 64KB on Linux ARM64. This allows Bun and other WebKit embedders to run on 64KB page kernels with full JIT and libpas support. Changes: - WTF/PageBlock.h: Set CeilingOnPageSize=64KB for Linux ARM64 - libpas/pas_internal_config.h: Raise small page (16KB->64KB), small bitfit page (16KB->64KB), medium page (128KB->256KB), and granule size (16KB->64KB) for Linux ARM64 - libpas/jit_heap_config.h: Raise JIT small page/granule (16KB->64KB) and JIT medium page/granule (128KB->256KB / 16KB->64KB) - libpas/pas_expendable_memory.h: Raise expendable memory page size (16KB->64KB) for Linux ARM64 - bmalloc/BPlatform.h: Add BUSE_PRECOMPUTED_CONSTANTS_VMPAGE64K These changes propagate correctly through: - ConfigSizeToProtect (via std::max(CeilingOnPageSize, 16*KB)) - OpcodeConfigSizeToProtect (same pattern) - MarkedBlock::blockSize (via std::max(16*KB, CeilingOnPageSize)) - All libpas heap configs that use PAS_*_DEFAULT_SIZE constants - mprotect/mmap calls that use runtime pageSize() Co-Authored-By: Claude <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughReworks page-size ceilings and related memory constants to make Linux ARM64 default to 64 KB page sizes in multiple bmalloc/libpas and WTF headers, adding guards and macros to expose 64 KB precomputed constants and adjust JIT/expendable memory and PAS shifts accordingly. Changes
Possibly related PRs
🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Source/bmalloc/libpas/src/libpas/pas_large_expendable_memory.h`:
- Around line 44-77: The new cap/alignment invariants need compile-time guards:
add _Static_asserts so PAS_LARGE_EXPENDABLE_MEMORY_ALIGNMENT is a power of two
(needed by pas_large_expendable_memory_header_for_object which rounds by that
alignment), ensure PAS_LARGE_EXPENDABLE_MEMORY_PAYLOAD_PAGES_CAPPED is exactly
page-aligned (i.e. PAS_LARGE_EXPENDABLE_MEMORY_PAYLOAD_SIZE_CAP is a multiple of
PAS_EXPENDABLE_MEMORY_PAGE_SIZE) to avoid truncation, and keep the existing
assertions that alignment > payload and alignment > total size; place these
asserts near the other _Static_asserts referencing
PAS_LARGE_EXPENDABLE_MEMORY_ALIGNMENT,
PAS_LARGE_EXPENDABLE_MEMORY_PAYLOAD_PAGES_CAPPED (or
PAS_LARGE_EXPENDABLE_MEMORY_PAYLOAD_SIZE_CAP) to cause compile-time failures if
future edits break these invariants.
In `@Source/WTF/wtf/PageBlock.h`:
- Around line 49-58: The change to set CeilingOnPageSize to 64 * KB for the
(USE(64KB_PAGE_BLOCK) || (OS(LINUX) && CPU(ARM64)) || CPU(PPC/ PPC64/ PPC64LE)
|| CPU(UNKNOWN)) branch is correct; approve and merge as-is, and optionally
add/ensure a Linux ARM64 build with 64 KiB pages in CI to catch regressions;
look for the CeilingOnPageSize constexpr in PageBlock.h to confirm the
conditional and value are unchanged before merging.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 30692a55-07a7-42bb-a784-aaad20575230
📒 Files selected for processing (6)
Source/WTF/wtf/PageBlock.hSource/bmalloc/bmalloc/BPlatform.hSource/bmalloc/libpas/src/libpas/jit_heap_config.hSource/bmalloc/libpas/src/libpas/pas_expendable_memory.hSource/bmalloc/libpas/src/libpas/pas_internal_config.hSource/bmalloc/libpas/src/libpas/pas_large_expendable_memory.h
Comment on lines
+44
to
+77
| /* Uncapped, payload and alignment scale as PAGE_SIZE^2 (512 MB with 64 KB pages). */ | ||
| #define PAS_LARGE_EXPENDABLE_MEMORY_PAYLOAD_SIZE_CAP (32llu * 1024llu * 1024llu) | ||
|
|
||
| #define PAS_LARGE_EXPENDABLE_MEMORY_PAYLOAD_PAGES_UNCAPPED \ | ||
| ((PAS_LARGE_EXPENDABLE_MEMORY_HEADER_SIZE - PAS_LARGE_EXPENDABLE_MEMORY_BASE_HEADER_SIZE) / sizeof(pas_expendable_memory_state)) | ||
| #define PAS_LARGE_EXPENDABLE_MEMORY_PAYLOAD_PAGES_CAPPED \ | ||
| (PAS_LARGE_EXPENDABLE_MEMORY_PAYLOAD_SIZE_CAP / PAS_EXPENDABLE_MEMORY_PAGE_SIZE) | ||
|
|
||
| #define PAS_LARGE_EXPENDABLE_MEMORY_PAYLOAD_PAGES \ | ||
| (PAS_LARGE_EXPENDABLE_MEMORY_PAYLOAD_PAGES_UNCAPPED < PAS_LARGE_EXPENDABLE_MEMORY_PAYLOAD_PAGES_CAPPED \ | ||
| ? PAS_LARGE_EXPENDABLE_MEMORY_PAYLOAD_PAGES_UNCAPPED \ | ||
| : PAS_LARGE_EXPENDABLE_MEMORY_PAYLOAD_PAGES_CAPPED) | ||
|
|
||
| #define PAS_LARGE_EXPENDABLE_MEMORY_PAYLOAD_SIZE \ | ||
| (((PAS_LARGE_EXPENDABLE_MEMORY_HEADER_SIZE - PAS_LARGE_EXPENDABLE_MEMORY_BASE_HEADER_SIZE) / \ | ||
| sizeof(pas_expendable_memory_state)) \ | ||
| * PAS_EXPENDABLE_MEMORY_PAGE_SIZE) | ||
| (PAS_LARGE_EXPENDABLE_MEMORY_PAYLOAD_PAGES * PAS_EXPENDABLE_MEMORY_PAGE_SIZE) | ||
|
|
||
| #define PAS_LARGE_EXPENDABLE_MEMORY_TOTAL_SIZE \ | ||
| (PAS_LARGE_EXPENDABLE_MEMORY_HEADER_SIZE + PAS_LARGE_EXPENDABLE_MEMORY_PAYLOAD_SIZE) | ||
|
|
||
| #define PAS_LARGE_EXPENDABLE_MEMORY_ALIGNMENT_UNCAPPED \ | ||
| (PAS_LARGE_EXPENDABLE_MEMORY_HEADER_SIZE / sizeof(pas_expendable_memory_state) * PAS_EXPENDABLE_MEMORY_PAGE_SIZE) | ||
| #define PAS_LARGE_EXPENDABLE_MEMORY_ALIGNMENT_CAPPED \ | ||
| (2llu * PAS_LARGE_EXPENDABLE_MEMORY_PAYLOAD_SIZE_CAP) | ||
|
|
||
| #define PAS_LARGE_EXPENDABLE_MEMORY_ALIGNMENT \ | ||
| (PAS_LARGE_EXPENDABLE_MEMORY_ALIGNMENT_UNCAPPED < PAS_LARGE_EXPENDABLE_MEMORY_ALIGNMENT_CAPPED \ | ||
| ? PAS_LARGE_EXPENDABLE_MEMORY_ALIGNMENT_UNCAPPED \ | ||
| : PAS_LARGE_EXPENDABLE_MEMORY_ALIGNMENT_CAPPED) | ||
|
|
||
| #if PAS_COMPILER(CLANG) | ||
| _Static_assert(PAS_LARGE_EXPENDABLE_MEMORY_ALIGNMENT > PAS_LARGE_EXPENDABLE_MEMORY_PAYLOAD_SIZE, | ||
| "Large expendable memory should be aligned more so than the payload size."); | ||
| "Large expendable memory should be aligned more so than the payload size."); | ||
| _Static_assert(PAS_LARGE_EXPENDABLE_MEMORY_ALIGNMENT > PAS_LARGE_EXPENDABLE_MEMORY_TOTAL_SIZE, | ||
| "Large expendable memory should be aligned more so than the total size."); | ||
| "Large expendable memory should be aligned more so than the total size."); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add compile-time guards for the new cap/alignment invariants.
pas_large_expendable_memory_header_for_object() rounds down by PAS_LARGE_EXPENDABLE_MEMORY_ALIGNMENT, so that value needs to stay a power of two. Also, PAS_LARGE_EXPENDABLE_MEMORY_PAYLOAD_PAGES_CAPPED currently truncates if the cap ever stops being page-aligned. A couple of _Static_asserts here would make future cap edits fail fast instead of changing the layout silently.
🧩 Suggested guardrails
`#if` PAS_COMPILER(CLANG)
+_Static_assert(!(PAS_LARGE_EXPENDABLE_MEMORY_PAYLOAD_SIZE_CAP % PAS_EXPENDABLE_MEMORY_PAGE_SIZE),
+ "Large expendable memory payload cap must stay page-aligned.");
+_Static_assert(!(PAS_LARGE_EXPENDABLE_MEMORY_ALIGNMENT & (PAS_LARGE_EXPENDABLE_MEMORY_ALIGNMENT - 1)),
+ "Large expendable memory alignment must stay a power of two.");
_Static_assert(PAS_LARGE_EXPENDABLE_MEMORY_ALIGNMENT > PAS_LARGE_EXPENDABLE_MEMORY_PAYLOAD_SIZE,
"Large expendable memory should be aligned more so than the payload size.");
_Static_assert(PAS_LARGE_EXPENDABLE_MEMORY_ALIGNMENT > PAS_LARGE_EXPENDABLE_MEMORY_TOTAL_SIZE,
"Large expendable memory should be aligned more so than the total size.");
`#endif`📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| /* Uncapped, payload and alignment scale as PAGE_SIZE^2 (512 MB with 64 KB pages). */ | |
| #define PAS_LARGE_EXPENDABLE_MEMORY_PAYLOAD_SIZE_CAP (32llu * 1024llu * 1024llu) | |
| #define PAS_LARGE_EXPENDABLE_MEMORY_PAYLOAD_PAGES_UNCAPPED \ | |
| ((PAS_LARGE_EXPENDABLE_MEMORY_HEADER_SIZE - PAS_LARGE_EXPENDABLE_MEMORY_BASE_HEADER_SIZE) / sizeof(pas_expendable_memory_state)) | |
| #define PAS_LARGE_EXPENDABLE_MEMORY_PAYLOAD_PAGES_CAPPED \ | |
| (PAS_LARGE_EXPENDABLE_MEMORY_PAYLOAD_SIZE_CAP / PAS_EXPENDABLE_MEMORY_PAGE_SIZE) | |
| #define PAS_LARGE_EXPENDABLE_MEMORY_PAYLOAD_PAGES \ | |
| (PAS_LARGE_EXPENDABLE_MEMORY_PAYLOAD_PAGES_UNCAPPED < PAS_LARGE_EXPENDABLE_MEMORY_PAYLOAD_PAGES_CAPPED \ | |
| ? PAS_LARGE_EXPENDABLE_MEMORY_PAYLOAD_PAGES_UNCAPPED \ | |
| : PAS_LARGE_EXPENDABLE_MEMORY_PAYLOAD_PAGES_CAPPED) | |
| #define PAS_LARGE_EXPENDABLE_MEMORY_PAYLOAD_SIZE \ | |
| (((PAS_LARGE_EXPENDABLE_MEMORY_HEADER_SIZE - PAS_LARGE_EXPENDABLE_MEMORY_BASE_HEADER_SIZE) / \ | |
| sizeof(pas_expendable_memory_state)) \ | |
| * PAS_EXPENDABLE_MEMORY_PAGE_SIZE) | |
| (PAS_LARGE_EXPENDABLE_MEMORY_PAYLOAD_PAGES * PAS_EXPENDABLE_MEMORY_PAGE_SIZE) | |
| #define PAS_LARGE_EXPENDABLE_MEMORY_TOTAL_SIZE \ | |
| (PAS_LARGE_EXPENDABLE_MEMORY_HEADER_SIZE + PAS_LARGE_EXPENDABLE_MEMORY_PAYLOAD_SIZE) | |
| #define PAS_LARGE_EXPENDABLE_MEMORY_ALIGNMENT_UNCAPPED \ | |
| (PAS_LARGE_EXPENDABLE_MEMORY_HEADER_SIZE / sizeof(pas_expendable_memory_state) * PAS_EXPENDABLE_MEMORY_PAGE_SIZE) | |
| #define PAS_LARGE_EXPENDABLE_MEMORY_ALIGNMENT_CAPPED \ | |
| (2llu * PAS_LARGE_EXPENDABLE_MEMORY_PAYLOAD_SIZE_CAP) | |
| #define PAS_LARGE_EXPENDABLE_MEMORY_ALIGNMENT \ | |
| (PAS_LARGE_EXPENDABLE_MEMORY_ALIGNMENT_UNCAPPED < PAS_LARGE_EXPENDABLE_MEMORY_ALIGNMENT_CAPPED \ | |
| ? PAS_LARGE_EXPENDABLE_MEMORY_ALIGNMENT_UNCAPPED \ | |
| : PAS_LARGE_EXPENDABLE_MEMORY_ALIGNMENT_CAPPED) | |
| #if PAS_COMPILER(CLANG) | |
| _Static_assert(PAS_LARGE_EXPENDABLE_MEMORY_ALIGNMENT > PAS_LARGE_EXPENDABLE_MEMORY_PAYLOAD_SIZE, | |
| "Large expendable memory should be aligned more so than the payload size."); | |
| "Large expendable memory should be aligned more so than the payload size."); | |
| _Static_assert(PAS_LARGE_EXPENDABLE_MEMORY_ALIGNMENT > PAS_LARGE_EXPENDABLE_MEMORY_TOTAL_SIZE, | |
| "Large expendable memory should be aligned more so than the total size."); | |
| "Large expendable memory should be aligned more so than the total size."); | |
| /* Uncapped, payload and alignment scale as PAGE_SIZE^2 (512 MB with 64 KB pages). */ | |
| `#define` PAS_LARGE_EXPENDABLE_MEMORY_PAYLOAD_SIZE_CAP (32llu * 1024llu * 1024llu) | |
| `#define` PAS_LARGE_EXPENDABLE_MEMORY_PAYLOAD_PAGES_UNCAPPED \ | |
| ((PAS_LARGE_EXPENDABLE_MEMORY_HEADER_SIZE - PAS_LARGE_EXPENDABLE_MEMORY_BASE_HEADER_SIZE) / sizeof(pas_expendable_memory_state)) | |
| `#define` PAS_LARGE_EXPENDABLE_MEMORY_PAYLOAD_PAGES_CAPPED \ | |
| (PAS_LARGE_EXPENDABLE_MEMORY_PAYLOAD_SIZE_CAP / PAS_EXPENDABLE_MEMORY_PAGE_SIZE) | |
| `#define` PAS_LARGE_EXPENDABLE_MEMORY_PAYLOAD_PAGES \ | |
| (PAS_LARGE_EXPENDABLE_MEMORY_PAYLOAD_PAGES_UNCAPPED < PAS_LARGE_EXPENDABLE_MEMORY_PAYLOAD_PAGES_CAPPED \ | |
| ? PAS_LARGE_EXPENDABLE_MEMORY_PAYLOAD_PAGES_UNCAPPED \ | |
| : PAS_LARGE_EXPENDABLE_MEMORY_PAYLOAD_PAGES_CAPPED) | |
| `#define` PAS_LARGE_EXPENDABLE_MEMORY_PAYLOAD_SIZE \ | |
| (PAS_LARGE_EXPENDABLE_MEMORY_PAYLOAD_PAGES * PAS_EXPENDABLE_MEMORY_PAGE_SIZE) | |
| `#define` PAS_LARGE_EXPENDABLE_MEMORY_TOTAL_SIZE \ | |
| (PAS_LARGE_EXPENDABLE_MEMORY_HEADER_SIZE + PAS_LARGE_EXPENDABLE_MEMORY_PAYLOAD_SIZE) | |
| `#define` PAS_LARGE_EXPENDABLE_MEMORY_ALIGNMENT_UNCAPPED \ | |
| (PAS_LARGE_EXPENDABLE_MEMORY_HEADER_SIZE / sizeof(pas_expendable_memory_state) * PAS_EXPENDABLE_MEMORY_PAGE_SIZE) | |
| `#define` PAS_LARGE_EXPENDABLE_MEMORY_ALIGNMENT_CAPPED \ | |
| (2llu * PAS_LARGE_EXPENDABLE_MEMORY_PAYLOAD_SIZE_CAP) | |
| `#define` PAS_LARGE_EXPENDABLE_MEMORY_ALIGNMENT \ | |
| (PAS_LARGE_EXPENDABLE_MEMORY_ALIGNMENT_UNCAPPED < PAS_LARGE_EXPENDABLE_MEMORY_ALIGNMENT_CAPPED \ | |
| ? PAS_LARGE_EXPENDABLE_MEMORY_ALIGNMENT_UNCAPPED \ | |
| : PAS_LARGE_EXPENDABLE_MEMORY_ALIGNMENT_CAPPED) | |
| `#if` PAS_COMPILER(CLANG) | |
| _Static_assert(!(PAS_LARGE_EXPENDABLE_MEMORY_PAYLOAD_SIZE_CAP % PAS_EXPENDABLE_MEMORY_PAGE_SIZE), | |
| "Large expendable memory payload cap must stay page-aligned."); | |
| _Static_assert(!(PAS_LARGE_EXPENDABLE_MEMORY_ALIGNMENT & (PAS_LARGE_EXPENDABLE_MEMORY_ALIGNMENT - 1)), | |
| "Large expendable memory alignment must stay a power of two."); | |
| _Static_assert(PAS_LARGE_EXPENDABLE_MEMORY_ALIGNMENT > PAS_LARGE_EXPENDABLE_MEMORY_PAYLOAD_SIZE, | |
| "Large expendable memory should be aligned more so than the payload size."); | |
| _Static_assert(PAS_LARGE_EXPENDABLE_MEMORY_ALIGNMENT > PAS_LARGE_EXPENDABLE_MEMORY_TOTAL_SIZE, | |
| "Large expendable memory should be aligned more so than the total size."); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Source/bmalloc/libpas/src/libpas/pas_large_expendable_memory.h` around lines
44 - 77, The new cap/alignment invariants need compile-time guards: add
_Static_asserts so PAS_LARGE_EXPENDABLE_MEMORY_ALIGNMENT is a power of two
(needed by pas_large_expendable_memory_header_for_object which rounds by that
alignment), ensure PAS_LARGE_EXPENDABLE_MEMORY_PAYLOAD_PAGES_CAPPED is exactly
page-aligned (i.e. PAS_LARGE_EXPENDABLE_MEMORY_PAYLOAD_SIZE_CAP is a multiple of
PAS_EXPENDABLE_MEMORY_PAGE_SIZE) to avoid truncation, and keep the existing
assertions that alignment > payload and alignment > total size; place these
asserts near the other _Static_asserts referencing
PAS_LARGE_EXPENDABLE_MEMORY_ALIGNMENT,
PAS_LARGE_EXPENDABLE_MEMORY_PAYLOAD_PAGES_CAPPED (or
PAS_LARGE_EXPENDABLE_MEMORY_PAYLOAD_SIZE_CAP) to cause compile-time failures if
future edits break these invariants.
Comment on lines
49
to
58
| // aarch64 systems seem to be all over the place. Most Linux distros use 4 KiB, but RHEL uses | ||
| // 64 KiB. Linux on Apple Silicon uses 16KiB for best performance, so use that for Linux on | ||
| // aarch64 by default. USE(64KB_PAGE_BLOCK) allows overriding this. | ||
| // 64 KiB. Since we cannot know the page size at compile time on Linux, use 64 KiB as the ceiling | ||
| // for Linux ARM64 to support all possible page size configurations (4 KiB, 16 KiB, 64 KiB). | ||
| // USE(64KB_PAGE_BLOCK) allows overriding this on other architectures too. | ||
| // | ||
| // Use 64 KiB for any unknown CPUs to be conservative. | ||
| #if OS(DARWIN) || PLATFORM(PLAYSTATION) || CPU(MIPS) || CPU(MIPS64) || CPU(LOONGARCH64) || (OS(LINUX) && CPU(ARM64) && !USE(64KB_PAGE_BLOCK)) | ||
| #if OS(DARWIN) || PLATFORM(PLAYSTATION) || CPU(MIPS) || CPU(MIPS64) || CPU(LOONGARCH64) | ||
| constexpr size_t CeilingOnPageSize = 16 * KB; | ||
| #elif USE(64KB_PAGE_BLOCK) || CPU(PPC) || CPU(PPC64) || CPU(PPC64LE) || CPU(UNKNOWN) | ||
| #elif USE(64KB_PAGE_BLOCK) || (OS(LINUX) && CPU(ARM64)) || CPU(PPC) || CPU(PPC64) || CPU(PPC64LE) || CPU(UNKNOWN) | ||
| constexpr size_t CeilingOnPageSize = 64 * KB; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
LGTM — this ceiling change belongs here.
It makes the compile-time upper bound safe for 64 KiB Linux ARM64 kernels without perturbing the other ARM64 ports. If CI does not already cover it, a Linux ARM64 / 64 KiB-page builder would be worth keeping on this path so future regressions fail before landing.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Source/WTF/wtf/PageBlock.h` around lines 49 - 58, The change to set
CeilingOnPageSize to 64 * KB for the (USE(64KB_PAGE_BLOCK) || (OS(LINUX) &&
CPU(ARM64)) || CPU(PPC/ PPC64/ PPC64LE) || CPU(UNKNOWN)) branch is correct;
approve and merge as-is, and optionally add/ensure a Linux ARM64 build with 64
KiB pages in CI to catch regressions; look for the CeilingOnPageSize constexpr
in PageBlock.h to confirm the conditional and value are unchanged before
merging.
The previous hardcoded 2048 assumed PAS_SMALL_PAGE_DEFAULT_SIZE=16KB. With 64KB pages, num_alloc_bits = page_size >> min_align_shift can reach 8192 (utility heap, PAS_INTERNAL_MIN_ALIGN_SHIFT=3), causing pas_baseline_allocator_attach_directory to PAS_ASSERT when a thread hits the baseline allocator slow path.
dylan-conway
force-pushed
the
claude/linux-arm64-64kb-pages-v2
branch
from
d584d24 to
96bff1f
Compare
dylan-conway
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Source/bmalloc/libpas/src/libpas/pas_internal_config.h`:
- Around line 155-157: PAS_MAX_OBJECTS_PER_PAGE is currently defined from
PAS_SMALL_PAGE_DEFAULT_SIZE but needs to be self-checking against all segregated
small-page configs; modify the definition to compute the value from the actual
maximum small-page size across the segregated configs (use a compile-time MAX
macro over those PAS_..._SMALL_PAGE_DEFAULT_SIZE symbols) or add a compile-time
check (static_assert/BUILD_ASSERT) that PAS_MAX_OBJECTS_PER_PAGE >=
(MAX_SMALL_PAGE_DEFAULT_SIZE >> PAS_INTERNAL_MIN_ALIGN_SHIFT); update the
macro/compile-time check near PAS_MAX_OBJECTS_PER_PAGE and reference the
segregated config size symbols (the various PAS_*_SMALL_PAGE_DEFAULT_SIZE
macros) and PAS_INTERNAL_MIN_ALIGN_SHIFT so future changes cannot silently
undersize it.
- Around line 62-66: The conditional around PAS_MEDIUM_BITFIT_PAGE_DEFAULT_SHIFT
is a no-op because both branches set it to 19; remove the `#if/`#else/#endif and
replace with a single define for PAS_MEDIUM_BITFIT_PAGE_DEFAULT_SHIFT 19,
eliminating references to PAS_ARM64 and PAS_OS(LINUX) in this block while
preserving the same macro name and value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: cd75617d-d91f-4d63-a428-2b3e1c73304c
📒 Files selected for processing (1)
Source/bmalloc/libpas/src/libpas/pas_internal_config.h
Comment on lines
+62
to
+66
| #if PAS_ARM64 && PAS_OS(LINUX) | ||
| #define PAS_MEDIUM_BITFIT_PAGE_DEFAULT_SHIFT 19 | ||
| #else | ||
| #define PAS_MEDIUM_BITFIT_PAGE_DEFAULT_SHIFT 19 | ||
| #endif |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Drop the no-op branch for PAS_MEDIUM_BITFIT_PAGE_DEFAULT_SHIFT.
Both branches define 19, so this #if adds maintenance noise without changing behavior. Collapse it to a single define unless a Linux-specific value is still intended.
♻️ Suggested cleanup
-#if PAS_ARM64 && PAS_OS(LINUX)
-#define PAS_MEDIUM_BITFIT_PAGE_DEFAULT_SHIFT 19
-#else
-#define PAS_MEDIUM_BITFIT_PAGE_DEFAULT_SHIFT 19
-#endif
+#define PAS_MEDIUM_BITFIT_PAGE_DEFAULT_SHIFT 19📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| #if PAS_ARM64 && PAS_OS(LINUX) | |
| #define PAS_MEDIUM_BITFIT_PAGE_DEFAULT_SHIFT 19 | |
| #else | |
| #define PAS_MEDIUM_BITFIT_PAGE_DEFAULT_SHIFT 19 | |
| #endif | |
| `#define` PAS_MEDIUM_BITFIT_PAGE_DEFAULT_SHIFT 19 |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Source/bmalloc/libpas/src/libpas/pas_internal_config.h` around lines 62 - 66,
The conditional around PAS_MEDIUM_BITFIT_PAGE_DEFAULT_SHIFT is a no-op because
both branches set it to 19; remove the `#if/`#else/#endif and replace with a
single define for PAS_MEDIUM_BITFIT_PAGE_DEFAULT_SHIFT 19, eliminating
references to PAS_ARM64 and PAS_OS(LINUX) in this block while preserving the
same macro name and value.
Comment on lines
+155
to
+157
| /* Must be >= max(page_size >> min_align_shift) across all segregated configs. | ||
| Utility heap uses PAS_INTERNAL_MIN_ALIGN_SHIFT (3) with the small page. */ | ||
| #define PAS_MAX_OBJECTS_PER_PAGE (PAS_SMALL_PAGE_DEFAULT_SIZE >> PAS_INTERNAL_MIN_ALIGN_SHIFT) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Make PAS_MAX_OBJECTS_PER_PAGE self-checking.
This is correct for the current constants, but the bound now depends on the small-page config remaining the worst case. Please derive it from the actual max across the segregated configs, or add compile-time checks, so future page/alignment tweaks can't silently undersize it.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Source/bmalloc/libpas/src/libpas/pas_internal_config.h` around lines 155 -
157, PAS_MAX_OBJECTS_PER_PAGE is currently defined from
PAS_SMALL_PAGE_DEFAULT_SIZE but needs to be self-checking against all segregated
small-page configs; modify the definition to compute the value from the actual
maximum small-page size across the segregated configs (use a compile-time MAX
macro over those PAS_..._SMALL_PAGE_DEFAULT_SIZE symbols) or add a compile-time
check (static_assert/BUILD_ASSERT) that PAS_MAX_OBJECTS_PER_PAGE >=
(MAX_SMALL_PAGE_DEFAULT_SIZE >> PAS_INTERNAL_MIN_ALIGN_SHIFT); update the
macro/compile-time check near PAS_MAX_OBJECTS_PER_PAGE and reference the
segregated config size symbols (the various PAS_*_SMALL_PAGE_DEFAULT_SIZE
macros) and PAS_INTERNAL_MIN_ALIGN_SHIFT so future changes cannot silently
undersize it.
Preview Builds
|
Array splatting @('-Baseline') to a script file binds to $args, not
[switch]$Baseline. The baseline Windows artifact was being built with
-march=haswell.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Re-apply of #165 plus fix for
PAS_MAX_OBJECTS_PER_PAGE.The previous hardcoded
2048was16KB >> PAS_INTERNAL_MIN_ALIGN_SHIFT. With 64KB small pages,num_alloc_bitscan reach 8192, causingpas_baseline_allocator_attach_directorytoPAS_ASSERTwhen a thread hits the baseline allocator slow path (observed as SIGTRAP on Worker creation).Now derived from
PAS_SMALL_PAGE_DEFAULT_SIZEso it tracks automatically.