<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Thu, Nov 30, 2017 at 3:48 PM, Rafael Avila de Espindola <span dir="ltr"><<a href="mailto:rafael.espindola@gmail.com" target="_blank">rafael.espindola@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Can we add a test showing the number of buckets that is selected?<br>
<br>
Rui Ueyama via Phabricator <<a href="mailto:reviews@reviews.llvm.org" target="_blank">reviews@reviews.llvm.org</a>> writes:<br>
<br>
> -// Returns a number of hash buckets to accomodate given number of elements.<br>
> -// We want to choose a moderate number that is not too small (which<br>
> -// causes too many hash collisions) and not too large (which wastes<br>
> -// disk space.)<br>
> -//<br>
> -// We return a prime number because it (is believed to) achieve good<br>
> -// hash distribution.<br>
> -static size_t getBucketSize(size_t NumSymbols) {<br>
> -  // List of largest prime numbers that are not greater than 2^n + 1.<br>
> -  for (size_t N : {131071, 65521, 32749, 16381, 8191, 4093, 2039, 1021, 509,<br>
> -                   251, 127, 61, 31, 13, 7, 3, 1})<br>
> -    if (N <= NumSymbols)<br>
> -      return N;<br>
> -  return 0;<br>
> -}<br>
<br>
It turns out that using a prime number was not worth it?<br></blockquote><div><br></div><div>Looks like so. It was cargo-culted from the GNU linkers. I don't think that the hash function for the .gnu.hash is so bad that it show some pattern when divided by a non-prime number. You can confirm that in the hash distribution histogram for this particular example.</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
>  // Add symbols to this symbol hash table. Note that this function<br>
>  // destructively sort a given vector -- which is needed because<br>
>  // GNU-style hash table places some sorting requirements.<br>
> @@ -1813,7 +1797,12 @@<br>
>      Symbols.push_back({B, Ent.StrTabOffset, hashGnu(B->getName())});<br>
>    }<br>
><br>
> -  NBuckets = getBucketSize(Symbols.size());<br>
> +  // We chose load factor 4 for the on-disk hash table. For each hash<br>
> +  // collision, the dynamic linker will compare a uint32_t hash value.<br>
> +  // Since the integer comparison is quite fast, I believe we can make<br>
> +  // the load factor even larger. 4 is just a conservative choice.<br>
> +  NBuckets = std::max<size_t>(Symbols.size(<wbr>) / 4, 1);<br>
<br>
Make the comment impersonal.<br>
<br>
LGTM with that.<br>
<br>
We can make the hashes bigger again (or use prime numbers) if we get a<br>
benchmark showing a problem. For now I think we should take the space<br>
saving.<br>
<br>
Cheers,<br>
Rafael<br>
</blockquote></div><br></div></div>