Add basic allocators#22
Merged
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a modular memory subsystem: Slice and allocator contract, nil stubs, fixed/bump/page allocator implementations with tests, and CMake/module wiring to expose core.memory. ChangesMemory Allocation Framework
Estimated code review effort: Suggested reviewers:
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 14
🧹 Nitpick comments (4)
engine/native/core/memory/fixedAllocator.cppm (1)
56-60: ⚡ Quick winMake
fixedAllocatorVtblimmutable.This vtable is process-wide function-pointer state; using
constinit constprevents accidental runtime mutation.Suggested patch
- AllocatorVTbl fixedAllocatorVtbl = { + constinit const AllocatorVTbl fixedAllocatorVtbl = { .alloc = alloc, .free = nilFree, .freeAll = freeAll, };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@engine/native/core/memory/fixedAllocator.cppm` around lines 56 - 60, The vtable variable fixedAllocatorVtbl should be defined as immutable to prevent runtime mutation: change its declaration to use constinit const (e.g., constinit const AllocatorVTbl fixedAllocatorVtbl = { .alloc = alloc, .free = nilFree, .freeAll = freeAll };) so the process-wide function-pointer state (AllocatorVTbl fixedAllocatorVtbl) is initialized at compile/runtime start and cannot be modified thereafter.engine/native/core/memory/bumpAllocator.test.cpp (2)
66-66: 💤 Low valueDocument why
alignof(Foo)is used instead ofalignof(uint32_t).Line 66 allocates
sizeof(uint32_t)but requestsalignof(Foo)alignment. While this is intentional for the "well packed" test, it's non-obvious. A brief comment would clarify the intent.📝 Proposed clarification
+ // Use Foo alignment to verify the allocator respects higher alignment requirements err = alloc.vtbl->alloc(alloc, &aSlice, sizeof(uint32_t), alignof(Foo));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@engine/native/core/memory/bumpAllocator.test.cpp` at line 66, Add a short in-place comment next to the alloc.vtbl->alloc(alloc, &aSlice, sizeof(uint32_t), alignof(Foo)) call explaining that the allocation deliberately requests the alignment of Foo while only allocating sizeof(uint32_t) to simulate/verify "well packed" behavior for subsequent Foo placements; mention that this mismatch is intentional to test alignment rounding and packing logic (reference symbols: alloc.vtbl->alloc, aSlice, Foo, uint32_t).
19-19: 💤 Low valueClarify or remove commented-out assertions.
Lines 19, 40, and 69 contain commented-out checks for
first->next == nullptr. Either enable these assertions if they represent valid test expectations, or remove them to avoid clutter. If they were disabled due to implementation behavior, document why with a comment.Also applies to: 40-40, 69-69
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@engine/native/core/memory/bumpAllocator.test.cpp` at line 19, The commented-out assertions checking bumpAlloc.first->next == nullptr should either be re-enabled or removed with an explanatory comment: inspect the test cases around the bumpAlloc variable in bumpAllocator.test to determine whether first->next should indeed be null after allocation; if the implementation guarantees it, uncomment the REQUIRE lines, otherwise delete them and add a brief comment explaining why first->next may be non-null (e.g., allocator metadata/fragmentation) so future readers understand the intent; apply the same change consistently for the other two occurrences of the same commented assertion.engine/native/core/memory/bumpAllocator.cppm (1)
83-83: 💤 Low valuePrefer logical
&&over bitwise&for readability.Using bitwise
&instead of logical&&is non-idiomatic and harms readability. While it may avoid a branch, modern compilers optimize logical operators effectively, and the clarity cost is significant.♻️ Proposed fix
- while (((*node) != nullptr) & (pos > 0)) + while (((*node) != nullptr) && (pos > 0))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@engine/native/core/memory/bumpAllocator.cppm` at line 83, The while loop condition in bumpAllocator.cppm uses the bitwise operator '&' in "while (((*node) != nullptr) & (pos > 0))"; change it to the logical operator '&&' so the condition reads using logical AND (e.g., "((*node) != nullptr) && (pos > 0)") to improve readability and intent for the loop that iterates based on 'node' and 'pos'.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@engine/native/core/memory/bumpAllocator.cppm`:
- Around line 106-109: The newly allocated Node instance (symbol: Node via
pointer variable node) has its size set but its next pointer never initialized;
update the allocation logic where (*node) is assigned (and after (*node)->size
is set) to explicitly initialize (*node)->next to nullptr so deinit/list
traversal cannot follow garbage, and ensure any related variables like lastNode
and pos remain consistent after this change.
- Line 95: The calculation of required node size using reqSize = (sizeof(Node) +
size) & ~alignMask is rounding down; update the calculation in the bump
allocator where reqSize is computed (the variables Node, size, alignMask and
reqSize) to round up by adding alignMask before masking, e.g. compute reqSize as
((sizeof(Node) + size + alignMask) & ~alignMask), ensuring the alignment mask
logic remains consistent with the allocator's alignment assumptions.
- Around line 111-114: The alignment arithmetic duplicates an incorrect reqSize
calculation and doesn't reflect the final aligned pointer; fix the allocation so
you first compute currentPtr from &((*lastNode)->data[oldPos]), then compute
alignedPtr = (currentPtr + alignMask) & ~alignMask, compute reqSize = alignedPtr
- currentPtr + size (or reqSize = (alignedPtr - currentPtr) + size) to include
padding plus requested size, and finally update allocData->allocated += reqSize;
update any uses of currentPtr to use alignedPtr for writing the allocation.
- Around line 108-110: After allocating a new node (the branch that assigns
`node` for the new allocation), update the per-node position state so `lastNode`
points to that newly allocated `node` and reset the in-node offsets: set `oldPos
= 0` (the offset within the new node) and `pos = 0`, then assign `lastNode =
node`; this ensures subsequent uses (e.g., the code that reads `oldPos` on the
next line) use the correct offset inside the newly allocated node.
- Line 17: Remove the `using namespace std;` from the exported module interface
in bumpAllocator.cppm and replace it with explicit std:: qualifications: delete
the directive and update all uses of standard library symbols in this module
(e.g., std::min, std::max, std::memset, std::size_t, std::swap, etc.) so callers
are not polluted; search for occurrences in functions and class definitions
within the exported module and prepend `std::` to each standard symbol or
include specific `using std::X;` declarations for only the needed names if
preferred.
- Around line 91-92: The calculation for the current allocation base uses the
wrong index variable: replace the use of pos with oldPos when computing
currentPtr (i.e., compute currentPtr from &((*lastNode)->data[oldPos])) so the
subsequent alignment adjustment of reqSize using currentPtr and alignMask uses
the actual previous offset; update the expression that computes currentPtr and
leave the alignment line (reqSize += ((reqSize + currentPtr - 1) & alignMask))
unchanged so it operates on the correct base.
- Line 74: Before computing alignMask, validate that the incoming align value is
a non-zero power of two by checking (align > 0 && (align & (align - 1)) == 0);
if the check fails, bail out with a clear failure (throw std::invalid_argument
or assert/log and return) so the subsequent bitmasking is safe; add this guard
immediately before the line that computes uintptr_t alignMask = align - 1 and
include a descriptive error message that mentions align and alignMask to aid
debugging.
- Line 92: The alignment padding calculation using reqSize += ((reqSize +
currentPtr - 1) & alignMask) is wrong; instead compute the aligned pointer or
padding based on power-of-two alignment and add that padding to reqSize. Replace
that expression by computing padding = (-currentPtr) & alignMask (or padding =
(alignment - (currentPtr & alignMask)) & alignMask) and then do reqSize +=
padding (or compute alignedPtr = currentPtr + padding and set reqSize +=
(alignedPtr - currentPtr)); update the code around reqSize, currentPtr, and
alignMask in bumpAllocator.cppm accordingly.
- Around line 65-118: In Error alloc (BumpAllocator) the variable oldPos is left
with a non-zero value after allocating a new node, so subsequent pointer math
uses the wrong offset; after the new node allocation block where you set (*node)
= (Node *)newBlock.data and lastNode = node, reset oldPos = 0 (and ensure the
newly created (*node)->next is null if not already) so the following
calculations using oldPos/currentPtr reference the start of the new node.
In `@engine/native/core/memory/bumpAllocator.test.cpp`:
- Around line 6-101: Add deterministic tests that assert exact pointer offsets
and consumed sizes to catch arithmetic/alignment bugs in
bump::BumpAllocator::alloc (via alloc.vtbl->alloc), and add tests for large
alignments (16/32/64) and sequences of allocations with varying sizes/alignments
to ensure the allocator advances bumpAlloc.allocated correctly; also add tests
for saveMark and resumeMark to save a bump offset, perform mixed allocations,
then resume and verify that subsequent allocations reuse the same exact
addresses and that bumpAlloc.first->size/allocated reflect expected values.
Ensure tests use uintptr_t pointer arithmetic comparisons and check page
chaining behavior when allocations span pages.
In `@engine/native/core/memory/fixedAllocator.cppm`:
- Around line 28-47: The alloc function currently ignores requested size/align;
update FixedAllocator::alloc to validate the request: compute an alignment
offset for allocData->buffer (use uintptr_t arithmetic) as offset = (align -
(uintptr_t(buffer) % align)) % align, then check that offset + size <=
allocData->size and that allocData->allocated is false; if the check fails
return Error::OutOfMemory, otherwise set dst->data = allocData->buffer + offset,
dst->size = size, mark allocData->allocated = true and return Error::Okay.
Ensure you reference the Allocator.allocatorData -> FixedAllocator, the Slice
dst, and Error::OutOfMemory / Error::Okay symbols when making the change.
In `@engine/native/core/memory/pageAllocator.cppm`:
- Around line 85-108: allocLargePages calls GetLargePageMinimum() without
checking for zero, which wraps to SIZE_MAX and breaks the sizing math; fix by
querying GetLargePageMinimum() into a variable (e.g., pageSize), and if pageSize
== 0 return early (either AllocatorError::OutOfMemory or a new AllocatorError
value like UnsupportedLargePages) or fall back to a non-large-page allocation
path, then compute pageSizeSub1 = pageSize - 1 and proceed to call VirtualAlloc
and populate dst->data/dst->size as before.
- Around line 110-113: The free function currently calls VirtualFree(block.data,
block.size, MEM_DECOMMIT | MEM_RELEASE) and returns void; fix it to match the
Unix implementation by returning Error and use the correct flag based on
block.size: if block.size == 0 call VirtualFree(block.data, 0, MEM_RELEASE) to
release the entire region, otherwise call VirtualFree(block.data, block.size,
MEM_DECOMMIT) to decommit pages. Check VirtualFree's BOOL return, convert
failure via GetLastError() into the Error return value, and update the function
signature (free(Allocator alloc, Slice block) -> Error) and any call sites
accordingly; reference symbols: free, Allocator, Slice, VirtualFree, Error.
- Line 56: The Windows implementations of the allocator functions have incorrect
return types: change the Windows alloc and free in pageAllocator.cppm to return
Error (matching the AllocatorVTbl vtable) instead of AllocatorError and void;
update the function signatures for alloc and free to return Error and ensure
they return appropriate Error values on failure/success consistent with the Unix
implementations and the AllocatorVTbl expectations.
---
Nitpick comments:
In `@engine/native/core/memory/bumpAllocator.cppm`:
- Line 83: The while loop condition in bumpAllocator.cppm uses the bitwise
operator '&' in "while (((*node) != nullptr) & (pos > 0))"; change it to the
logical operator '&&' so the condition reads using logical AND (e.g., "((*node)
!= nullptr) && (pos > 0)") to improve readability and intent for the loop that
iterates based on 'node' and 'pos'.
In `@engine/native/core/memory/bumpAllocator.test.cpp`:
- Line 66: Add a short in-place comment next to the alloc.vtbl->alloc(alloc,
&aSlice, sizeof(uint32_t), alignof(Foo)) call explaining that the allocation
deliberately requests the alignment of Foo while only allocating
sizeof(uint32_t) to simulate/verify "well packed" behavior for subsequent Foo
placements; mention that this mismatch is intentional to test alignment rounding
and packing logic (reference symbols: alloc.vtbl->alloc, aSlice, Foo, uint32_t).
- Line 19: The commented-out assertions checking bumpAlloc.first->next ==
nullptr should either be re-enabled or removed with an explanatory comment:
inspect the test cases around the bumpAlloc variable in bumpAllocator.test to
determine whether first->next should indeed be null after allocation; if the
implementation guarantees it, uncomment the REQUIRE lines, otherwise delete them
and add a brief comment explaining why first->next may be non-null (e.g.,
allocator metadata/fragmentation) so future readers understand the intent; apply
the same change consistently for the other two occurrences of the same commented
assertion.
In `@engine/native/core/memory/fixedAllocator.cppm`:
- Around line 56-60: The vtable variable fixedAllocatorVtbl should be defined as
immutable to prevent runtime mutation: change its declaration to use constinit
const (e.g., constinit const AllocatorVTbl fixedAllocatorVtbl = { .alloc =
alloc, .free = nilFree, .freeAll = freeAll };) so the process-wide
function-pointer state (AllocatorVTbl fixedAllocatorVtbl) is initialized at
compile/runtime start and cannot be modified thereafter.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a92f1d9e-81b9-4c44-a943-2661d84041dd
📒 Files selected for processing (10)
engine/native/CMakeLists.txtengine/native/core/CMakeLists.txtengine/native/core/core.cppmengine/native/core/memory/allocator.cppmengine/native/core/memory/bumpAllocator.cppmengine/native/core/memory/bumpAllocator.test.cppengine/native/core/memory/fixedAllocator.cppmengine/native/core/memory/pageAllocator.cppmengine/native/core/memory/root.cppmengine/native/core/memory/slice.cppm
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
engine/native/core/memory/pageAllocator.cppm (1)
48-51:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't drop
munmap/VirtualFreefailures on the floor.Both wrappers return
Error::Okayunconditionally even though the OS APIs report failure. That makes failed releases indistinguishable from successful ones and hides leaks/debugging signal in the only path that can surface them.Also applies to: 110-113
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@engine/native/core/memory/pageAllocator.cppm` around lines 48 - 51, The wrappers Error free(Allocator alloc, Slice block) and the corresponding Windows variant currently always return Error::Okay even when munmap/VirtualFree fail; modify both functions to check the OS call return value (munmap returns -1 on error, VirtualFree returns zero on failure), capture the platform error (errno on POSIX, GetLastError() on Windows), map it to the project's Error enum (e.g. return an Error::System or Error::OsFailure with the code/message) and return that failure instead of Error::Okay; ensure you update both the Slice-based free (Error free(Allocator alloc, Slice block)) and the other free overload around the 110-113 area to propagate real OS errors rather than swallowing them.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@engine/native/core/memory/pageAllocator.cppm`:
- Around line 29-30: The current rounding to page size computes reqSize as (size
+ pageSizeSub1) & ~pageSizeSub1 which can overflow if size + pageSizeSub1 wraps;
before doing the addition in each allocation routine replace the direct add with
an overflow guard: check if size > SIZE_MAX - pageSizeSub1 (or size +
pageSizeSub1 < size) and return/throw a clean allocation-failure (or
errno/false) instead of proceeding, then compute reqSize = (size + pageSizeSub1)
& ~pageSizeSub1; apply this guard everywhere you compute pageSizeSub1 and
reqSize (the three places using variables pageSizeSub1, reqSize and size).
---
Duplicate comments:
In `@engine/native/core/memory/pageAllocator.cppm`:
- Around line 48-51: The wrappers Error free(Allocator alloc, Slice block) and
the corresponding Windows variant currently always return Error::Okay even when
munmap/VirtualFree fail; modify both functions to check the OS call return value
(munmap returns -1 on error, VirtualFree returns zero on failure), capture the
platform error (errno on POSIX, GetLastError() on Windows), map it to the
project's Error enum (e.g. return an Error::System or Error::OsFailure with the
code/message) and return that failure instead of Error::Okay; ensure you update
both the Slice-based free (Error free(Allocator alloc, Slice block)) and the
other free overload around the 110-113 area to propagate real OS errors rather
than swallowing them.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 14f4cf83-614d-4ee2-ab07-0e8e92793f7c
📒 Files selected for processing (4)
engine/native/core/memory/bumpAllocator.cppmengine/native/core/memory/bumpAllocator.test.cppengine/native/core/memory/fixedAllocator.cppmengine/native/core/memory/pageAllocator.cppm
🚧 Files skipped from review as they are similar to previous changes (3)
- engine/native/core/memory/fixedAllocator.cppm
- engine/native/core/memory/bumpAllocator.test.cpp
- engine/native/core/memory/bumpAllocator.cppm
Contributor
|
Interface is clean and C++ enough in my book. I would have even kept the void*, but let's keep the rabbit happy. Only thing I'd consider is moving the implementation (along with most #includes) to .cpp files (just declare the same module there, no export). |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@engine/native/core/memory/bumpAllocator.cpp`:
- Around line 3-7: The code uses std::popcount in bumpAllocator.cpp but doesn’t
include the <bit> header; add the missing include by inserting `#include` <bit>
near the other headers so std::popcount is defined (this affects code around the
popcount usage in bumpAllocator.cpp, e.g., where std::popcount is called).
- Around line 66-69: The code dereferences (*lastNode) to compute currentPtr and
reqSize before checking if (*lastNode) is null, causing UB on first allocation;
move the null-check for (*lastNode) ahead of any dereference and only compute
currentPtr and the alignment-adjusted reqSize after confirming (*lastNode)
exists (or handle the first-node path separately), i.e., ensure the branch that
allocates/creates a new node for allocData->first runs before using
(*lastNode)->data/size, keeping symbols currentPtr, reqSize, alignMask, oldPos,
(*lastNode), and allocData->first as reference points when reorganizing the
logic.
In `@engine/native/core/memory/fixedAllocator.test.cpp`:
- Around line 15-16: The test currently calls alloc.vtbl->alloc(alloc, &block,
512, 16) and immediately checks alignment of block.data; change it to capture
the return value (e.g. Error err = alloc.vtbl->alloc(...)) and add a REQUIRE(err
== Error::Okay) before asserting alignment of block.data so you don't validate
an uninitialized pointer; reference the alloc.vtbl->alloc call, the block
variable, and Error::Okay when making this change.
In `@engine/native/core/memory/pageAllocator.cpp`:
- Line 79: The functions in pageAllocator.cpp currently return values like
AllocatorError::OutOfMemory while their declared return type is Error; update
those return sites (where AllocatorError::... is used) to return the
corresponding Error enum values (e.g., Error::OutOfMemory) or perform an
explicit static_cast<Error>(AllocatorError::...) if the two enums must remain
distinct; look for usages of AllocatorError in this file (the return statements
at the spots noted) and replace them with Error::... or casted values to match
the function signature.
- Around line 49-50: The code currently ignores OS-free failures by calling
munmap(block.data, block.size) and VirtualFree(...) and then unconditionally
returning Error::Okay; modify the deallocation paths to check the return values
of munmap and VirtualFree, and if they indicate failure, convert and propagate
an appropriate Error (include platform-specific detail from errno on POSIX and
GetLastError() on Windows) instead of returning Error::Okay; update both
occurrences (the munmap call using block.data/block.size and the VirtualFree
call) to return the failure Error when the OS call fails, preserving the success
path to return Error::Okay.
- Line 29: The calculation for reqSize uses (size + pageSizeSub1) which can
overflow; add an explicit overflow check before the rounding at each occurrence
(the three sites computing reqSize) by testing if size > SIZE_MAX - pageSizeSub1
(or similar safe bound) and handle the overflow path consistently (e.g., return
an error/false, throw std::overflow_error, or otherwise fail allocation) instead
of performing the wraparound; ensure you update all three locations that compute
reqSize (the occurrences referencing size, pageSizeSub1, and reqSize) so callers
cannot receive an under-sized allocation.
- Line 22: The allocator currently accepts the size_t parameter named "align"
but ignores it; update the page allocator functions in pageAllocator.cpp that
declare the parameter "align" (the three overloads accepting size_t align) to
validate and enforce alignment: first ensure align is a power-of-two and
non-zero, then either round the requested allocation to meet align (using the
allocator's granularity constants such as PAGE_SIZE / ALLOCATION_GRANULARITY /
LARGE_PAGE_MIN) or reject the request by returning an error/NULL if align
exceeds what the allocator can guarantee; add a clear check at the start of each
function to compare align against the allocator's guaranteed alignment and
handle accordingly so callers receive correctly aligned memory or an explicit
failure.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 50c8ff6d-6276-443b-8fbb-ae0ef6dd0359
📒 Files selected for processing (10)
engine/native/core/memory/allocator.cppengine/native/core/memory/allocator.cppmengine/native/core/memory/bumpAllocator.cppengine/native/core/memory/bumpAllocator.cppmengine/native/core/memory/bumpAllocator.test.cppengine/native/core/memory/fixedAllocator.cppengine/native/core/memory/fixedAllocator.cppmengine/native/core/memory/fixedAllocator.test.cppengine/native/core/memory/pageAllocator.cppengine/native/core/memory/pageAllocator.cppm
🚧 Files skipped from review as they are similar to previous changes (1)
- engine/native/core/memory/bumpAllocator.test.cpp
Comment on lines
+3
to
+7
| #include <algorithm> | ||
| #include <cassert> | ||
| #include <cstddef> | ||
| #include <cstdint> | ||
| #include <cstring> |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify popcount usage and direct header presence in bump allocator implementation.
rg -n --type=cpp 'std::popcount\s*\('
sed -n '1,20p' engine/native/core/memory/bumpAllocator.cppRepository: Redot-Engine/DraconicEngine
Length of output: 593
Add missing <bit> header for std::popcount usage.
std::popcount at line 56 requires the <bit> header. Relying on transitive includes is unreliable across different toolchains and compiler configurations.
Suggested fix
`#include` <algorithm>
+#include <bit>
`#include` <cassert>
`#include` <cstddef>📝 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
| #include <algorithm> | |
| #include <cassert> | |
| #include <cstddef> | |
| #include <cstdint> | |
| #include <cstring> | |
| `#include` <algorithm> | |
| `#include` <bit> | |
| `#include` <cassert> | |
| `#include` <cstddef> | |
| `#include` <cstdint> | |
| `#include` <cstring> |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@engine/native/core/memory/bumpAllocator.cpp` around lines 3 - 7, The code
uses std::popcount in bumpAllocator.cpp but doesn’t include the <bit> header;
add the missing include by inserting `#include` <bit> near the other headers so
std::popcount is defined (this affects code around the popcount usage in
bumpAllocator.cpp, e.g., where std::popcount is called).
| Allocator alloc, | ||
| Slice *dst, | ||
| size_t size, | ||
| size_t align |
Contributor
There was a problem hiding this comment.
align is ignored; enforce or reject unsupported alignment.
On Line 22, Line 58, and Line 90, the allocator accepts align but doesn’t use it. If the allocator contract requires requested alignment, callers can receive insufficiently aligned memory.
A minimal safe behavior is to reject alignments above what this allocator guarantees (page size / allocation granularity / large page minimum), or round allocation strategy accordingly.
Also applies to: 58-58, 90-90
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@engine/native/core/memory/pageAllocator.cpp` at line 22, The allocator
currently accepts the size_t parameter named "align" but ignores it; update the
page allocator functions in pageAllocator.cpp that declare the parameter "align"
(the three overloads accepting size_t align) to validate and enforce alignment:
first ensure align is a power-of-two and non-zero, then either round the
requested allocation to meet align (using the allocator's granularity constants
such as PAGE_SIZE / ALLOCATION_GRANULARITY / LARGE_PAGE_MIN) or reject the
request by returning an error/NULL if align exceeds what the allocator can
guarantee; add a clear check at the start of each function to compare align
against the allocator's guaranteed alignment and handle accordingly so callers
receive correctly aligned memory or an explicit failure.
Contributor
Author
There was a problem hiding this comment.
Not much you can do when you get pages from the operating system, almost all allocations will aligned to within one page resolution.
Contributor
There was a problem hiding this comment.
@VictorSohier, that's a fair point — mmap on Unix returns page-aligned memory (typically 4 KB) and VirtualAlloc on Windows aligns to dwAllocationGranularity (typically 64 KB), so the OS-provided alignment already covers virtually all practical requirements. Silently ignoring align is reasonable here since this is a base/backing allocator, not a general-purpose one.
One lightweight option would be adding a brief comment on the align parameter noting the OS-guaranteed alignment, so callers understand what to expect — but that's purely cosmetic and up to you. I'll withdraw the concern.
🧠 Learnings used
Learnt from: AR-DEV-1
Repo: Redot-Engine/DraconicEngine PR: 16
File: engine/native/rendering/rendergraph/rendergraph.cpp:3-3
Timestamp: 2026-05-17T09:15:49.434Z
Learning: DraconicEngine is intentionally C++23-only (GCC >= 14.0, Clang >= 18.0), with enforcement in cmake/Compiler.cmake. During code reviews, it’s OK to use C++23 standard library headers and facilities across the codebase (e.g., <print>) without raising compatibility concerns, since the build toolchain guarantees C++23 support. Avoid introducing C++ features or dependencies that require toolchains outside GCC/Clang versions enforced by the project.
Contributor
Author
There was a problem hiding this comment.
If the operating system had an alignment parameter, this would be a different story, but fixing this has the potential for gross overallocation.
Contributor
There was a problem hiding this comment.
@VictorSohier, completely agreed. Without an OS-level alignment knob, the only way to honour an arbitrary align would be to over-allocate by up to align - 1 extra bytes and hand back an offset pointer — wasteful and semantically wrong for a page-granularity backing allocator whose whole point is to hand out large, coarse blocks. Leaving align unused (with the understanding that the OS-guaranteed granularity is the effective alignment) is the right call here.
✏️ Learnings added
Learnt from: VictorSohier
Repo: Redot-Engine/DraconicEngine PR: 22
File: engine/native/core/memory/pageAllocator.cpp:22-22
Timestamp: 2026-05-18T22:23:32.940Z
Learning: In `engine/native/core/memory/pageAllocator.cpp` (DraconicEngine), the `align` parameter in `draco::memory::page::alloc` (and `allocLargePages`) is intentionally ignored. The OS already guarantees page-aligned memory (Unix: page size via mmap; Windows: dwAllocationGranularity via VirtualAlloc). Enforcing caller-requested alignment beyond the OS guarantee would require over-allocating extra pages and returning an offset pointer, which is inappropriate for a page-granularity backing allocator. Do not raise alignment-enforcement concerns for this file.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: AR-DEV-1
Repo: Redot-Engine/DraconicEngine PR: 16
File: engine/native/rendering/rendergraph/rendergraph.cpp:3-3
Timestamp: 2026-05-17T09:15:49.434Z
Learning: DraconicEngine is intentionally C++23-only (GCC >= 14.0, Clang >= 18.0), with enforcement in cmake/Compiler.cmake. During code reviews, it’s OK to use C++23 standard library headers and facilities across the codebase (e.g., <print>) without raising compatibility concerns, since the build toolchain guarantees C++23 support. Avoid introducing C++ features or dependencies that require toolchains outside GCC/Clang versions enforced by the project.
Contributor
Author
There was a problem hiding this comment.
It is a page allocator, align - 1, at minimum is 4095 bytes and potentially more depending on page size.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@engine/native/core/memory/pageAllocator.cpp`:
- Around line 62-70: The allocation rounding uses a 32-bit unsigned long mask
(pageSizeSub1) which gets promoted to size_t with zero-extended upper bits
causing truncation for >4GB requests; change the mask and related variable to
size_t (replace unsigned long pageSizeSub1 with size_t pageSizeSub1 and ensure
reqSize computation uses size_t arithmetic) so (~pageSizeSub1) is a full-width
mask when computing reqSize in the code around GetSystemInfo and the reqSize =
(size + pageSizeSub1) & (~pageSizeSub1) calculation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: be5d79b9-6fed-47df-9e0c-ee5f23e2896c
📒 Files selected for processing (1)
engine/native/core/memory/pageAllocator.cpp
JoltedJon
previously approved these changes
May 18, 2026
| .freeAll = freeAll, | ||
| }; | ||
|
|
||
| size_t saveMark(BumpAllocator *self); |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@engine/native/core/memory/bumpAllocator.cpp`:
- Around line 58-95: The loop that walks free nodes can leave pos==0 while node
has advanced, causing lastNode to reference the previous node and mis-evaluate
the fit and padding; after the walk (before computing reqSize/fit) normalize the
boundary by setting lastNode = node and oldPos = 0 when pos == 0, then compute
currentPtr from the actual candidate node address
((uintptr_t)&((*lastNode)->data[oldPos])) and derive reqSize using alignMask
from that pointer; use that reqSize in the existing fit check (the branch using
*lastNode and ((*lastNode)->size - oldPos)) so allocData->base.vtbl->alloc and
subsequent writes cannot overwrite an existing next chain or miscompute padding.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5fd515de-a877-4322-96a4-770b94371e2a
📒 Files selected for processing (3)
engine/native/core/memory/allocator.cppmengine/native/core/memory/bumpAllocator.cppengine/native/core/memory/pageAllocator.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- engine/native/core/memory/allocator.cppm
…inst a pointer that exists, correctly aligning the size.
JoltedJon
previously approved these changes
May 18, 2026
OldDev78
approved these changes
May 19, 2026
Contributor
OldDev78
left a comment
OldDev78
left a comment
There was a problem hiding this comment.
Excellent. Just some minor cleanup, like a few include in cppms, and empty global module fragments. But we have allocators now.
mcdubhghlas
approved these changes
May 19, 2026
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
4 participants
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.
New Features
Summary by CodeRabbit