<div class="gmail_quote">On Mon, Mar 5, 2012 at 3:15 AM, Jay Foad <span dir="ltr"><<a href="mailto:jay.foad@gmail.com">jay.foad@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="im">On 5 March 2012 10:17, Chandler Carruth <<a href="mailto:chandlerc@google.com">chandlerc@google.com</a>> wrote:<br>
</div><div class="im">> The measured changes in performance are all down into the noise floor,<br>
> despite everything I did to lower that. From what I can tell, this change<br>
> has no observable performance impact, and reduces the number of hashing<br>
> functions in use. Good to commit?<br>
<br>
</div>I like it.<br>
<br>
I notice that the patch doesn't actually remove HashString, because<br>
it's still used in loads of other places. Do you plan to convert them<br>
so that we can *really* "reduce the number of hashing functions in<br>
use"? (If you don't plan to do it yourself I suppose it would make a<br>
good beginner's project for someone else.)<br></blockquote><div><br></div><div>Yep, I'm steadily working through uses of HashString and DenseMapInfo converting them as I go. This one is just one of the more sensitive ones.</div>
<div><br></div><div><br></div><div>Also, in the interest of full disclosure, I'm tracking down some performance issues w/ the current hashing function stemming from poor inlining decisions when LLVM is optimizing code that calls this... Finally got it all isolated and can show that its just an inlining issue. Still, it doesn't impact anything I can measure in an execution of Clang, just impacts my theoretical micro-benchmarks.</div>
</div>