Feature: Add a scratch allocator #29
Conversation
|
Warning Rate limit exceeded@kimkulling has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 18 minutes and 39 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe changes in this pull request include the addition of a new test file for the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
kimkulling
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Outside diff range and nitpick comments (12)
doc/Common.md (3)
Line range hint 41-47: Fix syntax errors in the TBitField example code.
The code example contains several C++ syntax errors that would prevent compilation:
Apply these corrections to make the example valid C++:
TBitField<uint32_t> bitfield(0);
// The first bit is set
bitfield.setBit(1);
- if (bitfield.getBit(1)) std::cout "First bit is set" << std:endl;
+ if (bitfield.getBit(1)) std::cout << "First bit is set" << std::endl;
bitfield.clearBit(1);
- if (!bitfield.getBit(1)) std::cout "First bit is not set" << std:endl;
+ if (!bitfield.getBit(1)) std::cout << "First bit is not set" << std::endl;56-63: Add documentation for TSharedPtr and Variant sections.
These sections are currently empty but appear to be important utility classes. Since this PR focuses on memory management (adding ScratchAllocator), it would be valuable to properly document these memory-related components, especially TSharedPtr.
Please add:
- Brief introduction explaining the purpose
- Common use cases
- Code examples demonstrating proper usage
Would you like me to help draft the documentation for these sections?
Line range hint 1-63: General documentation improvements needed.
A few minor improvements would enhance the documentation quality:
- Fix typos:
- "USer" → "User"
- "singe" → "single"
- Maintain consistent formatting across code examples (spacing, indentation)
doc/Memory.md (1)
16-18: Fix typos and enhance use cases description.
The current text needs some improvements:
- Fix typo: "sove" → "solve"
- Add missing comma: "For this, you need..."
- Consider adding specific examples of algorithms where this allocator would be beneficial, such as:
- Temporary data structures in graph algorithms
- Intermediate buffers in parsing operations
- Scratch space for sorting algorithms
Apply this diff:
-You need to sove any kind of algorithms. For this you need to work with dynamic allocations, which will be thrown away
-after finishing your work.
+You need to solve algorithms that require temporary storage. For this, you need to work with dynamic allocations, which will be thrown away
+after finishing your work. Common examples include graph traversal, parsing operations, and sorting algorithms.🧰 Tools
🪛 LanguageTool
[typographical] ~17-~17: Use a comma after an introductory phrase.
Context: ...ou need to sove any kind of algorithms. For this you need to work with dynamic allocatio...
(COMMA_INTRODUCTORY_WORDS_PHRASES)
include/cppcore/Memory/TScratchAllocator.h (2)
33-37: Fix documentation typos and enhance class description.
- There's a typo in the @param documentation: "apram" should be "param"
- The class description could benefit from additional details about:
- Memory alignment considerations
- Thread safety guarantees
- Maximum allocation size limitations
Also applies to: 59-59
135-137: Rename reservedMem() to usedMem() for clarity.
The method name reservedMem() suggests it returns the total reserved memory, but it actually returns the used memory (mIndex). Consider renaming it to better reflect its purpose.
template<class T>
-size_t TScratchAllocator<T>::reservedMem() const {
+size_t TScratchAllocator<T>::usedMem() const {
return mIndex;
}CMakeLists.txt (2)
Line range hint 58-64: Add TScratchAllocator.h to memory sources.
The new ScratchAllocator's header file needs to be added to the cppcore_memory_src variable.
Apply this diff:
SET ( cppcore_memory_src
include/cppcore/Memory/MemUtils.h
include/cppcore/Memory/TDefaultAllocator.h
include/cppcore/Memory/TStackAllocator.h
include/cppcore/Memory/TPoolAllocator.h
+ include/cppcore/Memory/TScratchAllocator.h
code/Memory/MemUtils.cpp
)167-167: Unnecessary empty line change.
This appears to be a formatting-only change and can be reverted.
include/cppcore/Memory/TStackAllocator.h (4)
188-191: Add documentation for the reset() method.
The new reset() method lacks documentation explaining its behavior and how it differs from clear(). Add doxygen-style documentation to clarify:
- Purpose of the method
- Difference between reset() and clear()
- Memory management implications
Add documentation like this:
/// @brief Resets the allocator state without deallocating memory.
/// @note Unlike clear(), this method only resets the allocation pointer without
/// freeing memory, allowing for efficient reuse of the allocated buffer.Line range hint 92-116: Consider adding alignment support for the allocator.
The current implementation doesn't handle alignment requirements of allocated types. This could cause issues with types requiring specific alignment (e.g., SIMD types, certain structs).
Consider:
- Adding alignment support in the Header structure
- Aligning the allocated memory appropriately
Example approach:
struct Header {
size_t m_size;
size_t m_alignment; // Store alignment requirement
};
// In alloc method:
size_t align_up(size_t n, size_t alignment) {
return (n + alignment - 1) & ~(alignment - 1);
}Line range hint 143-150: Guard against potential overflow in reserve().
The size calculation size * sizeof(T) could overflow for large values of size.
Add overflow check:
template <class T>
inline void TStackAllocator<T>::reserve(size_t size) {
+ if (size > std::numeric_limits<size_t>::max() / sizeof(T)) {
+ throw std::overflow_error("Requested size would cause overflow");
+ }
if (size > (m_capacity)) {
clear();
m_capacity = size * sizeof(T);
m_data = new byte_t[m_capacity];
}
}Line range hint 29-36: Enhance class documentation with scratch allocation pattern details.
The class documentation should be expanded to include:
- Explanation of the scratch allocation pattern
- Usage examples
- Memory management strategy (LIFO behavior)
- Differences between clear() and reset()
Add to the class documentation:
/// @brief This class implements a simple stack-based allocation scheme.
/// Initially you have to define its size. Each allocation will be done from this initially created
/// pool and must be deallocated in LIFO order. All data will be released when the allocator will
/// be released.
///
/// This allocator is particularly useful for scratch/temporary allocations where memory
/// can be efficiently reused. Example usage:
///
/// @code
/// TStackAllocator<int> allocator(1024); // Reserve space for 1024 ints
/// int* temp1 = allocator.alloc(100); // Allocate space for 100 ints
/// // ... use temp1 ...
/// allocator.release(temp1); // Release in LIFO order
///
/// // Reuse the same memory without deallocation
/// allocator.reset();
/// int* temp2 = allocator.alloc(50); // Reuse the memory
/// @endcode📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (11)
- CMakeLists.txt (2 hunks)
- doc/Common.md (2 hunks)
- doc/Memory.md (1 hunks)
- include/cppcore/IO/FileSystem.h (1 hunks)
- include/cppcore/Memory/TDefaultAllocator.h (1 hunks)
- include/cppcore/Memory/TPoolAllocator.h (1 hunks)
- include/cppcore/Memory/TScratchAllocator.h (1 hunks)
- include/cppcore/Memory/TStackAllocator.h (1 hunks)
- test/Random/RandomGeneratorTest.cpp (1 hunks)
- test/memory/TScratchAllocatorTest.cpp (1 hunks)
- test/memory/TStackAllocatorTest.cpp (1 hunks)
✅ Files skipped from review due to trivial changes (2)
- include/cppcore/IO/FileSystem.h
- include/cppcore/Memory/TDefaultAllocator.h
🧰 Additional context used
🪛 LanguageTool
doc/Memory.md
[typographical] ~17-~17: Use a comma after an introductory phrase.
Context: ...ou need to sove any kind of algorithms. For this you need to work with dynamic allocatio...
(COMMA_INTRODUCTORY_WORDS_PHRASES)
🔇 Additional comments (8)
test/memory/TScratchAllocatorTest.cpp (1)
19-28: Verify memory leak detection.
Consider adding memory leak detection to ensure the allocator properly manages memory.
doc/Memory.md (1)
11-14: LGTM!
The introduction clearly explains the purpose and behavior of the scratch allocator.
test/Random/RandomGeneratorTest.cpp (1)
28-28: LGTM! Clean class declaration.
The simplified class declaration without empty comment block is more concise and aligns with modern C++ style guidelines.
test/memory/TStackAllocatorTest.cpp (2)
29-29: LGTM! Good cleanup of empty protected section.
The simplified class declaration maintains the same functionality while removing unnecessary empty protected section. The test fixture is properly set up with Google Test framework.
29-29: Verify consistency with ScratchAllocator implementation.
Since this PR introduces a new ScratchAllocator, let's ensure consistent patterns across allocator implementations.
✅ Verification successful
Let me gather more specific information about the allocator implementations.
No consistency issues found between TStackAllocator and TScratchAllocator
After reviewing both implementations, they are intentionally different by design:
- TStackAllocator implements a LIFO allocation scheme with explicit release
- TScratchAllocator is designed for one-shot allocations without individual releases
- Both allocators maintain consistent memory management patterns within their respective purposes
- Both implement common interface methods (capacity, reservedMem, freeMem) with consistent behavior
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Compare allocator implementations for consistency
# Find all allocator implementations
echo "=== Searching for allocator implementations ==="
rg -l "class.*Allocator" --type cpp
# Check allocation method signatures
echo -e "\n=== Checking allocation method signatures ==="
ast-grep --pattern 'class $ALLOCATOR {
$$$
$RETURN_TYPE* alloc($$$) {
$$$
}
$$$
}'
# Check release method signatures
echo -e "\n=== Checking release method signatures ==="
ast-grep --pattern 'class $ALLOCATOR {
$$$
bool release($$$) {
$$$
}
$$$
}'
Length of output: 954
Script:
#!/bin/bash
# Let's examine the actual allocator implementations more closely
# Check TStackAllocator.h implementation
echo "=== TStackAllocator.h Contents ==="
cat include/cppcore/Memory/TStackAllocator.h
# Check TScratchAllocator.h implementation
echo -e "\n=== TScratchAllocator.h Contents ==="
cat include/cppcore/Memory/TScratchAllocator.h
# Search for allocation patterns
echo -e "\n=== Searching for allocation patterns ==="
rg "alloc\(" include/cppcore/Memory/ -A 2
# Search for deallocation/release patterns
echo -e "\n=== Searching for deallocation patterns ==="
rg "release\(" include/cppcore/Memory/ -A 2
Length of output: 14736
CMakeLists.txt (2)
153-153: LGTM: Test file addition looks good.
The test file is correctly added to the memory test group and follows the existing pattern.
Line range hint 1-186: Verify implementation file for ScratchAllocator.
If the ScratchAllocator has a separate implementation file (*.cpp), it should be added to the source files.
include/cppcore/Memory/TPoolAllocator.h (1)
97-97:
The reset() implementation needs improvement.
The current implementation of reset() only sets m_current back to m_first but doesn't properly handle the internal state:
- It doesn't reset
m_currentIdxin the pools, which could lead to incorrect allocation tracking - The
m_freeListstate is not addressed, potentially causing memory management issues - Documentation doesn't explain the implications of calling reset()
Consider this improved implementation:
/// @brief Will reset the allocator to its initial state.
/// @note This will invalidate all previously allocated memory. Ensure all allocated objects
/// are properly destroyed before calling reset().
/// @thread_safety This method is not thread-safe and should not be called concurrently
/// with other allocator operations.
void reset() {
if (nullptr == m_first) {
return;
}
// Reset allocation counters
Pool* current = m_first;
while (current != nullptr) {
current->m_currentIdx = 0;
current = current->m_next;
}
// Move all subsequent pools to free list
if (m_first->m_next != nullptr) {
if (m_freeList != nullptr) {
// Find end of current chain
Pool* last = m_first->m_next;
while (last->m_next != nullptr) {
last = last->m_next;
}
// Link to existing free list
last->m_next = m_freeList;
}
m_freeList = m_first->m_next;
m_first->m_next = nullptr;
}
m_current = m_first;
}Let's verify the usage of reset() in tests:
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (6)
doc/Memory.md (1)
11-14: Enhance the introduction with more technical details.
Consider adding more specific details about:
- The memory management model (linear allocation)
- Thread safety considerations
- Memory alignment guarantees
Apply this diff to enhance the introduction:
## CPPCore::TScratchAllocator
### Introduction
-The scratch allocator preallocates a memory block which can be used in your program. You do not have to deallocate any of the allocations.
-This will be done when clearing the allocator. All allocations will be invalidated.
+The scratch allocator implements a linear allocation model by preallocating a contiguous memory block.
+Individual allocations are performed sequentially without tracking, eliminating the need for explicit
+deallocation. The entire memory block can be reset instantly using the clear() operation, which
+invalidates all previous allocations. Note: This allocator is not thread-safe and does not guarantee
+any specific memory alignment.test/memory/TScratchAllocatorTest.cpp (1)
23-27: Enhance header includes and namespace usage.
- Add
<cstddef>for propersize_ttype definition - Consider avoiding
using namespacedirective in header files to prevent potential naming conflicts
#include <cppcore/Memory/TScratchAllocator.h>
+#include <cstddef>
#include <gtest/gtest.h>
-using namespace cppcore;
+// Prefer explicit namespace usage
+using cppcore::ScratchAllocator;include/cppcore/Memory/TScratchAllocator.h (4)
58-59: Fix typo in parameter documentation.
There's a typo in the @param tag: @apram should be @param.
- /// @apram[in] size The pool size to reserve.
+ /// @param[in] size The pool size to reserve.33-38: Enhance class documentation.
The documentation should include important details about:
- Thread safety guarantees
- Memory ownership model
- Alignment guarantees
- Potential exceptions
/// @brief A simple scratch allocator.
///
/// You can use it to manage all the dynamic allocations without caring about cleaning it up. All
/// Allocations will be a one-shot alloc, which do not need to get released. When calling clear
-/// all allocation will be invalidated and cannot be used anymore.
+/// all allocations will be invalidated and cannot be used anymore.
+///
+/// @note This allocator is not thread-safe. Access from multiple threads must be externally synchronized.
+/// @note Memory is owned by the allocator and will be automatically freed when the allocator is destroyed.
+/// @note No alignment guarantees are provided for allocated memory.
+/// @throw std::bad_alloc if memory allocation fails81-83: Document private member variables.
Add documentation for private member variables to improve maintainability.
private:
+ /// Pointer to the allocated memory block
T *mBlock;
+ /// Total size of the allocated block in number of elements
size_t mSize;
+ /// Current allocation index within the block
size_t mIndex;92-96: Add noexcept specification and document exceptions.
The constructor that allocates memory should document potential exceptions.
template<class T>
-inline TScratchAllocator<T>::TScratchAllocator(size_t numItems) :
+inline TScratchAllocator<T>::TScratchAllocator(size_t numItems) noexcept(false) :
mBlock(nullptr), mSize(numItems), mIndex(0u) {
reserve(numItems);
}Add to the documentation:
/// @brief The class constructor with the pool size.
/// @param[in] numItems The buffer size.
+ /// @throw std::bad_alloc if memory allocation fails📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- doc/Memory.md (1 hunks)
- include/cppcore/Memory/TScratchAllocator.h (1 hunks)
- test/memory/TScratchAllocatorTest.cpp (1 hunks)
🧰 Additional context used
🪛 LanguageTool
doc/Memory.md
[typographical] ~17-~17: Use a comma after an introductory phrase.
Context: ...ou need to save any kind of algorithms. For this you need to work with dynamic allocatio...
(COMMA_INTRODUCTORY_WORDS_PHRASES)
🔇 Additional comments (3)
doc/Memory.md (1)
20-38: 🛠️ Refactor suggestion
The example code needs comprehensive updates.
The previous review comments about typos in the include path are still valid. Additionally, the example could be more comprehensive.
Beyond the changes suggested in the previous review, consider enhancing the example further:
-#include <Memroy/TScrachtAllocator.h>
+#include <Memory/TScratchAllocator.h>
using namespace ::cppcore;
+struct Vector3 { float x, y, z; };
+
int main() {
- // Will work
ScratchAllocator myAllocator(1024);
- char *ptr1 = myAllocator.alloc(512);
+
+ // Basic allocation
+ char* ptr1 = myAllocator.alloc(512);
assert(ptr1 != nullptr);
- // Overrange shall get catched
+ // Allocation of array of objects
+ Vector3* vertices = static_cast<Vector3*>(
+ myAllocator.alloc(10 * sizeof(Vector3)));
+ assert(vertices != nullptr);
+
+ // Demonstrate out-of-memory handling
char *ptr2 = myAllocator.alloc(600);
assert(ptr2 == nullptr);
+ // Clear all allocations
+ myAllocator.clear();
+
+ // After clearing, memory can be reused
+ ptr2 = myAllocator.alloc(600);
+ assert(ptr2 != nullptr);
+
return 0;
}test/memory/TScratchAllocatorTest.cpp (1)
64-65: Verify expected behavior after clear().
The test expects capacity and freeMem to become 0 after clear(), which seems incorrect. The clear operation should typically:
- Reset the allocator to its initial state
- Maintain the original capacity
- Restore all memory as free
Either:
- The test assertions are incorrect and should expect BufferSize
- There's a potential bug in ScratchAllocator::clear()
✅ Verification successful
Test assertions are correct, clear() behavior is by design
Looking at the TScratchAllocator implementation, the clear() method is designed to:
- Delete the allocated block
- Set the block pointer to nullptr
- Reset size to 0
- Reset index to 0
The test assertions are correct because:
capacity()returnsmSizewhich is set to 0 inclear()freeMem()returnsmSize - mIndex, which is 0 whenmSizeis 0
This is a deliberate design choice where clear() completely deallocates the memory and resets the allocator to its initial empty state, rather than just resetting the index. The allocator needs to be explicitly reserve()'d again to be reused.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for ScratchAllocator implementation to verify clear() behavior
ast-grep --pattern 'class ScratchAllocator {
$$$
void clear() {
$$$
}
$$$
}'
Length of output: 90
Script:
#!/bin/bash
# Let's try searching for the file containing ScratchAllocator implementation
fd -e h -e hpp -e cpp "ScratchAllocator" --exec cat {}
Length of output: 6977
include/cppcore/Memory/TScratchAllocator.h (1)
144-144: LGTM!
The type alias provides a convenient way to use the allocator with char type.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
include/cppcore/Memory/TScratchAllocator.h (3)
31-39: Enhance class documentation.
The documentation should include important details about:
- Thread safety guarantees
- Exception safety guarantees
- Alignment guarantees for allocations
/// @brief A simple scratch allocator.
///
/// You can use it to manage all the dynamic allocations without caring about cleaning it up. All
/// Allocations will be a one-shot alloc, which do not need to get released. When calling clear
-/// all allocation will be invalidated and cannot be used anymore.
+/// all allocation will be invalidated and cannot be used anymore.
+///
+/// @note This allocator is not thread-safe. External synchronization is required for concurrent access.
+/// @note Strong exception guarantee: if an allocation fails, the allocator state remains unchanged.
+/// @note Allocations are aligned according to the alignment requirements of type T.58-60: Fix typo in parameter documentation.
There's a typo in the @param tag.
/// @brief Will reserve the pool.
- /// @apram[in] size The pool size to reserve.
+ /// @param[in] size The pool size to reserve.
void reserve(size_t size);158-158: Add documentation for the type alias.
Document the purpose and common use cases for the char-based allocator alias.
+/// @brief Convenience alias for a byte-based scratch allocator.
+/// @details Useful for allocating raw memory blocks or byte buffers.
using ScratchAllocator = TScratchAllocator<char>;📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- doc/Memory.md (1 hunks)
- include/cppcore/Memory/TScratchAllocator.h (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- doc/Memory.md
🔇 Additional comments (1)
include/cppcore/Memory/TScratchAllocator.h (1)
128-134:
Add error handling in reserve().
The method should maintain the object's invariants if allocation fails.
template<class T>
-void TScratchAllocator<T>::reserve(size_t size) {
+void TScratchAllocator<T>::reserve(size_t size) noexcept(false) {
+ if (size == 0) {
+ clear();
+ return;
+ }
+
+ T* newBlock = nullptr;
+ try {
+ newBlock = new T[size];
+ } catch (const std::bad_alloc&) {
+ // Keep existing state unchanged
+ return;
+ }
+
clear();
- mBlock = new T[size];
+ mBlock = newBlock;
mSize = size;
mIndex = 0u;
}Likely invalid or redundant comment.
|
Summary by CodeRabbit
Release Notes
New Features
ScratchAllocatorclass for dynamic memory management, allowing for efficient one-shot allocations.reset()method to theTPoolAllocatorclass for improved memory state management.ScratchAllocatorto validate creation, allocation, and clearing functionalities.Bug Fixes
Common.mdfor clarity.Documentation
CPPCore::TScratchAllocatorinMemory.md, including usage examples.Common.mdfor improved clarity and consistency in examples.Style