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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added inline comments.


================
Comment at: libcxx/benchmarks/map.bench.cpp:121
+      State.PauseTiming();
+      Data = makeTestingSets(MapSize, Mode::Hit, Shuffle::None, 1000);
+      State.ResumeTiming();
----------------
Why regenerating the data?


================
Comment at: libcxx/benchmarks/map.bench.cpp:124
-        State.SkipWithError("Map copy not identical");
-#endif
     }
----------------
Why remove the validation?


================
Comment at: libcxx/include/__tree:1642
+    for (const_iterator __e = end(); __first != __last; ++__first)
+        __insert_unique(__e, *__first);
 }
----------------
I really wished somebody would have named this function `__insert_unique_hint`.


================
Comment at: libcxx/include/map:1902
             if (this != _VSTD::addressof(__m)) {
-                __tree_.clear();
                 __tree_.value_comp() = __m.__tree_.value_comp();
----------------
Actually I think this is needed.
- we allocated nodes with allocator `foo`
- we change the allocator to `bar` in `__tree_.__copy_assign_alloc(__m.__tree_);`
- the `__tree_.__assign_multi` will remove the exiting nodes using allocator `bar` instead of `foo`

Am I missing something?

When I'm correct please fix the other 3 assignments too. And then we probably need tests too.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121485