[libcxx-commits] [PATCH] D121485: [libc++] Fix performance inconsistency between map copy-assignment and copy-constructor

[Eric Fiselier via Phabricator via libcxx-commits]

EricWF added a comment.

I don't think a custom benchmarking test is appropriate. Please rewrite your benchmarks using Google benchmark.



================
Comment at: libcxx/benchmarks/map.bench.cpp:175
+  void run(benchmark::State& State) const {
+    auto Data = makeTestingSets(MapSize, Mode::Hit, Shuffle::None, 1);
+    auto& Map = Data.Maps.front();
----------------
var-const wrote:
> Question: why are we using a different naming convention here (with capitalized variable `Name`s)?
Probably because I started it because I was feeling funky. Not sure it matters much.


================
Comment at: libcxx/benchmarks/map.bench.cpp:177
+    auto& Map = Data.Maps.front();
+    while (State.KeepRunningBatch(MapSize)) {
+#ifndef VALIDATE
----------------
I don't think this is the correct usage of `KeepRunningBatch`. 


================
Comment at: libcxx/benchmarks/map.bench.cpp:179
+#ifndef VALIDATE
+      std::map<uint64_t, int64_t> M;
+      M = Map;
----------------
What about the case where the map is already populated? And with different sizes?


================
Comment at: libcxx/include/__tree:1098
     __tree(const __tree& __t);
-    __tree& operator=(const __tree& __t);
     template <class _ForwardIterator>
         void __assign_unique(_ForwardIterator __first, _ForwardIterator __last);
----------------
Mordante wrote:
> var-const wrote:
> > From the patch description: "In the future, we could instead reintroduce a copy assignment operator in
> > tree and copy the whole tree without performing any comparison". Would it be worthwhile adding a TODO to that effect here?
> +1 for the TODO.
On another note: a copy assignment operator gets implicitly declared by the compiler. For readability lets make that implicit declaration explicit. 


================
Comment at: libcxx/include/__tree:1620
-        __copy_assign_alloc(__t);
-        __assign_multi(__t.begin(), __t.end());
-    }
----------------
Could we make this faster for non-multi containers by using a different assign f ml


================
Comment at: libcxx/include/set:598
+                __tree_.__copy_assign_alloc(__s.__tree_);
+                insert(__s.begin(), __s.end());
+            }
----------------
What's the performance like if you call `__assign_unique(__s.begin(), __s.end())` instead?


================
Comment at: libcxx/test/libcxx/containers/associative/perf.assignment_vs_construction.pass.cpp:3
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
----------------
var-const wrote:
> A couple of questions:
> - can/should we use Google Benchmark here?
> - is this file compiled with optimizations enabled?
Yes, this should be google benchmark.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121485