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

Array destructor #14

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

Conversation

FatihBAKIR
Copy link

@FatihBAKIR FatihBAKIR commented Sep 27, 2016

I've implemented the array destructor storage compaction with support for merging and splitting w.r.t. the comment on deferred_heap.h:77.

Although I've implemented the merging when a new object or array is added just after another array by incrementing the count instead of pushing another array_destructor, deferred_heap never allocates two blocks consecutively (the start block in the gpage), therefore, such a case never happens.

@CLAassistant
Copy link

CLAassistant commented Sep 27, 2016

CLA assistant check
All committers have signed the CLA.

@hsutter
Copy link
Owner

hsutter commented Sep 27, 2016

Thanks, I'll take a look. Note that it should be exercised for vector<T, deferred_allocator<T>>.

@FatihBAKIR
Copy link
Author

@hsutter, the registration of destructors is done in deferred_heap::construct and deferred_heap::construct_array, for single objects and arrays respectively. However, the deferred_allocator can only call the first one since the allocator interface doesn't have a construct method for array construction.

So, right now, deferred_vector (or anything that uses the deferred_allocator cannot benefit from the array_destructor optimization. I think there are 2 options:

  1. Either we can represent the single object destructors as an array_destructor with count 1.
  2. Since deferred_heap can track whether an object at an address was allocated in an array allocation or a single object allocation, it can give a hint to destructors::store so that we can try to compact 2 single object destructors to an array destructor.

I think option 1 would be space and time inefficient since we'll have a lot more array_destructors to search through to do compaction and 2 redundant size_ts per single object.

@hsutter
Copy link
Owner

hsutter commented Sep 30, 2016

@FatihBAKIR Actually it is beneficial, in the intended use case is vector<T, deferred_allocator<T>>. In that use, the vector will get memory for N objects and then individually construct them in-place during push_backs (and similar) without any padding between the T objects. That's the case where this would be useful and we could simply bump the count of consecutive objects when the vector performs a push_back, which helps keep a push_back loop at O(N) instead of turning it into O(N^2) as it would be now by storing individual destructors.

@hsutter hsutter added this to the Future - Backlog milestone Nov 27, 2016
@hsutter
Copy link
Owner

hsutter commented Nov 27, 2016

Thanks again for this PR. For now I'm waiting for feedback and bug reports from actual use of the library, and deferring enhancements and optimizations until then but keeping them in the backlog.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants