[libcxx] [PATCH] unordered_map: Avoid unnecessary mallocs when no insert occurs

[Duncan P. N. Exon Smith via cfe-commits]

ping

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