<div dir="ltr">Hey Duncan,<div><br></div><div>I know this isn't required, but would it be possible to put this on phabricator?</div><div><br></div><div>/Eric</div></div><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Jan 18, 2016 at 4:34 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">ping<br>
<div class="HOEnZb"><div class="h5"><br>
> On 2016-Jan-11, at 16:23, Duncan P. N. Exon Smith <<a href="mailto:dexonsmith@apple.com">dexonsmith@apple.com</a>> wrote:<br>
><br>
> ping<br>
><br>
>> On 2016-Jan-04, at 12:37, Duncan P. N. Exon Smith <<a href="mailto:dexonsmith@apple.com">dexonsmith@apple.com</a>> wrote:<br>
>><br>
>> ping<br>
>><br>
>>> On 2015-Dec-17, at 13:56, Duncan P. N. Exon Smith <<a href="mailto:dexonsmith@apple.com">dexonsmith@apple.com</a>> wrote:<br>
>>><br>
>>><br>
>>>> On 2015-Dec-16, at 14:42, Duncan P. N. Exon Smith <<a href="mailto:dexonsmith@apple.com">dexonsmith@apple.com</a>> wrote:<br>
>>>><br>
>>>> This is a follow-up to r239666: "Fix PR12999 - unordered_set::insert<br>
>>>> calls operator new when no insert occurs".  That fix didn't apply to<br>
>>>> `unordered_map` because unordered_map::value_type gets packed inside:<br>
>>>> --<br>
>>>> union __value_type {<br>
>>>> pair<key_type, mapped_type> __nc;       // Only C++11 or higher.<br>
>>>> pair<const key_type, mapped_type> __cc; // Always.<br>
>>>> // Constructors...<br>
>>>> };<br>
>>>> --<br>
>>>> and the underlying __hash_table only knows about __value_type.<br>
>>><br>
>>> Sorry for the quick ping, but I realized this morning that my approach<br>
>>> was still leaving mallocs on the table.<br>
>>><br>
>>> I've attached a new patch that handles more cases.<br>
>>><br>
>>> This patch should avoid unnecessary mallocs whenever the caller passes<br>
>>> in a pair<T, U> such that T is trivially convertible to key_type.<br>
>>><br>
>>> Since __hash_table's value_type is really *never* a pair (for<br>
>>> unordered_map, it's a union of two pairs) the static dispatch is all in<br>
>>> unordered_map.  It's doing this:<br>
>>> - If the argument isn't a pair<>, alloc.<br>
>>> - If argument.first can be referenced as const key_type&, don't alloc.<br>
>>> - If argument.first can be trivially converted to key_type, don't<br>
>>>  alloc.<br>
>>> - Else alloc.<br>
>>><br>
>>> In the pre-C++11 world the caller has already converted to<br>
>>> unordered_map::value_type.  We can always avoid the alloc.<br>
>>><br>
>>> To support all of this:<br>
>>> - In C++03, __unordered_map_equal and __unordered_map_hasher need to<br>
>>>  handle unordered_map::value_type.<br>
>>> - In C++03, __hash_table::__insert_unique_value() now takes its<br>
>>>  argument by template.<br>
>>> - In C++11, __hash_table::__insert_unique_value() is now a one-liner<br>
>>>  that forwards to __insert_unique_key_value() for the real work.<br>
>>> - The versions of __hash_table::__construct_node() that take a<br>
>>>  pre-computed hash have been renamed to __construct_node_hash(), and<br>
>>>  these versions use perfect forwarding.<br>
>>><br>
>>> Most of the following still apply:<br>
>>><br>
>>>> This is one of my first patches for libc++, and I'm not sure of a few<br>
>>>> things:<br>
>>>> - Did I successfully match the coding style?  (I'm kind of lost<br>
>>>> without clang-format TBH.)<br>
>>>> - Should I separate the change to __construct_node_hash() into a<br>
>>>> separate prep commit?  (I would if this were LLVM, but I'm not sure<br>
>>>> if the common practice is different for libc++.)<br>
>>>> - Most of the overloads I added to __unordered_map_hasher and<br>
>>>> __unordered_map_equal aren't actually used by<br>
>>>> __hash_table::__insert_unique_value().  Should I omit the unused<br>
>>>> ones?  (Again, for LLVM I would have omitted them.)<br>
>>><br>
>>> (For the updated patch, I went with the LLVM approach of only adding<br>
>>> the used API.  It seems more appropriate in this case.)<br>
>>><br>
>>>> After this I'll fix the same performance issue in std::map (and I<br>
>>>> assume std::set?).<br>
>><br>
>> <0001-unordered_map-Avoid-unnecessary-mallocs-when-no-i-v2.patch><br>
><br>
<br>
</div></div></blockquote></div><br></div>