<div dir="ltr">Hi Howard,<div><br></div><div>Thanks for the advice. I'll make sure to performance test these changes without moving forward.</div><div>I also appreciate the insight. </div><div><br></div><div><span style="font-family:arial,sans-serif;font-size:13px">> 0 is a valid bucket count. An upward bump of 0 buckets goes to 2.</span><br style="font-family:arial,sans-serif;font-size:13px">
<span style="font-family:arial,sans-serif;font-size:13px">1 is considered not a power of 2, but it rounded up to 2.</span><br style="font-family:arial,sans-serif;font-size:13px"><span style="font-family:arial,sans-serif;font-size:13px">2 is considered prime, not a power of 2. Thus a upward bump off bucket size 2 goes to 5, not 4.</span><br>
</div><div><br></div><div>Are you referring to the input/output from the __next_power2 function?</div><div><font face="arial, sans-serif">Passing 1 to __next_power2 causes undefined behavior because it passes 0 to __clz. </font></div>
<div><font face="arial, sans-serif"><br></font></div><div><font face="arial, sans-serif">I'll consider just backing out these changes and renaming the functions so they don't imply mathematical accuracy. </font><span style="font-family:arial,sans-serif"> </span></div>
<div><span style="font-family:arial,sans-serif"><br></span></div><div><span style="font-family:arial,sans-serif">Thanks</span></div><div><font face="arial, sans-serif"><br></font></div><div><span style="font-family:arial,sans-serif;font-size:13px"><br>
</span></div></div><div class="gmail_extra"><br><br><div class="gmail_quote">On Sun, Aug 17, 2014 at 7:13 PM, Howard Hinnant <span dir="ltr"><<a href="mailto:howard.hinnant@gmail.com" target="_blank">howard.hinnant@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Just a word of caution:<br>
<br>
These functions were not intended to be mathematically accurate. They were intended to make the unordered containers as fast as possible. Performance tests (not checked in) were used to tune these functions. Put one too many branches in a hot path, and you can easily destroy the performance gains that were intended by allowing power-of-2 bucket sizes. I’m not saying this patch does this. I’m just saying, you are in danger of doing so if you haven’t performance tested these changes.<br>
<br>
Also, for the purpose of the libc++ unordered containers:<br>
<br>
0 is a valid bucket count. An upward bump of 0 buckets goes to 2.<br>
1 is considered not a power of 2, but it rounded up to 2.<br>
2 is considered prime, not a power of 2. Thus a upward bump off bucket size 2 goes to 5, not 4.<br>
<br>
For all other bucket sizes, it is unambiguous as to whether one is dealing with a prime or power of 2.<br>
<br>
These definitions are intended to promote best practices for the libc++ unordered containers, as opposed to being mathematically accurate.<br>
<br>
Howard<br>
<div><div class="h5"><br>
On Aug 17, 2014, at 8:46 PM, Eric Fiselier <<a href="mailto:eric@efcs.ca">eric@efcs.ca</a>> wrote:<br>
<br>
> I've abstracted the changes to __hash_table to call __is_rehash_power2. As for internal API testing, there currently is none. there has been some offline discussion about changing that and (this being a motivating case). I'll bring it up with Marshall again when he gets back.<br>
><br>
> <a href="http://reviews.llvm.org/D4948" target="_blank">http://reviews.llvm.org/D4948</a><br>
><br>
> Files:<br>
> include/__hash_table<br>
><br>
> Index: include/__hash_table<br>
> ===================================================================<br>
> --- include/__hash_table<br>
> +++ include/__hash_table<br>
> @@ -63,7 +63,7 @@<br>
> bool<br>
> __is_power2(size_t __bc)<br>
> {<br>
> - return __bc > 2 && !(__bc & (__bc - 1));<br>
> + return __bc && !(__bc & (__bc - 1));<br>
> }<br>
><br>
> inline _LIBCPP_INLINE_VISIBILITY<br>
> @@ -74,10 +74,17 @@<br>
> }<br>
><br>
> inline _LIBCPP_INLINE_VISIBILITY<br>
> +bool __is_rehash_power2(size_t __bc)<br>
> +{<br>
> + return __bc > 2 && __is_power2(__bc);<br>
> +}<br>
> +<br>
> +inline _LIBCPP_INLINE_VISIBILITY<br>
> size_t<br>
> __next_pow2(size_t __n)<br>
> {<br>
> - return size_t(1) << (std::numeric_limits<size_t>::digits - __clz(__n-1));<br>
> + return __n < 2 ? __n + 1<br>
> + : size_t(1) << (std::numeric_limits<size_t>::digits - __clz(__n-1));<br>
> }<br>
><br>
> template <class _Tp, class _Hash, class _Equal, class _Alloc> class __hash_table;<br>
> @@ -1615,7 +1622,7 @@<br>
> {<br>
> if (size()+1 > __bc * max_load_factor() || __bc == 0)<br>
> {<br>
> - rehash(_VSTD::max<size_type>(2 * __bc + !__is_power2(__bc),<br>
> + rehash(_VSTD::max<size_type>(2 * __bc + __is_rehash_power2(__bc),<br>
> size_type(ceil(float(size() + 1) / max_load_factor()))));<br>
> __bc = bucket_count();<br>
> __chash = __constrain_hash(__nd->__hash_, __bc);<br>
> @@ -1658,7 +1665,7 @@<br>
> size_type __bc = bucket_count();<br>
> if (size()+1 > __bc * max_load_factor() || __bc == 0)<br>
> {<br>
> - rehash(_VSTD::max<size_type>(2 * __bc + !__is_power2(__bc),<br>
> + rehash(_VSTD::max<size_type>(2 * __bc + __is_rehash_power2(__bc),<br>
> size_type(ceil(float(size() + 1) / max_load_factor()))));<br>
> __bc = bucket_count();<br>
> }<br>
> @@ -1728,7 +1735,7 @@<br>
> size_type __bc = bucket_count();<br>
> if (size()+1 > __bc * max_load_factor() || __bc == 0)<br>
> {<br>
> - rehash(_VSTD::max<size_type>(2 * __bc + !__is_power2(__bc),<br>
> + rehash(_VSTD::max<size_type>(2 * __bc + __is_rehash_power2(__bc),<br>
> size_type(ceil(float(size() + 1) / max_load_factor()))));<br>
> __bc = bucket_count();<br>
> }<br>
> @@ -1776,7 +1783,7 @@<br>
> __node_holder __h = __construct_node(__x, __hash);<br>
> if (size()+1 > __bc * max_load_factor() || __bc == 0)<br>
> {<br>
> - rehash(_VSTD::max<size_type>(2 * __bc + !__is_power2(__bc),<br>
> + rehash(_VSTD::max<size_type>(2 * __bc + __is_rehash_power2(__bc),<br>
> size_type(ceil(float(size() + 1) / max_load_factor()))));<br>
> __bc = bucket_count();<br>
> __chash = __constrain_hash(__hash, __bc);<br>
> @@ -1946,8 +1953,9 @@<br>
> __n = _VSTD::max<size_type><br>
> (<br>
> __n,<br>
> - __is_power2(__bc) ? __next_pow2(size_t(ceil(float(size()) / max_load_factor()))) :<br>
> - __next_prime(size_t(ceil(float(size()) / max_load_factor())))<br>
> + (__is_rehash_power2(__bc))<br>
> + ? __next_pow2(size_t(ceil(float(size()) / max_load_factor())))<br>
> + : __next_prime(size_t(ceil(float(size()) / max_load_factor())))<br>
> );<br>
> if (__n < __bc)<br>
> __rehash(__n);<br>
</div></div>> <D4948.12598.patch>_______________________________________________<br>
> cfe-commits mailing list<br>
> <a href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a><br>
> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
<br>
</blockquote></div><br></div>