P1144 Relocation primitives #6324
Conversation
isidorostsa
requested review from
aurianer,
msimberg,
biddisco and
hkaiser
as code owners
|
TODO: decide the namespace of the primitives, maybe change the name of the files to reflect that they are primitives. |
Performance test reportHPX PerformanceComparison
Info
Comparison
Info
Comparison
Info
Explanation of Symbols
|
Performance test reportHPX PerformanceComparison
Info
Comparison
Info
Comparison
Info
Explanation of Symbols
|
|
@isidorostsa could you please take care of the inspect problems reported: |
Oh sorry I forgot to push this, thanks for the nudge. |
libs/core/type_support/include/hpx/type_support/is_relocatable.hpp
Outdated
Show resolved
Hide resolved
libs/core/type_support/include/hpx/type_support/is_trivially_relocatable.hpp
Outdated
Show resolved
Hide resolved
libs/core/type_support/include/hpx/type_support/is_trivially_relocatable.hpp
Outdated
Show resolved
Hide resolved
libs/core/type_support/include/hpx/type_support/relocate_at.hpp
Outdated
Show resolved
Hide resolved
libs/core/type_support/include/hpx/type_support/relocate_at.hpp
Outdated
Show resolved
Hide resolved
libs/core/type_support/include/hpx/type_support/uninitialized_relocate.hpp
Show resolved
Hide resolved
| #if __cplusplus <= 201703L | ||
| // pre c++17 std::destroy_at can be used only on non-array types | ||
| template <typename T, | ||
| HPX_CONCEPT_REQUIRES_(std::is_destructible_v<T> && !std::is_array_v<T>)> | ||
| #else | ||
| // c++17 std::destroy_at can be used on array types, destructing each element | ||
| template <typename T, HPX_CONCEPT_REQUIRES_(std::is_destructible_v<T>)> | ||
| #endif |
There was a problem hiding this comment.
I'm confused by this. You're unconditionally using destroy_guard inside relocate, and unconditionally using destroy_at inside destroy_guard... so don't these comments simply indicate that you have a bug in C++17 when relocate is used on array types? ...Ah, except that relocate already can't be used on arrays, because is_constructible_v<int[4], int[4]> is false. So all this ifdef stuff is simply unnecessary. You can replace lines 22–29 with simply
template<typename T>
There was a problem hiding this comment.
The idea was to make destroy_guard usable outside relocation too. The checks done here are not to prevent invalid relocation, but invalid use of this facillity.
There was a problem hiding this comment.
Okay. Personally I'd still say "You Ain't Gonna Need It"; and if you think you will need a bulletproof reusable destroy_guard then I'd say you should be investing more in it — it should be in its own file instead of relocate_at.hpp, and named hpx::destroy_guard instead of hpx::detail::destroy_guard, and you should make it work for arrays in C++17 instead of giving it different behavior depending on the C++ version. But that all sounds tangential to my interest in this PR, at least.
There was a problem hiding this comment.
Yea, I do agree with that. We are almost certainly not going to use this elsewhere.
I do however have to leave the HPX_CONCEPT_REQUIRES etc there for the address sanitizer's warnings.
Thanks for your interest in enforcing best practices, I appreciate it :)
There was a problem hiding this comment.
I do however have to leave the
HPX_CONCEPT_REQUIRESetc there for the address sanitizer's warnings.
quizzical look I don't understand why a requires-clause/enable_if/etc would have anything to do with address sanitizer. Or even (assuming you misspoke) what it would have to do with static analysis. This is again a tangent and non-blocking, but I'm curious if you could paste the warning/error that you get by omitting the HPX_CONCEPT_REQUIRES?
There was a problem hiding this comment.
It was a verbal warning to me by a colleague :P . I will see if I can find out more and get back to you!
There was a problem hiding this comment.
@Quuxplusone Doesn't it making it safer constraining the types that can be destroyed/guarded from the destroy guard? Def not a strong opinion of mine...
There was a problem hiding this comment.
@gonidelis: "Doesn't it make it safer [to be a constrained template]...?" — Here's my opinion: I'd say no. In fact, I'd say constraints make the average function template less "safe" in general, because if you think about what would happen with a caller like this...
template<class T> /* requires (!is_array_v<T>) */
void callee(T *t) { std::destroy_at(t); }
void test(MyThing *t) {
callee(t);
}
Without the constraint, obviously we call this callee and get a hard error immediately if the instantiation doesn't pan out (e.g. if MyThing happens to be an array type, or void, or whatever). With the constraint, if MyThing happens to be an array type, then this callee quietly drops out of overload resolution; so either we call some completely different (worse-matching but still viable) overload of callee, or we get an error about "no viable candidate found," which is more difficult to debug. Without the constraint, we simply get a hard error directly on the line with the destroy_at (or even better, down inside destroy_at where we'd see it trying to do t->~T()).
Now, in the case of a class template like destroy_guard, we don't have that exact problem, because class templates can't be overloaded. Constraining a class template doesn't make the failure story any worse, AFAIK. But it doesn't make it any better, either. When we have two roughly equivalent options, equally usable, we should prefer the simpler one (fewer advanced features, fewer lines of code); in this case that means choosing the unconstrained class template destroy_guard.
Alternative argument: C++20 constraints have exactly one[*] purpose: to mess with overload resolution. If you're not intending to mess with overload resolution, you shouldn't use constraints. (If you mean static_assert, okay; but then you should say static_assert, not requires.)
[* — well, actually at least two purposes: constraint syntax can also be used to define partial specializations]
|
@isidorostsa Could you please rebase this once #6332 has been merged? That should resolve the build system issues you are seeing. |
Yes, of course, thank you |
… into relocate_primitives
isidorostsa
force-pushed
the
relocate_primitives
branch
from
f1b8090 to
ea53ac9
Compare
Performance test reportHPX PerformanceComparison
Info
Comparison
Info
Comparison
Info
Explanation of Symbols
|
|
bors merge |
|
Build succeeded! The publicly hosted instance of bors-ng is deprecated and will go away soon. If you want to self-host your own instance, instructions are here. If you want to switch to GitHub's built-in merge queue, visit their help page. |
This is a starting point for the implementation of the P1144 relocate_at and uninitialized_relocate algorithms for HPX. They will later be used as base primitives for parallelized and hpx-styled versions.