P1144 #1
P1144 #1
Conversation
…vs no relocation.
include/parlay/alloc.h
Outdated
| @@ -154,7 +154,7 @@ inline void p_free(void* ptr) { | |||
| // std::vector<int, parlay::allocator<int>> | |||
| // | |||
| template <typename T> | |||
| struct allocator { | |||
| struct PARLAY_TRIVIALLY_RELOCATABLE allocator { | |||
There was a problem hiding this comment.
This annotation isn't needed, AFAICT. allocator is a Rule-of-Zero type with no data members. (Its templated constructor doesn't matter because it still has an implicitly defaulted copy ctor and move ctor.)
include/parlay/type_traits.h
Outdated
| // Container implementations can differ widely by vendor, so we don't want to specialize these | ||
| // with a broad brush. Instead, only specialize them for stdlibs that we are confident about. |
There was a problem hiding this comment.
FYI, https://quuxplusone.github.io/blog/2019/02/20/p1144-what-types-are-relocatable/ — although I'd encourage you to do less of this warranting-of-types-you-don't-own, because this is exactly how Folly got in trouble. 😛 Of course it makes a difference for performance (iff you're not using a P1144-enabled STL implementation to begin with). The good news is that you've guarded this all under PARLAY_MUST_SPECIALIZE_IS_TRIVIALLY_RELOCATABLE, so if there are any bugs, my P1144 implemention will fix them rather than cause them. ;)
include/parlay/type_traits.h
Outdated
| template<typename T, typename Deleter> | ||
| PARLAY_ASSUME_TRIVIALLY_RELOCATABLE_IF((is_trivially_relocatable_v<Deleter>), std::unique_ptr<T, Deleter>); |
There was a problem hiding this comment.
Technically you also need to check is_trivially_relocatable_v<Deleter::pointer>; or just drop the Deleter arg the way you dropped the allocator from all the STL containers below.
include/parlay/type_traits.h
Outdated
| template<typename T> | ||
| PARLAY_ASSUME_TRIVIALLY_RELOCATABLE(std::optional<T>); |
There was a problem hiding this comment.
This is wrong; you need IF(is_trivially_relocatable_v<T>).
include/parlay/utilities.h
Outdated
| if constexpr (!std::is_trivially_default_constructible_v<T>) { | ||
| uninitialized<T> holder; | ||
| std::memcpy(&holder.value, src, sizeof(T)); | ||
| return holder.value; |
There was a problem hiding this comment.
This seems like overengineering. IIUC the whole point of this bits_to_object is you're trying to avoid the "move and destroy" codepath in relocate below. But return holder.value won't copy-elide, so you'll get a move anyway. In fact you'll get a copy anyway, won't you?!
So unless I'm mistaken, this is a pessimization and also must be lacking test coverage for unique_ptr or any other trivially-relocatable-but-non-copyable type.
| using reference = std::add_lvalue_reference_t<value_type>; | ||
| using pointer = std::add_pointer_t<value_type>; | ||
|
|
||
| uninitialized_iterator_adapter(Iterator it_) : it(it_) {} |
|
|
||
| reference operator*() const { return it->value; } | ||
|
|
||
| pointer operator->() { return std::addressof(it->value); } |
| // | ||
| // The resulting iterator will have the same iterator category as Iterator. | ||
| template<typename Iterator> | ||
| class uninitialized_iterator_adapter { |
There was a problem hiding this comment.
nit: ISO (and me) spell it adaptor, although the dictionary accepts both ways.
https://en.cppreference.com/w/cpp/experimental/resource_adaptor
| @@ -219,7 +219,7 @@ sequence<size_t> integer_sort_r(slice<InIterator, InIterator> In, | |||
| // uninitialized_relocate_n, which can memcpy multiple elements at a time | |||
| // to save on performing every copy individually. | |||
| if constexpr (std::is_same_v<assignment_tag, uninitialized_relocate_tag>) { | |||
| uninitialized_relocate_n(Out.begin(), In.begin(), Out.size()); | |||
| parlay::uninitialized_relocate(In.begin(), In.end(), Out.begin()); | |||
There was a problem hiding this comment.
Throughout: I guess In.size() == Out.size()? which is also known as n? You switch from Out.size() on the left to In.end() on the right, so they better be the same size.
No description provided.