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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante accepted this revision as: Mordante.
Mordante added a comment.

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

> In D128021#3627048 <https://reviews.llvm.org/D128021#3627048>, @Mordante wrote:
>
>> In D128021#3627047 <https://reviews.llvm.org/D128021#3627047>, @itrofimow wrote:
>>
>>> binary size after changes: https://pastebin.com/raw/z23mLN93
>>
>>
>>
>>    text	   data	    bss	    dec	    hex	filename
>>   10323	    744	      8	  11075	   2b43	binary_size_main
>>   10996	    744	      8	  11748	   2de4	binary_size_patch
>>
>> Thanks! I like these numbers better! This about half of the size overhead of the previous version.
>> I assume the main values have changed due to rebasing, right?
>
> I did rebase, however i think main values changed due to me compiling on a different machine.
> Not sure if i understand the `size` command output completely, but this time binary takes only ~150 bytes more on disk versus ~4.5Kb in the previous version

The size command gives more insight in the real binary size. The file on disk contains a lot of additional information, like debug information, information needed for the dynamic linker, etc.

LGTM modulo some small nits. I'll leave the final approval to @philnik.



================
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);
----------------
itrofimow wrote:
> 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
I think it's good to use real types when possible.


================
Comment at: libcxx/benchmarks/unordered_set_operations.bench.cpp:113
+// Early return is there just for convenience, since we only compare strings
+// of equal length in BM_Rehash.
+struct SlowStringEq {
----------------
Thanks this clarifies it!
You could consider adding an assert that the sizes are equal.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128021