Tracking allocator#24
Open
VictorSohier wants to merge 1 commit into
Open
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR extends the allocator framework to capture allocation call-site information in debug builds and introduces a new tracking allocator that maintains a doubly-linked list of active allocations for analytics. Core allocator interfaces, concrete implementations, and tests are updated to support the new source-location parameter. ChangesAllocator Source-Location Tracking and Analytics
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 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: 3
🤖 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/fixedAllocator.test.cpp`:
- Around line 15-16: The test calls alloc.alloc(&block, 512, 16) but never
verifies it succeeded before inspecting block.data; change the test to assert
the allocation returned success (e.g., REQUIRE on the boolean/result of
alloc.alloc) before the alignment check, so verify alloc.alloc(...) is true and
only then perform REQUIRE((((uintptr_t)block.data) & 15) == 0); reference
alloc.alloc and block in the fixedAllocator.test.cpp test.
In `@engine/native/core/memory/trackingAllocator.cpp`:
- Around line 88-89: The code currently clears allocData->nodes before calling
allocData->base.vtbl->freeAll(allocData->base); change this so you call freeAll
first, capture its return/result, and only set allocData->nodes = nullptr (and
any other tracking-state resets) if freeAll succeeded; if freeAll fails, leave
allocData->nodes unchanged and return the error result from
allocData->base.vtbl->freeAll(allocData->base) so tracking still reflects live
blocks. Ensure you reference allocData->base.vtbl->freeAll and allocData->nodes
when making the change.
- Around line 69-82: The unlink logic in the free path dereferences prev/next
blindly and always adjusts block.size even when no matching Node was found;
update the code in trackingAllocator.cpp (the Node list manipulation around
node, prev, next) to: only perform memset(node) and add sizeof(Node) to
block.size when a matching node is found; when unlinking, check for nullptr
before dereferencing prev or next and update head/tail pointers appropriately to
handle removing the head, tail, or the sole node; if node is not found, do not
modify block.size or attempt unlinking—simply call
allocData->base.vtbl->free(allocData->base, block) (optionally log or assert the
unexpected free) to avoid crashes and allocator corruption.
🪄 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: 17559e75-eb06-4c1a-8335-b6bf804b8709
📒 Files selected for processing (14)
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.cppmengine/native/core/memory/root.cppmengine/native/core/memory/trackingAllocator.cppengine/native/core/memory/trackingAllocator.cppmengine/native/core/memory/trackingAllocator.test.cpp
Comment on lines
+15
to
16
| alloc.alloc(&block, 512, 16); | ||
| REQUIRE((((uintptr_t)block.data) & 15) == 0); |
Contributor
There was a problem hiding this comment.
Assert allocation success before checking alignment.
Line 15 ignores the alloc.alloc(...) result. If allocation fails, Line 16 may assert on an invalid block.
Proposed fix
- alloc.alloc(&block, 512, 16);
+ Error err = alloc.alloc(&block, 512, 16);
+ REQUIRE(err == Error::Okay);
REQUIRE((((uintptr_t)block.data) & 15) == 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/fixedAllocator.test.cpp` around lines 15 - 16, The
test calls alloc.alloc(&block, 512, 16) but never verifies it succeeded before
inspecting block.data; change the test to assert the allocation returned success
(e.g., REQUIRE on the boolean/result of alloc.alloc) before the alignment check,
so verify alloc.alloc(...) is true and only then perform
REQUIRE((((uintptr_t)block.data) & 15) == 0); reference alloc.alloc and block in
the fixedAllocator.test.cpp test.
Comment on lines
+69
to
+82
| while (node && (node->details.data.data != block.data)) | ||
| { | ||
| node = node->next; | ||
| } | ||
| if (node) | ||
| { | ||
| prev = node->prev; | ||
| next = node->next; | ||
| prev->next = next; | ||
| next->prev = prev; | ||
| memset(node, 0, sizeof(Node)); | ||
| } | ||
| block.size += sizeof(Node); | ||
| return allocData->base.vtbl->free(allocData->base, block); |
Contributor
There was a problem hiding this comment.
Fix free unlink/not-found paths to prevent crashes and allocator corruption.
Line 77 and Line 78 dereference prev/next without null checks, which crashes when freeing head/tail (or the only node). Also, when no node matches, the code still increases block.size and forwards free to the base allocator.
Proposed fix
- if (node)
- {
- prev = node->prev;
- next = node->next;
- prev->next = next;
- next->prev = prev;
- memset(node, 0, sizeof(Node));
- }
- block.size += sizeof(Node);
- return allocData->base.vtbl->free(allocData->base, block);
+ if (!node)
+ {
+ // Return the project-specific non-Okay error for invalid/untracked frees.
+ return Error::Unknown;
+ }
+
+ prev = node->prev;
+ next = node->next;
+ if (prev) { prev->next = next; }
+ else { allocData->nodes = next; }
+ if (next) { next->prev = prev; }
+
+ memset(node, 0, sizeof(Node));
+ block.size += sizeof(Node);
+ return allocData->base.vtbl->free(allocData->base, block);🤖 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/trackingAllocator.cpp` around lines 69 - 82, The
unlink logic in the free path dereferences prev/next blindly and always adjusts
block.size even when no matching Node was found; update the code in
trackingAllocator.cpp (the Node list manipulation around node, prev, next) to:
only perform memset(node) and add sizeof(Node) to block.size when a matching
node is found; when unlinking, check for nullptr before dereferencing prev or
next and update head/tail pointers appropriately to handle removing the head,
tail, or the sole node; if node is not found, do not modify block.size or
attempt unlinking—simply call allocData->base.vtbl->free(allocData->base, block)
(optionally log or assert the unexpected free) to avoid crashes and allocator
corruption.
Comment on lines
+88
to
+89
| allocData->nodes = nullptr; | ||
| return allocData->base.vtbl->freeAll(allocData->base); |
Contributor
There was a problem hiding this comment.
Only clear tracking state after base.freeAll succeeds.
Line 88 clears allocData->nodes before checking Line 89’s result. If base.freeAll fails, analytics/reporting will say zero active allocations while blocks may still be live.
Proposed fix
- allocData->nodes = nullptr;
- return allocData->base.vtbl->freeAll(allocData->base);
+ Error err = allocData->base.vtbl->freeAll(allocData->base);
+ if (err == Error::Okay)
+ {
+ allocData->nodes = nullptr;
+ }
+ return err;📝 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
| allocData->nodes = nullptr; | |
| return allocData->base.vtbl->freeAll(allocData->base); | |
| Error err = allocData->base.vtbl->freeAll(allocData->base); | |
| if (err == Error::Okay) | |
| { | |
| allocData->nodes = nullptr; | |
| } | |
| return err; |
🤖 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/trackingAllocator.cpp` around lines 88 - 89, The
code currently clears allocData->nodes before calling
allocData->base.vtbl->freeAll(allocData->base); change this so you call freeAll
first, capture its return/result, and only set allocData->nodes = nullptr (and
any other tracking-state resets) if freeAll succeeded; if freeAll fails, leave
allocData->nodes unchanged and return the error result from
allocData->base.vtbl->freeAll(allocData->base) so tracking still reflects live
blocks. Ensure you reference allocData->base.vtbl->freeAll and allocData->nodes
when making the change.
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.
New Features
Summary by CodeRabbit