Cascaded allocator #2113
Cascaded allocator #2113
Conversation
|
Only read your description, please use std::vector, ACE_Vector is old and we don't want to use that anymore |
2. using instead of typedef
|
I have fixed these questions, Have I lefted any unfinished review issues ? |
I am busy with other things, review time on github is very limited, you will have to wait until me or someone else has time to do another review, I only did a high level review |
|
ok, thx :) |
ACE/ace/Malloc_T.h
Outdated
|
|
||
| private: | ||
| // Useful STL-style traits. | ||
| using comb_alloc_type = ACE_Dynamic_Cached_Allocator<ACE_Null_Mutex>; |
There was a problem hiding this comment.
there is no need because the nested allocator will be protected by container class api
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
2、fix some code style
2、fix test function name
ACE/ace/Malloc_T.cpp
Outdated
| this->hierarchy_.push_back(tmp); | ||
| if (0 == this->hierarchy_.size()) |
There was a problem hiding this comment.
when push_back fails it will throw, maybe use std::unique_ptr?
There was a problem hiding this comment.
These APIs have strong exception safety guarantee, so there is no need to use std::unique_ptr.
Is it true ?
There was a problem hiding this comment.
By the way, I can see the result of these calls to judge whether potential exceptions have occurred
There was a problem hiding this comment.
See https://en.cppreference.com/w/cpp/language/exceptions, push_back can throw, when it does the state of the std::vector is guaranteed at that moment, but it will not just return. The exception should go back to the caller to my idea, your allocator should not leak when push_back (or any operation it uses) throws
There was a problem hiding this comment.
when using unique_ptr the check after the push_back seems invalid
There was a problem hiding this comment.
I have used std::unique_ptr for idiom in new commit
But for a vector of pointer, I think sdt::vector can keep strong exception guarantee.
If an exception is thrown (which can be due to Allocator::allocate() or element copy/move constructor/assignment), this > function has no effect ([strong exception guarantee]> (https://en.cppreference.com/w/cpp/language/exceptions#Exception_safety)).
Strong exception guarantee — If the function throws an exception, the state of the program is rolled back to the state > just before the function call (for example, [std::vector::push_back]>(https://en.cppreference.com/w/cpp/container/vector/push_back)).
There was a problem hiding this comment.
Sorry, Your idea is right!
The APIs provide strong exception guarantee, but not Nothrow
I will delete the defence codes
…ly the first and last allocator has free chunks
WalkthroughThis pull request introduces a new subproject commit along with a new cascaded dynamic cached allocator. The changes add a templated allocator class with methods for memory allocation (malloc, calloc, free) and pool management (dump, pool_depth, pool_sum, mutex) across new source, header, and inline files. Additionally, test files and configuration entries have been added to verify the allocator’s functionality within the project. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User/Application
participant A as Cascaded Allocator
participant CA as Child Allocator
U->>A: Request allocation (malloc/calloc)
alt Allocation fits in first child allocator
A->>CA: Allocate memory
else Fallback path
A->>CA: Attempt allocation from last child or create new child
end
CA-->>A: Return allocated pointer
A-->>U: Return memory pointer
Poem
✨ Finishing Touches
🪧 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: 2
🧹 Nitpick comments (3)
ACE/ace/Malloc_T.cpp (1)
181-311: Validate expanded allocator logic and concurrency approach.
- The constructor defers allocation failures by reconstructing the hierarchy later if needed; verify whether this lazy handling of
ACE_NEWfailures is acceptable.- The
mallocmethod tries the “first” and “last” allocators, then expands. This is efficient under normal usage, but consider exposing a configuration to limit the maximum number of cascaded allocators to avoid runaway expansions.- Concurrency:
- Properly guarded by
ACE_GUARD, ensuring safe multi-thread usage.- The
freemethod routes all freed chunks to the first allocator, which is a valid design but could cause fragmentation in heavily threaded scenarios.
Overall, design is logically consistent with the PR objectives.ACE/tests/Allocator_Cascaded_Test.cpp (2)
110-111: Fix misleading comment.The comment suggests "three" chunks but the actual count is the sum of initial and cascaded chunks.
- " cascaded pool depth must be three after having freed all malloc ptrs\n"); + " cascaded pool depth must equal total chunks (initial + cascaded) after freeing all malloc ptrs\n");
125-126: Enhance error message clarity.The error message could be more descriptive about the expected memory content.
- " calloc call must set the memory content as expected\n"); + " calloc must initialize memory with the specified initial value\n");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
ACE/MPC(1 hunks)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)
✅ Files skipped from review due to trivial changes (1)
- ACE/MPC
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (11)
ACE/ace/Malloc_T.h (4)
8-9: No issues noted with updated authors.
19-19: Confirm concurrency handling with Null_Mutex.
UsingNull_Mutexmeans there is effectively no locking in this allocator. Ensure higher-level synchronization (if required) is guaranteed by the container class or the calling code to prevent race conditions when multiple threads access this allocator hierarchy.
29-29: Usage of std::vector is aligned with modern C++ best practices.
210-311: Consider overflow checks and test coverage for expanding hierarchy.
- The cascade logic creates new allocators with size
initial_n_chunks_ * (1 << hierarchy_.size()). In extreme cases, this could overflow or lead to memory exhaustion. Consider adding upper-bound checks to protect against unexpectedly large expansions.- Ensure thorough tests covering multi-level expansions and associated memory usage.
- The rest of the design, including internal locking and usage of modern containers, looks good.
ACE/ace/Malloc_T.cpp (1)
17-17: Including<memory>for smart pointers is a good improvement.ACE/ace/Malloc_T.inl (1)
41-53: Potential overflow inpool_sum()and external lock usage.
pool_sum()returns the cumulative chunk count but does not guard against potential overflow if the hierarchy grows very large. Consider adding an upper bound or a warning.- Exposing
mutex()allows external lock manipulation, which can be flexible but might lead to unpredictable locking orders when used incorrectly. Validate or document expectations for external code.Do you want a follow-up patch to add overflow checks or to document safe usage patterns for this lock accessor?
ACE/tests/Allocator_Cascaded_Test.cpp (3)
1-2: Remove duplicate empty lines.
20-28: LGTM!The error handling macro is well-structured and correctly uses ACE's logging facilities.
157-170: LGTM!The main function correctly follows ACE's test framework conventions.
ACE/tests/run_test.lst (1)
19-19: LGTM!The test entry is correctly added to the test list.
ACE/tests/tests.mpc (1)
148-154: LGTM!The project definition is well-structured and follows the file's conventions.
| ACE_TEST_EXCEPTION_RETURN (ptr == nullptr, | ||
| " pool must return valid ptr for calloc call with normal nbytes\n"); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix incorrect error message.
The error message mentions "calloc" but the test is for "malloc".
- " pool must return valid ptr for calloc call with normal nbytes\n");
+ " pool must return valid ptr for malloc call with normal nbytes\n");📝 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.
| ACE_TEST_EXCEPTION_RETURN (ptr == nullptr, | |
| " pool must return valid ptr for calloc call with normal nbytes\n"); | |
| ACE_TEST_EXCEPTION_RETURN (ptr == nullptr, | |
| " pool must return valid ptr for malloc call with normal nbytes\n"); |
| // ============================================================================ | ||
| // ============================================================================ | ||
| // | ||
| // = LIBRARY | ||
| // tests | ||
| // | ||
| // = DESCRIPTION | ||
| // This program tests the basic APIs supported in | ||
| // <ACE_Cascaded_Dynamic_Cached_Allocator>, and demonstrates how to use it. | ||
| // | ||
| // = AUTHOR | ||
| // Smith Achang <[email protected]> | ||
| // | ||
| // ============================================================================ | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add copyright notice.
The file is missing the standard ACE copyright notice.
Add the following copyright notice at the beginning of the file:
+// -*- C++ -*-
+
+// ============================================================================
+//
+// = LIBRARY
+// tests
+//
+// = FILENAME
+// Allocator_Cascaded_Test.cpp
+//
+// = DESCRIPTION
+// This program tests the basic APIs supported in
+// <ACE_Cascaded_Dynamic_Cached_Allocator>, and demonstrates how to use it.
+//
+// = AUTHOR
+// Smith Achang <[email protected]>
+//
+// Copyright (c) 2025 CodeRabbit Inc.
+// All rights reserved.
+//
+// ============================================================================📝 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.
| // ============================================================================ | |
| // ============================================================================ | |
| // | |
| // = LIBRARY | |
| // tests | |
| // | |
| // = DESCRIPTION | |
| // This program tests the basic APIs supported in | |
| // <ACE_Cascaded_Dynamic_Cached_Allocator>, and demonstrates how to use it. | |
| // | |
| // = AUTHOR | |
| // Smith Achang <[email protected]> | |
| // | |
| // ============================================================================ | |
| // -*- C++ -*- | |
| // ============================================================================ | |
| // | |
| // = LIBRARY | |
| // tests | |
| // | |
| // = FILENAME | |
| // Allocator_Cascaded_Test.cpp | |
| // | |
| // = DESCRIPTION | |
| // This program tests the basic APIs supported in | |
| // <ACE_Cascaded_Dynamic_Cached_Allocator>, and demonstrates how to use it. | |
| // | |
| // = AUTHOR | |
| // Smith Achang <[email protected]> | |
| // | |
| // Copyright (c) 2025 CodeRabbit Inc. | |
| // All rights reserved. | |
| // | |
| // ============================================================================ | |
| // ============================================================================ | |
| // ============================================================================ | |
| // | |
| // = LIBRARY | |
| // tests | |
| // | |
| // = DESCRIPTION | |
| // This program tests the basic APIs supported in | |
| // <ACE_Cascaded_Dynamic_Cached_Allocator>, and demonstrates how to use it. | |
| // | |
| // = AUTHOR | |
| // Smith Achang <[email protected]> | |
| // | |
| // ============================================================================ |
…to cascadedAllocator from server
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
ACE/tests/Allocator_Cascaded_Test.cpp (2)
1-16:⚠️ Potential issueAdd standard ACE copyright notice and fix header style.
The file header needs to follow ACE's standard format and include the copyright notice.
Add the copyright notice and fix the header style:
- -//============================================================================ -/** - * @file Allocator_Cascaded_Test.cpp - * - * This program tests the basic APIs supported in <ACE_Cascaded_Dynamic_Cached_Allocator> and demonstrates how to use it. - * - * @author Smith Achang <[email protected]> - */ -//============================================================================ +// -*- C++ -*- + +// ============================================================================ +// +// = LIBRARY +// tests +// +// = FILENAME +// Allocator_Cascaded_Test.cpp +// +// = DESCRIPTION +// This program tests the basic APIs supported in +// <ACE_Cascaded_Dynamic_Cached_Allocator>, and demonstrates how to use it. +// +// = AUTHOR +// Smith Achang <[email protected]> +// +// Copyright (c) 2025 CodeRabbit Inc. +// All rights reserved. +// +// ============================================================================
69-70:⚠️ Potential issueFix incorrect error message.
The error message mentions "malloc" but the test is for "calloc".
- " pool must return valid ptr for malloc call with normal nbytes\n"); + " pool must return valid ptr for calloc call with normal nbytes\n");
🧹 Nitpick comments (2)
ACE/tests/Allocator_Cascaded_Test.cpp (2)
112-112: Define magic numbers as named constants.The magic number
8 * 1024should be defined as a named constant for better maintainability.+ static const size_t TOTAL_ALLOC_COUNT = 8 * 1024; - size_t const totalAllocSum = 8 * 1024;
27-152: Add test cases for flexible chunk handling.Given that the PR's objective is to handle fixed-size chunks more flexibly, consider adding test cases that verify:
- Dynamic growth behavior when
n_chunksis not predetermined- Memory fragmentation scenarios
- Performance comparison with
ACE_Cached_AllocatorWould you like me to generate additional test cases to cover these scenarios?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
ACE/tests/Allocator_Cascaded_Test.cpp(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (2)
ACE/tests/Allocator_Cascaded_Test.cpp (2)
17-25: LGTM! Well-structured error handling macro.The macro follows good practices with proper do-while(0) pattern and consistent error handling style.
154-167: LGTM! Main function follows ACE test framework conventions.The implementation correctly uses ACE's test framework macros and handles the return value properly.
| size_t pool_sum, old_pool_sum, pool_depth, old_pool_depth; | ||
| char initial_value = '\0'; | ||
|
|
||
| ACE_Cascaded_Dynamic_Cached_Allocator<ACE_SYNCH_MUTEX> alloc (initial_n_chunks, chunk_size); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add thread safety tests.
The allocator uses ACE_SYNCH_MUTEX but there are no tests verifying thread safety. Consider adding concurrent allocation tests.
Would you like me to generate a concurrent test implementation to verify thread safety?
ACEhas fixed-sizeACE*_Cached_Allocator, but in some case the totaln_chunkscan't be determined easyly :(If the
ACEframework has the ability to malloc fixed-size chunks 'infinitely' just likeACE_Malloc< ACE_MEM_POOL_1, ACE_LOCK >for mallocing various-size chunks, that will be very useful!design
ACE_Dynamic_Cached_AllocatornotACE_Cached_AllocatorACE_Vector<T>to enable the flexibility of allocator hierarchy2 * n_chunks(upper level)capacitySummary by CodeRabbit
New Features
Tests