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

[Konstantin Varlamov via Phabricator via libcxx-commits]

var-const added inline comments.


================
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();
----------------
Question: why are we using a different naming convention here (with capitalized variable `Name`s)?


================
Comment at: libcxx/benchmarks/map.bench.cpp:1032
   makeCartesianProductBenchmark<ConstructorMove>(MapSize);
+  makeCartesianProductBenchmark<CopyAssignment>(MapSize);
 
----------------
Question: is a similar benchmark for `set` needed? And perhaps `multimap`, unless it's covered by this test?


================
Comment at: libcxx/include/set:594
         {
-            __tree_ = __s.__tree_;
+            if (this != _VSTD::addressof(__s)) {
+                __tree_.clear();
----------------
Question: it's `_VSTD` and not `std` for consistency within the file, right?


================
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.
----------------
A couple of questions:
- can/should we use Google Benchmark here?
- is this file compiled with optimizations enabled?


================
Comment at: libcxx/test/libcxx/containers/associative/perf.assignment_vs_construction.pass.cpp:21
+
+std::vector<std::string> generate_keys(int N) {
+    auto generator = std::default_random_engine(12345);
----------------
Nit: include `<vector>`.


================
Comment at: libcxx/test/libcxx/containers/associative/perf.assignment_vs_construction.pass.cpp:27
+    for (int i = 0; i != N; ++i) {
+        auto key = std::to_string(generator());
+        keys.push_back(key);
----------------
If I understand correctly, all the generated strings will be short enough for SSO to kick in. Is this desirable (I presume it effectively turns copies into moves and minimizes the time difference between the old and new implementations, thus testing for the "worst" case scenario)? If yes, it might be worth adding a comment to that effect.

Also, now that I think of it, what's the reason to use strings and not integers as the key type?


================
Comment at: libcxx/test/libcxx/containers/associative/perf.assignment_vs_construction.pass.cpp:33
+    std::set<std::string> tmp(keys.begin(), keys.end());
+    keys.assign(tmp.begin(), tmp.end());
+
----------------
Alternatively, this code could use an `unordered_map` to store generated keys and not add them to `keys` if they were already seen before. Though I don't think efficiency matters for this function.


================
Comment at: libcxx/test/libcxx/containers/associative/perf.assignment_vs_construction.pass.cpp:43
+    for (size_t i = 0; i < iterations; ++i) {
+        const auto start = std::chrono::high_resolution_clock::now();
+        f();
----------------
Very optional / question: using `<chrono>` always makes me want to create a short alias like `using namespace chr = std::chrono`. What's your preference on this?


================
Comment at: libcxx/test/libcxx/containers/associative/perf.assignment_vs_construction.pass.cpp:50
+
+    return std::accumulate(durations.begin(), durations.end(), 0) / iterations;
+}
----------------
Note: this could overflow if `iterations` were unbounded, though if we're always using a small integer, it doesn't matter.


================
Comment at: libcxx/test/libcxx/containers/associative/perf.assignment_vs_construction.pass.cpp:57-59
+    Container tmp;
+    for (const auto& key : keys)
+        tmp.insert({key, 1});
----------------
Optional: `test_map` and `test_set` could probably be merged into a single function by moving the difference out into a helper. The best I could come up with is to have a functor that takes care of the difference in `value_type` (to avoid specializing for all 4 map/set variations):
```
template <typename Container, typename Func>
void test(std::vector<std::string> const& keys, Func const& value_type_func) {
...
tmp.insert(value_type_func(key));

...

test<std::map<std::string, int>(keys, [](const auto& s) { return std::make_pair(s, 1); });
```
Though it might be not worth it.


================
Comment at: libcxx/test/libcxx/containers/associative/perf.assignment_vs_construction.pass.cpp:62
+    std::size_t ctor_avg = microseconds_avg(iterations, [&] {
+        Container foo(tmp);
+    });
----------------
I'm a bit concerned about the compiler seeing that `foo` is never used and potentially optimizing it away. Is it known that it's not the case / cannot be the case?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121485