[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 accepted this revision as: var-const.
var-const added a comment.

In D147741#4346199 <https://reviews.llvm.org/D147741#4346199>, @hiraditya wrote:

> In D147741#4344506 <https://reviews.llvm.org/D147741#4344506>, @var-const wrote:
>
>> @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.
>
> Initially i thought there should be an impact but there isn't because compiler does the right thing even as is. I can remove the test for copy assignment if that makes more sense?

I wouldn't remove the test -- just wanted to make sure we understand why there's a difference. I'm a little surprised the compiler could optimize the assignment case but not the construction case, but if that's what the assembly shows, then I don't think we need to dig any further.



================
Comment at: libcxx/benchmarks/ContainerBenchmarks.h:42
+  auto size = st.range(0);
+  Container c1(size);
+  Container c2(size);
----------------
hiraditya wrote:
> var-const wrote:
> > Is it deliberate that `c1` and `c2` are the same size (even though `c1` gets overwritten)? If yes, please add a comment.
> Yeah, there was no reason to have `c2` as the same size. Updated the test.
Hmm, I expected that `c1` will be empty, not `c2` (since `c2` is the one that provides the final value). Or does the optimization only apply if the existing capacity can be reused? If so, then I think the previous state with both `c1` and `c2` being large was better.


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

https://reviews.llvm.org/D147741