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

[Ivan via Phabricator via libcxx-commits]

itrofimow added inline comments.


================
Comment at: libcxx/benchmarks/ContainerBenchmarks.h:140
+static void BM_Rehash(benchmark::State& st, Container c, GenInputs gen) {
+    auto in = gen(st.range(0));
+    c.max_load_factor(3.0);
----------------
Mordante wrote:
> Please don't use `auto` when the type isn't clear, same for the `bucket_count`.
Type here differs for different benchmark registrations, so it would be some decltype otherwise.

I could change `bucket_count` to be a size_t if you insist, but i feel like it's not only clearly `size_t`, but also the same type `rehash` takes anyway


================
Comment at: libcxx/benchmarks/unordered_set_operations.bench.cpp:311
+    getRandomStringInputs)->Arg(TestNumInputs);
+
 BENCHMARK_MAIN();
----------------
philnik wrote:
> 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).
Strangely enough i couldn't find any `unordered_map/multimap/multiset` benchmarks, and i'm not sure i am familiar enough with this codebase to write a comprehensive ones from scratch


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128021