<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Sun, Jul 17, 2016 at 2:54 PM, Arthur O'Dwyer <span dir="ltr"><<a href="mailto:arthur.j.odwyer@gmail.com" target="_blank">arthur.j.odwyer@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 dir="ltr">Given that this patch is basically Chandler's talk from CppCon 2015 (<a href="https://www.youtube.com/watch?v=nXaxk27zwlk" target="_blank">https://www.youtube.com/watch?v=nXaxk27zwlk</a>), I'm surprised that the commit message isn't explicitly mentioning that; and surprised that Chandler himself isn't weighing in on either the "this is a good idea" or "this is a bad idea" side.</div></blockquote><div><br></div><div><br></div><div>The commit message should have been clearer. Hopefully Chandler will weigh in one way or another.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div><br><div>IMHO, if replacing "%" with "fastmod" in general-purpose code like this were a good idea,</div><div>(A) libc++ should introduce a helper function __fastmod(m,n) for the purpose, not repeat the same patch everywhere there's currently a "%" operator; and/or</div><div>(B) someone with authority over the Clang x86 backend (*cough*Chandler*cough*) should look into improving the codegen for "%" by auto-detecting when it might make sense to use this heuristic.</div><div><br></div><div>The alleged existence of performance regressions on this patch as it stands seems like evidence for doing (B), IMHO, even if it takes longer.</div></div></div></blockquote><div><br></div><div>The performance regressions are almost certainly related to the optimization I removed/replaced in this patch. Previously when __hash_table::find was walking the bucket elements it would only re-computer the constrained hash</div><div>if the unconstrained hash didn't match that of the element being searched for. I removed this optimization in this patch with the intent of re-implementing it in the very near future (it was only in-tree for about a week).</div><div><br></div><div>What I'm curious about if this performance regression was cause by (1) the removal of this 2 week old optimization or (2) the implementation of "fast mod". I suspect it's because of (1).</div><div><br></div><div>I'm writing more benchmarks as we speak to figure this out.</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div><div><br></div><div>It's also counterintuitive to me that (__h < __bc) would be true any significant fraction of the time, on a 64-bit platform. Does this happen because __bc is often astronomically high, or because __h is often astronomically low (presumably due to bad hash functions, such as "always hash to constant 0")?</div></div></div></blockquote><div><br></div><div>It happens due to bad hash functions. Primarily because integral types use the identify hash function.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div><div><br></div><div>my $.02,</div><div>Arthur</div><div><br></div></div><div><div class="h5"><div class="gmail_extra"><br><div class="gmail_quote">On Sun, Jul 17, 2016 at 1:11 PM, Eric Fiselier via cfe-commits <span dir="ltr"><<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">Hi Duncan,<div><br></div><div>It's possibly expected. It depends on what operation it's performing. I expected a bit of a performance drop in some cases but I have a plan to fix those.</div><div>Do you have a link to LNT?</div></div><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Jul 13, 2016 at 6:41 PM, Duncan P. N. Exon Smith <span dir="ltr"><<a href="mailto:dexonsmith@apple.com" target="_blank">dexonsmith@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hmm. I implied there were other regressions, but I just finished scanning them. Shootout-C++/hash2 is the only major one. The others were small, and only at -O0.<br>
<span><br>
> On 2016-Jul-13, at 17:38, Duncan P. N. Exon Smith via cfe-commits <<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a>> wrote:<br>
><br>
> We saw mixed results from this on LNT, including some major regressions. For example, on x86_64, SingleSource/Benchmarks/Shootout-C++/hash2 regressed 18.5% at -O3 and over 20% at -Os.<br>
><br>
> Is this expected?<br>
<br>
</span>^ Still interested in an answer, though ;).<br>
<div><div><br>
><br>
>> On 2016-Jul-11, at 15:02, Eric Fiselier via cfe-commits <<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a>> wrote:<br>
>><br>
>> Author: ericwf<br>
>> Date: Mon Jul 11 17:02:02 2016<br>
>> New Revision: 275114<br>
>><br>
>> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=275114&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=275114&view=rev</a><br>
>> Log:<br>
>> Don't compute modulus of hash if it is smaller than the bucket count.<br>
>><br>
>> This cleans up a previous optimization attempt in hash, and results in<br>
>> additional performance improvements over that previous attempt. Additionally<br>
>> this new optimization does not hinder the power of 2 bucket count optimization.<br>
>><br>
>> Modified:<br>
>> libcxx/trunk/include/__hash_table<br>
>><br>
>> Modified: libcxx/trunk/include/__hash_table<br>
>> URL: <a href="http://llvm.org/viewvc/llvm-project/libcxx/trunk/include/__hash_table?rev=275114&r1=275113&r2=275114&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/libcxx/trunk/include/__hash_table?rev=275114&r1=275113&r2=275114&view=diff</a><br>
>> ==============================================================================<br>
>> --- libcxx/trunk/include/__hash_table (original)<br>
>> +++ libcxx/trunk/include/__hash_table Mon Jul 11 17:02:02 2016<br>
>> @@ -90,7 +90,8 @@ inline _LIBCPP_INLINE_VISIBILITY<br>
>> size_t<br>
>> __constrain_hash(size_t __h, size_t __bc)<br>
>> {<br>
>> - return !(__bc & (__bc - 1)) ? __h & (__bc - 1) : __h % __bc;<br>
>> + return !(__bc & (__bc - 1)) ? __h & (__bc - 1) :<br>
>> + (__h < __bc ? __h : __h % __bc);<br>
>> }<br>
>><br>
>> inline _LIBCPP_INLINE_VISIBILITY<br>
>> @@ -2201,8 +2202,7 @@ __hash_table<_Tp, _Hash, _Equal, _Alloc><br>
>> if (__nd != nullptr)<br>
>> {<br>
>> for (__nd = __nd->__next_; __nd != nullptr &&<br>
>> - (__hash == __nd->__hash_<br>
>> - || __constrain_hash(__nd->__hash_, __bc) == __chash);<br>
>> + __constrain_hash(__nd->__hash_, __bc) == __chash;<br>
>> __nd = __nd->__next_)<br>
>> {<br>
>> if ((__nd->__hash_ == __hash) && key_eq()(__nd->__value_, __k))<br>
>> @@ -2231,8 +2231,7 @@ __hash_table<_Tp, _Hash, _Equal, _Alloc><br>
>> if (__nd != nullptr)<br>
>> {<br>
>> for (__nd = __nd->__next_; __nd != nullptr &&<br>
>> - (__hash == __nd->__hash_<br>
>> - || __constrain_hash(__nd->__hash_, __bc) == __chash);<br>
>> + __constrain_hash(__nd->__hash_, __bc) == __chash;<br>
>> __nd = __nd->__next_)<br>
>> {<br>
>> if ((__nd->__hash_ == __hash) && key_eq()(__nd->__value_, __k))<br>
>><br>
>><br>
>> _______________________________________________<br>
>> cfe-commits mailing list<br>
>> <a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a><br>
>> <a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits</a><br>
><br>
> _______________________________________________<br>
> cfe-commits mailing list<br>
> <a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a><br>
> <a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits</a><br>
<br>
</div></div></blockquote></div><br></div>
<br>_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits</a><br>
<br></blockquote></div><br></div></div></div></div>
</blockquote></div><br></div></div>