Cascaded Multi Size Based Allocators Achiving the 'infinite' space ability as ACE_Malloc #2135
Conversation
…_TAO into cascadedAllocator
2. using instead of typedef
…to cascadedAllocator not updated
2、make simple for statement at one line 3、add the pool_sum API for component transparency 4、move pool_depth API from INL file to source file for its complexity
2、modify the test case to use lock strategy, that will more likely find some compiling error than free lock
modify the invalid document
…cascadedMalloc sync from github
sync from cascadedAllocator
sync for coding style
1、right const 2、pointer var declaration
sync from cascadedAllocator for coding style
…ly the first and last allocator has free chunks
merge the optimization of the last allocator branch judgement
WalkthroughThis change introduces two new cascaded memory allocator classes into the ACE framework. Both allocators implement dynamic allocation strategies with cascaded approaches and support memory operations using methods such as malloc, calloc, and free. In addition, inline methods are added for locking and pool metrics. The update extends the public API through header modifications and integrates a comprehensive set of tests along with corresponding project configurations, ensuring the new allocation schemes function correctly. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant DCA as CascadedDynamicAllocator
participant CMA as CascadedMultiSizeAllocator
Client->>DCA: Request malloc(nbytes)
DCA->>DCA: Check if nbytes ≤ chunk_size
alt Memory Available in Current Chunk
DCA-->>Client: Return pointer
else Memory Not Sufficient
DCA->>DCA: Create/find child allocator
DCA-->>Client: Return pointer from child
end
Client->>CMA: Request malloc(nbytes)
CMA->>CMA: Evaluate available block sizes
alt Suitable Block Found
CMA-->>Client: Return pointer
else Create New Allocator as Needed
CMA->>CMA: Update hierarchy and metrics
CMA-->>Client: Return pointer
end
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
🪧 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
ACE/ace/Malloc_T.h (2)
215-231: Naming and clarity
The new doc comments explain that the allocator can branch out in a hierarchy. Consider emphasizing any limitations or overhead when many nested allocators exist.
336-425: Cascaded multi-size-based allocator design
- Binary search usage: The approach to find the minimal position in the hierarchy is clear.
- Resize then allocate: Good practice to resize the vector before allocating. If
resizethrows, no heap memory is lost.- Single-octet chunk header: Offsets by one byte for the user pointer. This is a well-known approach; just ensure that alignment is always sufficient for all target platforms.
ACE/ace/Malloc_T.cpp (1)
481-515: Binary search
The binary search to find the chunk size is a good optimization. Watch out for edge cases with very small or zero requests. The code returnsnullptrif the shift limit is exceeded, which is a safe fallback.ACE/tests/Allocator_Cascaded_Test.cpp (4)
22-44: Add documentation for the DELTA macro.While the test macros are well-implemented, the DELTA macro's purpose and calculation could be clearer with documentation.
Add a comment explaining the DELTA macro's purpose and calculation:
+// Calculates the number of chunks for a given hierarchy level. +// If the calculated chunks (initial_n_chunks >> level) is greater than min_initial_n_chunks, +// use the calculated value; otherwise, use min_initial_n_chunks. #define DELTA(level, initial_n_chunks, min_initial_n_chunks) \ (initial_n_chunks >> level) > min_initial_n_chunks ? (initial_n_chunks >> level) : min_initial_n_chunks
45-170: Use RAII to prevent memory leaks in test cases.While the tests are thorough, there's a risk of memory leaks if tests fail between malloc and free calls. Consider using RAII to ensure cleanup.
Consider using a simple RAII wrapper:
class ScopedMemory { public: ScopedMemory(void* ptr, ACE_Cascaded_Dynamic_Cached_Allocator<ACE_SYNCH_MUTEX>& alloc) : ptr_(ptr), alloc_(alloc) {} ~ScopedMemory() { if (ptr_) alloc_.free(ptr_); } void* release() { void* tmp = ptr_; ptr_ = nullptr; return tmp; } private: void* ptr_; ACE_Cascaded_Dynamic_Cached_Allocator<ACE_SYNCH_MUTEX>& alloc_; }; // Usage example: void* ptr = alloc.malloc(nbytes); ScopedMemory guard(ptr, alloc); // Test the allocated memory // Memory is automatically freed when guard goes out of scope
172-521: Reduce code duplication in test functions.The test functions share similar setup code and assertions. Consider extracting common patterns into test helpers.
Consider creating test helpers:
// Test helper for common allocator setup template<typename Allocator> struct AllocatorTestHelper { static Allocator create_allocator(size_t initial_n_chunks = 11, size_t chunk_size = sizeof(void*) + 5, size_t min_initial_n_chunks = 2) { return Allocator(initial_n_chunks, chunk_size, min_initial_n_chunks); } static void verify_pool_metrics(Allocator& alloc, size_t expected_sum, size_t expected_depth, const char* context) { std::stringstream ss; ss << context << " pool sum must be " << expected_sum; ACE_ASSERT_RETURN(alloc.pool_sum() == expected_sum, ss.str().c_str()); ss.str(""); ss << context << " pool depth must be " << expected_depth; ACE_ASSERT_RETURN(alloc.pool_depth() == expected_depth, ss.str().c_str()); } }; // Usage example: using Helper = AllocatorTestHelper<ACE_Cascaded_Multi_Size_Based_Allocator<ACE_SYNCH_MUTEX>>; auto alloc = Helper::create_allocator(); Helper::verify_pool_metrics(alloc, expected_sum, expected_depth, "After allocation");
523-542: Enhance error reporting in the main test function.While the test execution is well-structured, it could provide more detailed feedback when tests fail.
Consider enhancing error reporting:
int run_main (int, ACE_TCHAR *[]) { ACE_START_TEST (ACE_TEXT ("Allocator_Cascaded_Test")); int retval = 0; + struct TestCase { + const char* name; + int (*func)(); + }; + TestCase tests[] = { + {"Cascaded_Allocator", run_cascaded_allocator_test}, + {"Cascaded_Multi_Size_Based_Allocator_Basic", run_cascaded_multi_size_based_allocator_basic_test}, + {"Cascaded_Multi_Size_Based_Allocator_Hierarchy", run_cascaded_multi_size_based_allocator_hierarchy_test}, + {"Cascaded_Multi_Size_Based_Allocator_Hierarchy_Free", run_cascaded_multi_size_based_allocator_hierarchy_free_test}, + {"Cascaded_Multi_Size_Based_Allocator_Hierarchy_Differential", run_cascaded_multi_size_based_allocator_hierarchy_differential_test} + }; - ACE_DEBUG ((LM_INFO, "%C Run the tests for Cascaded_Allocator ...\n", __func__)); - retval += run_cascaded_allocator_test(); - - ACE_DEBUG ((LM_INFO, "%C Run the tests for Cascaded_Multi_Size_Based_Allocator ...\n", __func__)); - retval += run_cascaded_multi_size_based_allocator_basic_test(); - retval += run_cascaded_multi_size_based_allocator_hierarchy_test (); - retval += run_cascaded_multi_size_based_allocator_hierarchy_free_test (); - retval += run_cascaded_multi_size_based_allocator_hierarchy_differential_test (); + for (const auto& test : tests) { + ACE_DEBUG ((LM_INFO, "%C Running test: %C\n", __func__, test.name)); + int result = test.func(); + if (result != 0) { + ACE_ERROR ((LM_ERROR, "%C Test failed: %C\n", __func__, test.name)); + } + retval += result; + } ACE_END_TEST; return retval; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
ACE/ace/Malloc_T.cpp(2 hunks)ACE/ace/Malloc_T.h(4 hunks)ACE/ace/Malloc_T.inl(1 hunks)ACE/tests/Allocator_Cascaded_Test.cpp(1 hunks)ACE/tests/run_test.lst(1 hunks)ACE/tests/tests.mpc(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (26)
ACE/ace/Malloc_T.h (6)
8-9: Authors updated successfully
No issues with adding a new author line here.
19-19: Include for ACE_Null_Mutex
Good addition since the new classes useACE_Null_Mutex.
29-30: STL vector import
Bringing in<vector>is necessary for tracking allocator hierarchies.
210-214: Documentation is clear
The class description is detailed and helps clarify the purpose of the cascaded dynamic allocator.
232-311: Concurrency consideration
Lock usage withACE_MT(ACE_GUARD)is employed in critical methods. This is good. Verify that concurrent calls tomalloc()andfree()on different nested allocators won't lead to unexpected cross-allocator state issues.
313-335: Multi-size-based allocator docs
Documentation is succinct and points out the hierarchical approach for multiple chunk sizes. No issues spotted.ACE/ace/Malloc_T.cpp (14)
17-17: Smart pointer imports
Including<memory>is needed forstd::unique_ptr; recommended usage for exception safety.
181-203: Constructor’s exception safety
Storing the newly allocated child allocator in astd::unique_ptrthen pushing back prevents leaks. The finalrelease()call is appropriate oncepush_backsucceeds.
204-215: Hierarchy deallocation
Destroying all allocators within the vector is correct. Good cleanup in the destructor.
216-260:mallocmethod concurrency
The method locks around the allocation logic. This is essential because it potentially modifies the allocator hierarchy. Good job ensuring consistency.
216-260: Possible integer overflow
When creating a new child allocator,1 << sizeis used to scale chunks. This could overflow on large values. A pre-check or safe cast is recommended.
262-297:callocandfree
Thecallocmethod reusesmalloc. Thefreemethod routes deallocation to the first allocator in the list. This design is consistent with the doc but verify that each pointer truly originates from the hierarchy.
299-364: Stubs for map-like operations
remove,bind,trybind, etc., are explicitly not supported. Returning-1is fine—just ensure the calling code either handles or never calls these.
366-404:pool_depthanddump
Implementation is consistent with the doc. Using the lock insidepool_depthensures thread safety. Dump logic is guarded byACE_HAS_DUMP.
406-449: Multi-size-based constructor
Allocates an initial cascade level with slight overhead for the “header” byte. This clarifies how each chunk is tracked.
451-479: Multi-size-based destructor
Looping through the hierarchy and deleting all non-null members is correct.
517-526: Zero-initialized allocation
callocmethod simply callsmallocthen usesmemset. Straightforward approach.
528-549: Free logic with single-octet header
Subtracting one byte to find the chunk header is a known approach. Should be validated that alignment is correct on all platforms.
551-647: Map-like stubs
Same approach of returning-1so the user knows these operations are unsupported.
649-672: Dump method
Dumps internal hierarchy for debugging. The per-allocator dump is also invoked.ACE/ace/Malloc_T.inl (3)
41-47:pool_sum()accessor
Excellent use ofACE_GUARD_RETURNfor concurrency and returning thechunk_sum_safely.
49-53: Mutex accessor
Exposes the class-level lock reference, enabling external locking patterns if needed.
55-59: Lock accessor in multi-size-based allocator
Completes the concurrency model. This matches the approach in the cascaded dynamic allocator.ACE/tests/run_test.lst (1)
19-19: LGTM!The test entry is correctly added and follows the file's format. Since no platform restrictions are specified, the test will run on all platforms, which is appropriate for testing the new allocator classes.
ACE/tests/Allocator_Cascaded_Test.cpp (1)
1-21: LGTM!The file header and includes are well-structured and contain all necessary dependencies for the test implementation.
ACE/tests/tests.mpc (1)
148-154: LGTM!The project configuration is correctly structured and includes all necessary settings:
- Properly avoids ace_for_tao dependency
- Correctly specifies the executable name
- Includes the appropriate source file
| template <class ACE_LOCK> | ||
| class ACE_Cascaded_Dynamic_Cached_Allocator : public ACE_Allocator | ||
| { | ||
| public: | ||
| // Useful STL-style traits. | ||
| using comb_alloc_type = ACE_Dynamic_Cached_Allocator<ACE_Null_Mutex>; | ||
| using comb_alloc_ptr = comb_alloc_type*; | ||
|
|
||
| /// Create a cached memory pool with @a initial_n_chunks chunks | ||
| /// each with @a chunk_size size. | ||
| ACE_Cascaded_Dynamic_Cached_Allocator (size_t initial_n_chunks, size_t chunk_size); | ||
|
|
||
| /// Clear things up. | ||
| ~ACE_Cascaded_Dynamic_Cached_Allocator (); | ||
|
|
||
| /** | ||
| * Get a chunk of memory from free list cache. Note that @a nbytes is | ||
| * only checked to make sure that it's less or equal to @a chunk_size, | ||
| * and is otherwise ignored since malloc() always returns a pointer to an | ||
| * item of @a chunk_size size. | ||
| */ | ||
| virtual void *malloc (size_t nbytes); | ||
|
|
||
| /** | ||
| * Get a chunk of memory from free list cache, giving them | ||
| * @a initial_value. Note that @a nbytes is only checked to make sure | ||
| * that it's less or equal to @a chunk_size, and is otherwise ignored | ||
| * since calloc() always returns a pointer to an item of @a chunk_size. | ||
| */ | ||
| virtual void *calloc (size_t nbytes, char initial_value = '\0'); | ||
|
|
||
| /// This method is a no-op and just returns 0 since the free list | ||
| /// only works with fixed sized entities. | ||
| virtual void *calloc (size_t n_elem, size_t elem_size, char initial_value = '\0'); | ||
|
|
||
| /// Return a chunk of memory back to free list cache. | ||
| virtual void free (void *ptr); | ||
|
|
||
| /// Dump the state of this object. | ||
| virtual void dump () const; | ||
|
|
||
| /// These methods are no-ops. | ||
| virtual int remove (); | ||
| virtual int bind (const char *name, void *pointer, int duplicates = 0); | ||
| virtual int trybind (const char *name, void *&pointer); | ||
| virtual int find (const char *name, void *&pointer); | ||
| virtual int find (const char *name); | ||
| virtual int unbind (const char *name); | ||
| virtual int unbind (const char *name, void *&pointer); | ||
| virtual int sync (ssize_t len = -1, int flags = MS_SYNC); | ||
| virtual int sync (void *addr, size_t len, int flags = MS_SYNC); | ||
| virtual int protect (ssize_t len = -1, int prot = PROT_RDWR); | ||
| virtual int protect (void *addr, size_t len, int prot = PROT_RDWR); | ||
|
|
||
| /// Return the number of chunks available in the hierarchy. | ||
| size_t pool_depth (); | ||
|
|
||
| /// Return the sum of chunks including used and freed in the hierarchy. | ||
| size_t pool_sum (); | ||
|
|
||
| /// Returns a reference to the lock used to provide mutual exclusion to | ||
| /// the allocator hierarchy. | ||
| ACE_LOCK &mutex (); | ||
|
|
||
| private: | ||
| /// Synchronization variable for API. | ||
| ACE_LOCK mutex_; | ||
|
|
||
| /// Remember how we allocate the memory so we can clear things up later. | ||
| std::vector<comb_alloc_ptr> hierarchy_; | ||
|
|
||
| /// Remember the size of initial n_chunks for creating fresh allocator in future. | ||
| const size_t initial_n_chunks_; | ||
|
|
||
| /// Remember the size of our chunks for creating fresh allocator in future. | ||
| const size_t chunk_size_; | ||
|
|
||
| /// Remember the sum of our chunks including used and freed | ||
| size_t chunk_sum_; | ||
| }; |
There was a problem hiding this comment.
💡 Codebase verification
Review Verification on Potential Integer Overflow with Bit Shifting in ACE/ace/Malloc_T.cpp
After reviewing the grep output and inspecting the context of the 1 << size usage in the cascaded allocator in the file ACE/ace/Malloc_T.cpp, the check for potential overflow appears to be localized. In the implementation, the shift operation is used in a calculation to determine the number of chunks by multiplying this->initial_n_chunks_ with (1 << size). There isn’t any explicit bounds check around size in the context we observed.
Since the shift magnitude depends on the value of size, and given that no safeguards or maximum bounds are enforced on it within the bit-shift expression, there is indeed a risk that a large, potentially user-influenced, size value could trigger undefined behavior by causing an integer overflow.
Attention Points:
- Verify that the value of
size(likely derived from the number of allocators in the hierarchy) is either capped or controlled so that it never becomes large enough to trigger an integer overflow. - Consider adding a bounds check before performing
(1 << size)(or using a safe shift utility) to ensure thatsizeremains within a defined, safe range.
🔗 Analysis chain
Potential integer overflow when shifting
The logic in the malloc() method (in the corresponding .cpp file) shifts by size (1 << size). Ensure that size remains small enough and not user-controlled in a way that triggers large shifts. A bounds check with a well-defined maximum would help protect against overflow.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search all references to "1 <<" to see if user inputs can cause large shifts.
rg -A 3 '1 <<'
Length of output: 18264
…to cascadedAllocator from server
merge from cascadedAllocator for coderabbitai's suggestions
ACE_Mallocmemory pool has only one linked list,so when it has a very long list and has various, random chunk size distribution, the cost of its memory management is great.
In our case, the
freeAPI ofACE_Mallochas consumed much CPU time.So I use the
ACE_Cascaded_Dynamic_Cached_Allocatorclass provided in my previous PR to form a allocator hierarchy with various fixed-size achieving the 'infinite' space ability asACE_Mallocby a cost of O(1).Main Design
Use size-based allocator to reduce the cost of malloc/free
Decrease
ACE_Cascaded_Dynamic_Cached_Allocator's initial_n_chunks constructor parameter according to the hierarchy level, butcan be adjusted by a threshold
min_initial_n_chunksparameterIncrease
ACE_Cascaded_Dynamic_Cached_Allocator's chunk_size constructor parameter according to the hierarchy level, it will enable bigger chunks can be mallocedSummary by CodeRabbit
New Features
Tests