Small vector relocation #6468
Small vector relocation #6468
Conversation
Coverage summary from CodacySee diff coverage on Codacy
Coverage variation details
Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: Diff coverage details
Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: See your quality gate settings Change summary preferencesYou may notice some variations in coverage metrics with the latest Coverage engine update. For more details, visit the documentation |
|
@isidorostsa could you please take care of the inspect and clang-format issues? Also please rebase onto master. |
Yes, apologies for not already doing that |
Performance test reportHPX PerformanceComparison
Info
Comparison
Info
Comparison
Info
Explanation of Symbols
|
| static_assert(!emulate_inplace_vector, | ||
| "If called in an inplace_vector, it is a bug."); |
There was a problem hiding this comment.
FWIW, std::inplace_vector allows you to call .reserve. It's a no-op for small capacities, and invariably throws bad_alloc (or length_error? I'm not sure even LWG knows yet) for large capacities. I don't know if you consider that an improvement or not.
| if constexpr (emulate_inplace_vector) | ||
| { | ||
| // Can not have an inplace_vector with a size larger than N | ||
| if (s <= capacity()) |
There was a problem hiding this comment.
Nit: Cannot
Major: Isn't this condition backwards? You mean if (s > capacity()).
| // We assume that the template parameters of "other" are the same | ||
| // as "this". |
There was a problem hiding this comment.
The comment is superfluous: the injected-class-name small_vector on line 549 literally means small_vector<T, MinInlineCapacity, emulate_inplace_vector> because that's how the language works.
| if constexpr (emulate_inplace_vector) | ||
| { | ||
| return capacity<direction::direct>(); | ||
| } |
There was a problem hiding this comment.
This seems redundant, since is_direct() returns constexpr true in that case. Or if you're trying to avoid even instantiating capacity<direction::indirect>() in that case, then it ought to be enclosed in an else block.
| template <typename T, std::size_t N, typename Alloc, bool emul> | ||
| struct is_trivially_relocatable< | ||
| hpx::detail::small_vector<T, N, Alloc, emul>> | ||
| : is_trivially_relocatable<T> |
There was a problem hiding this comment.
And Alloc needs to be trivially relocatable, and Alloc::pointer, and it must have the right POCMA/POCCA/POCS behavior if you care about any of that stuff. See https://github.com/Quuxplusone/llvm-project/blob/trivially-relocatable-v107/libcxx/include/__memory/allocator_traits.h#L421-L433 and https://github.com/Quuxplusone/llvm-project/blob/trivially-relocatable-v107/libcxx/include/vector#L399-L407 for my approach.
There was a problem hiding this comment.
Ah, that is interesting. Will correct and write tests for this. Thank you for the catch!
You are absolutely right to point that out, I will be doing that. |
I agree to this. |
Coverage summary from CodacySee diff coverage on Codacy
Coverage variation details
Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: Diff coverage details
Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: See your quality gate settings Change summary preferencesYou may notice some variations in coverage metrics with the latest Coverage engine update. For more details, visit the documentation |
|
@isidorostsa what's the status of this? Could you rebase this onto master please? |
I was looking at this PR yesterday, I'll split this in two parts, one regarding the inplace_vector aspect and one regarding the trivial relocation aspect. |
hpx::small_vectoris a small buffer optimized vector. This PR introduces two enhancements:Making
small_vectorcompatible with the proposed inplace_vector. This is done with creating astatic_vectortypedef forsmall_vectorthat limits it to only use the direct storage mode, without introducing overhead with that limitation.Utilizing P1144's relocation algorithms to insert and remove elements, as well as relocate the buffer when needed. This abstracts away much of the logic needed of those operations and also optimizes them based on P1144's relocation semantics. A
small_vectoris also marked as trivially relocatable when the type it is carrying is trivially relocatable itself. That is because it is always possible that the element will be stored in the object itself.For a
small_vector<std::vector<T>>the observable speedup of using trivial relocation for insertions, push_backs, and deletions is in the order of 2-3x, benchmarks and tests for this are to be added to this PR.