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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added a comment.

Nice that this issue has been found!



================
Comment at: libcxx/include/__tree:1098
     __tree(const __tree& __t);
-    __tree& operator=(const __tree& __t);
     template <class _ForwardIterator>
----------------
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.


================
Comment at: libcxx/include/set:594
         {
-            __tree_ = __s.__tree_;
+            if (this != _VSTD::addressof(__s)) {
+                __tree_.clear();
----------------
var-const wrote:
> Question: it's `_VSTD` and not `std` for consistency within the file, right?
These changes are quite unexpected when reading the patch description. Can you update the description?
(Same for the multi(map|set).)


================
Comment at: libcxx/test/libcxx/containers/associative/perf.assignment_vs_construction.pass.cpp:10
+// This test ensures that copy assignment and copy construction are within a reasonable
+// margin from each other performance wise.
+
----------------
When this can't be done in Google Benchmark please add the rationale here.


================
Comment at: libcxx/test/libcxx/containers/associative/perf.assignment_vs_construction.pass.cpp:71
+    auto percentage = (std::labs((ssize_t)ctor_avg - (ssize_t)assignment_avg) / (double)ctor_avg) * 100.0;
+    assert(percentage < 10.0 && "Copy assignment and copy constructor have more than 10% difference");
+}
----------------
When this is the reason to not do it in Google Benchmark I think it's worthwhile to consider how we can test for timing regressions in the benchmarks. (This would even be good to look into regardless of this patch.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121485