[libcxx-commits] [PATCH] D128021: [libc++] Don't call key_eq in unordered_map/set rehashing routine

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik added a comment.

This looks pretty good so far, but I'd like to see a full set of benchmarks before approving. BTW it's really stupid that we have all the member function definitions out of line. It makes this diff so much larger than it needs to be.

In D128021#3593737 <https://reviews.llvm.org/D128021#3593737>, @itrofimow wrote:

> Can some please help me with clang-format builds failing?
> This is what i used to update the patch `arc diff HEAD~ --update D128021 --nolint`, but format build are still failing.
> What should i do to fix that?

You can ignore the clang-format runner. It's just soft-failing. Your problem with the CI currently is that you've got some non-ASCII symbols in your patch. That's why `Generated output` fails.



================
Comment at: libcxx/benchmarks/ContainerBenchmarks.h:143
+    c.insert(in.begin(), in.end());
+    benchmark::DoNotOptimize(&(*c.begin()));
+    const auto bucket_count = c.bucket_count();
----------------
`benchmark::DoNotOptimize(c)` should be more than enough. This doesn't allow the compiler to optimize anything from the container.


================
Comment at: libcxx/benchmarks/unordered_set_operations.bench.cpp:311
+    getRandomStringInputs)->Arg(TestNumInputs);
+
 BENCHMARK_MAIN();
----------------
itrofimow wrote:
> This is with the patch:
> BM_Rehash/unordered_set_string_arg/1024      44848 ns        44846 ns        96172
> 
> This is on trunk:
> BM_Rehash/unordered_set_string_arg/1024      55711 ns        55706 ns        74198
> 
> full output: https://pastebin.com/7DMcNTna
> 
> 
> This benchmark doesn't mean there is an immense speedup in rehashing, it just shows that there is a speedup - `max_load_factor(3)` was used to be sure that there will be collisions and key_eq branch in rehashing would be hit. So this just measures how long it takes to compare long random strings, but with the patch we don't compare them at all.
Could you provide the results of a full set of input data (i.e. elements other than string and other sizes)? I don't think you cherry-picked, but having only a single data point smells like it. Since this patch also affects `unordered_map`, `unordered_multimap` and `unordered_multiset` I'd also like to see benchmarks for those (the multi-version are optional, but a nice to have).


================
Comment at: libcxx/include/__hash_table:863-868
+// Since both unordered_set/map and unordered_multiset/multimap 
+// use the same implementation of hash_table we might do reduntand work 
+// for unique keys containers (namely calling key_eq in rehashing).
+// This trait helps to distinguish between unique keys/non-unique keys containers.
+template <bool _UniqueKeys>
+struct __hash_table_unique_keys : std::integral_constant<bool, _UniqueKeys> {};
----------------
I don't think this is necessary. Just use `integral_constant<bool, {true, false}>` where `__hash_table` is used or use a NTTP instead.


================
Comment at: libcxx/include/__hash_table:2284
                         __next_pointer __np = __cp;
-                        for (; __np->__next_ != nullptr &&
-                               key_eq()(__cp->__upcast()->__value_,
-                                        __np->__next_->__upcast()->__value_);
-                                                           __np = __np->__next_)
-                            ;
+			if (!_UniqueKeys::value) {
+                            for (; __np->__next_ != nullptr &&
----------------
jloser wrote:
> Question: what's the libc++ stance for something like this which could be `if constexpr` in C++17 or later? Is it worth writing
> 
> ```
> #if _LIBCPP_STD_VER >= 17
> if constexpr
> #else
> if
> #endif
> (!UniqueKeys::Value) {
> ```
> 
> I suspect the answer is just writing `if` for all standards modes and just letting the compiler realize it's always a constant value now up-front.
It's definitely not worth it. This makes the code a lot less readable for a minor to non-existent improvement in compile times.


================
Comment at: libcxx/include/__hash_table:2284
                         __next_pointer __np = __cp;
-                        for (; __np->__next_ != nullptr &&
-                               key_eq()(__cp->__upcast()->__value_,
-                                        __np->__next_->__upcast()->__value_);
-                                                           __np = __np->__next_)
-                            ;
+			if (!_UniqueKeys::value) {
+                            for (; __np->__next_ != nullptr &&
----------------
philnik wrote:
> jloser wrote:
> > Question: what's the libc++ stance for something like this which could be `if constexpr` in C++17 or later? Is it worth writing
> > 
> > ```
> > #if _LIBCPP_STD_VER >= 17
> > if constexpr
> > #else
> > if
> > #endif
> > (!UniqueKeys::Value) {
> > ```
> > 
> > I suspect the answer is just writing `if` for all standards modes and just letting the compiler realize it's always a constant value now up-front.
> It's definitely not worth it. This makes the code a lot less readable for a minor to non-existent improvement in compile times.
Could you indent this properly?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128021