<div class="gmail_quote">On Wed, Mar 7, 2012 at 8:14 PM, Jakob Stoklund Olesen <span dir="ltr"><<a href="mailto:stoklund@2pi.dk">stoklund@2pi.dk</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="HOEnZb"><div class="h5"><br>
On Mar 7, 2012, at 12:29 AM, Duncan Sands wrote:<br>
<br>
>> I think you underestimate how slow it is to hash things using incremental calls<br>
>> to hash_combine. The reason for the other interface is that it is *much* faster.<br>
><br>
> why is this?  Is it just that the optimizers are doing a poor job, or is there a<br>
> more fundamental reason that means that multiple hash_combine calls can't be as<br>
> efficient as hash_combine_range?<br>
<br>
</div></div>Good hash functions have an internal state that is much larger than the final hash. Chandler's variant of CityHash has 6 x 64-bit state registers in hash_state. The large internal state is essential for cryptographic hash functions to prevent collisions, and hashes that simply try to be fast can be less careful about losing entropy when adding new data.<br>

<br>
The normal operation of a hash function is to accumulate entropy from all input data in the internal state. When all entropy has been collected, the internal state is projected onto the smaller output space, a size_t / hash_code in this case. The final projection is less performance sensitive than the accumulation since it only runs once per hash.<br>
</blockquote><div><br></div><div>So far, this is a great explanation.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">The hash_combine interface forces hashes to be computed as an expression tree. Every call to hash_combine projects onto the smaller output space prematurely, and the projection function is executed for each node of the expression tree instead of just once. This increases the likelihood of collisions, and it makes the hash more expensive to compute than it needs to be.<br>
</blockquote><div><br></div><div>While this is a bit true, it's only a bit. There are a couple of important factors to the algorithm in question.</div><div><br></div><div>First is that final cost you are referring to as "projecting onto a smaller space". This is usually referred to as the "finalization" phase. This isn't actually terribly slow in modern hash functions (such as the one I've added). It's not as free as continuing to hash a run of contiguous data, but it's quite likely cheaper than crossing cache lines for example.</div>
<div><br></div><div>Second is that it doesn't actually increase the likelihood of collisions significantly. I was actually surprised by this, but talking to our hashing experts (authors of City and Murmur), they indicated it really wasn't significant for these types of hashing functions. (This clearly isn't necessarily true or relevant for cryptographic functions.)</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Chandler is trying to minimize the number of nodes in the expression tree by avoiding hash_combine() calls.</blockquote>
<div><br></div><div>Not quite. If it were *just* the nodes in the graph, it really might not make such a big difference. I think the big difference comes from placing all the data to be hashed into a dense contiguous buffer that we can read out of efficiently.</div>
<div><br></div><div>The functions actually do this internally when they aren't called with an existing buffer of suitable form, but in this particular case, there isn't a great way to take advantage of that... More on that in a bit.</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> I don't think that allocating memory in a hash function is a good alternative, though.<br></blockquote>
<div><br></div><div>True, in the general case, this isn't a good tradeoff. But what I've actually done (and note that this has only come up in one place throughout LLVM thus far) is a bit more complex than that. I used a SmallVector to build up small buffers directly on the stack, and only fall over to allocating from the heap when we're hashing a significant amount of data.</div>
<div><br></div><div>This *still* isn't ideal, I agree. The hashing routines were actually designed to avoid this by keeping a single fixed-size buffer (hopefully on the stack), filling and re-using it iteratively. The only case where I think I currently use this strategy is a case where we don't have a good iterator to walk over the elements we want to hash. Given an iterator, we'll used a fixed buffer, and recycle that data iteratively.</div>
<div><br></div><div>The goal of all of this is not really about avoiding the finalization step, it's about reading contiguous, sequential data as opposed to jumping from point-to-point.</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
I would propose an interface that doesn't constantly collapse the hash state when hashing composite objects. The hash_state class can be given an interface very similar to raw_ostream:<br>
<br>
hash_state &operator<<(hash_state &HS, const ConstantClass *CP) {<br>
  HS << CP->getType();<br>
<div class="im">  for (unsigned I = 0, E = CP->getNumOperands(); I < E; ++I)<br>
</div>    HS << CP->getOperand(I);<br>
  return HS;<br>
}<br>
<br>
The hash_state object is created by the final consumer of the hash value, and passed around by reference to all the parts of a composite object.</blockquote><div><br></div><div>This option was definitely looked at by a bunch of us when designing the interface for the standards proposal. It was one of the favorites even. Everyone felt that this was a workable interface, but that it required more boiler plate code to stitch together. There was a strong desire to remove almost all loops, as those tend to be expensive both to write and to optimize.</div>
<div><br></div><div>I think it's not an unreasonable goal to have the above look like:</div><div><br></div><div>hash_combine(CP->getType(),</div><div>             hash_combine_range(CP->operand_begin(), CP->operand_end()));</div>
<div><br></div><div>This shouldn't require allocating anything on the heap, and it is really unlikely to be noticeably slower in practice. The slow part is likely to be reading the data above that is not in-cache.</div>
<div><br></div><div><br></div><div>That said, I wouldn't look at the generated code yet. The above interface being efficient was definitely predicated on the idea that the optimizer would Do The Right Thing. Currently, it doesn't. I'm trying to fix that. ;] Without that fix, the function call overhead alone will actually suck up most of the time.</div>
</div>