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

[Ivan via Phabricator via libcxx-commits]

itrofimow marked 12 inline comments as done.
itrofimow added a comment.

So i got some benchmarks: unordered_set_benches <https://pastebin.com/raw/cYTm7DjE>

And they didn't make any sense for `BM_FindRehash`: how could my changes affect it that much, when they don't even touch `::find`?
After some profiling i found out that branch misprediction in `std::equal_to` increases up to x2 from ~11% to 23%, with overall mispediction being close to 4%.

Changing initial rehash in `BM_FindRehash` from 8 to 7 leads to this: unordered_set_benches <https://pastebin.com/raw/9aWbwpFh>, which is what i expected to be,
but i still have no idea what happens in original version.



================
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> {};
----------------
philnik wrote:
> I don't think this is necessary. Just use `integral_constant<bool, {true, false}>` where `__hash_table` is used or use a NTTP instead.
I feel like that would make `__hash_table` usage way less readable: is `true` self explanatory enough in `__hash_table<Key, Hash, Eq, Alloc, true>`?


================
Comment at: libcxx/include/ext/hash_map:490
     typedef std::__hash_table<__value_type, __hasher,
-                         __key_equal,  __allocator_type>   __table;
+                         __key_equal,  __allocator_type, std::__hash_table_unique_keys<true> >   __table;
 
----------------
Mordante wrote:
> There's no need for `std::` here.
doesn't compile without it


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128021