[libcxx-commits] [PATCH] D147741: [libc++, std::vector] call the optimized version of __uninitialized_allocator_copy for trivial types

[Konstantin Varlamov via Phabricator via libcxx-commits]

var-const requested changes to this revision.
var-const added a comment.
This revision now requires changes to proceed.

@hiraditya The benchmark shows a significant speedup for copy construction but almost no speedup for copy assignment. Is the latter expected? If not, it might indicate an issue with the optimization or, more likely, with the benchmark.



================
Comment at: libcxx/benchmarks/ContainerBenchmarks.h:42
+  auto size = st.range(0);
+  Container c1(size);
+  Container c2(size);
----------------
Is it deliberate that `c1` and `c2` are the same size (even though `c1` gets overwritten)? If yes, please add a comment.


================
Comment at: libcxx/include/__memory/uninitialized_algorithms.h:579
+_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 _Up*
+__uninitialized_allocator_copy_impl(_Alloc&, _Type* __first1, _Type* __last1, _Up* __first2) {
   // TODO: Remove the const_cast once we drop support for std::allocator<T const>
----------------
Nit: this name looks inconsistent with `_Type`. One option is to rename `_Type` to `_Tp`, but I'd suggest renaming `_Up` to `_Out` or similar (and perhaps `_Type` to `_In` for symmetry).


================
Comment at: libcxx/include/__memory/uninitialized_algorithms.h:596
+    auto __unwrapped_range = std::__unwrap_range(__first1, __last1);
+    return std::__rewrap_iter(__first2, std::__uninitialized_allocator_copy_impl(__alloc, __unwrapped_range.first, __unwrapped_range.second, std::__unwrap_iter(__first2)));
+}
----------------
This line is too long, but more importantly, I think this expression is a little hard to read because of so many nested subexpressions. How about splitting it into something like:
```
auto __unwrapped_out = std::__unwrap_iter(__first2);
auto __result = std::__uninitialized_allocator_copy_impl(__alloc, __unwrapped_range.first, __unwrapped_range.second, __unwrapped_out);
return std::__rewrap_iter(__first2, __result);
```
?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D147741/new/

https://reviews.llvm.org/D147741