Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cascaded allocator #2113

Open
wants to merge 46 commits into
base: master
Choose a base branch
from
Open

Conversation

smithAchang
Copy link

ACE has fixed-size ACE*_Cached_Allocator, but in some case the total n_chunks can't be determined easyly :(

If the ACE framework has the ability to malloc fixed-size chunks 'infinitely' just like ACE_Malloc< ACE_MEM_POOL_1, ACE_LOCK > for mallocing various-size chunks, that will be very useful!

design

  • choose ACE_Dynamic_Cached_Allocator not ACE_Cached_Allocator

ACE_Cached_Allocator provides poor enhancement for the API

  • combinate ACE_Vector<T> to enable the flexibility of allocator hierarchy
  • all allocators forms a hierarchy with the allocator in lower level has 2 * n_chunks(upper level) capacity

@jwillemsen jwillemsen added the needs review Needs to be reviewed label Sep 4, 2023
@jwillemsen
Copy link
Member

Only read your description, please use std::vector, ACE_Vector is old and we don't want to use that anymore

ACE/ace/Malloc_T.cpp Outdated Show resolved Hide resolved
ACE/ace/Malloc_T.cpp Outdated Show resolved Hide resolved
ACE/ace/Malloc_T.h Show resolved Hide resolved
ACE/tests/Allocator_Cascaded_Test.cpp Show resolved Hide resolved
ACE/tests/Allocator_Cascaded_Test.cpp Outdated Show resolved Hide resolved
ACE/ace/Malloc_T.h Outdated Show resolved Hide resolved
ACE/ace/Malloc_T.cpp Outdated Show resolved Hide resolved
ACE/ace/Malloc_T.h Outdated Show resolved Hide resolved
@jwillemsen jwillemsen removed their request for review September 6, 2023 05:53
@smithAchang
Copy link
Author

I have fixed these questions,

Have I lefted any unfinished review issues ?

Copy link
Author

@smithAchang smithAchang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

@jwillemsen jwillemsen removed their request for review September 6, 2023 11:30
@jwillemsen
Copy link
Member

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

@smithAchang
Copy link
Author

ok, thx :)


private:
// Useful STL-style traits.
using comb_alloc_type = ACE_Dynamic_Cached_Allocator<ACE_Null_Mutex>;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be ACE_LOCK?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 test function name
this->hierarchy_.push_back(tmp);
if (0 == this->hierarchy_.size())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

empty?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when push_back fails it will throw, maybe use std::unique_ptr?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These APIs have strong exception safety guarantee, so there is no need to use std::unique_ptr.

Is it true ?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By the way, I can see the result of these calls to judge whether potential exceptions have occurred

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when using unique_ptr the check after the push_back seems invalid

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, Your idea is right!

The APIs provide strong exception guarantee, but not Nothrow

I will delete the defence codes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement needs review Needs to be reviewed
Development

Successfully merging this pull request may close these issues.

None yet

2 participants