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

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

I'll upload a new patch in a moment.  Replies inline below.

> On 2016-Jan-21, at 23:12, Eric Fiselier <eric at efcs.ca> wrote:
> 
> EricWF added a comment.
> 
> Overall the patch looks good but I have a few concerns.
> 
>> - If argument.first can be trivially converted to key_type, don't alloc.
> 
> 
> I'm concerned with this part of the change because:
> 
> - The `is_trivially_*` traits are often not available and can sometimes blow up.

When aren't they?  How do they blow up?

Is there a _LIBCPP config macro we could add?

> - It also makes the implementation of `__insert_extract_key_if_pair` a lot more complicated.
> - It is technically non-standard because the standard says "insert(P&&)` should be handled as-if "emplace(P&&").

Sorry I missed this.

> I would like to put a lot more thought into this part of your patch. Could you remove it and add it in a follow up?

Split.  I'll upload the main/first part.

> I would also love if we applied this optimization to single argument "emplace" calls.\

Just committed r258575 which should take care of this.  The new patch
will have extra tests to cover insert_hint(), emplace(), and
emplace_hint().

> 
> ================
> Comment at: include/__hash_table:867
> @@ -864,1 +866,3 @@
> +    _LIBCPP_INLINE_VISIBILITY
> +    pair<iterator, bool> __insert_unique_key_value(const _KeyTp &__key, _ValueTp&& __x);
> #else
> ----------------
> I really don't like how the name and signature of the function changes between C++03 and C++11. It's confusing me a lot while reviewing the patch.  I would much rather it always accept two parameters and be called `__insert_unique_key_value`. 
> 
> We can add a C++03 __insert_unique_value that dispatches like you do above if needed.

Yes, that's better.  Done.

> ================
> Comment at: include/unordered_map:403
> @@ +402,3 @@
> +    _LIBCPP_INLINE_VISIBILITY
> +    size_t operator()(const typename _Cp::value_type& __x) const
> +        {return static_cast<const _Hash&>(*this)(__x.first);}
> ----------------
> This part LGTM so long as instantiating _Cp in the function signature doesn't prevent unordered_map from being used with incomplete types.

I'll double check this before commit (I'll look for a test and
add one if it's missing).

> ================
> Comment at: include/unordered_map:993
> @@ +992,3 @@
> +                _VSTD::forward<_Pp>(__x),
> +                is_same<typename decay<_FirstTp>::type, key_type>());
> +        }
> ----------------
> We don't want to decay because we don't want "array-to-pointer" and "function-to-pointer" conversions.  We just want to remove the cv and ref qualifiers.
> 
> I *just* added a "__uncvref"trait in <type_traits>. Please use that instead.
> 
> On second thought, I don't think we want to remove the volatile qualifiers.

Right.  Fixed.

> 
> ================
> Comment at: include/unordered_map:998
> @@ +997,3 @@
> +    _LIBCPP_INLINE_VISIBILITY
> +    pair<iterator, bool> __insert_extract_key_if_referenceable(_Pp&& __x, false_type)
> +        {
> ----------------
> I'm pretty concerned about the trivially_constructible part of the patch/optimization.  Can we handle this in a follow up revision instead?

Yup.