[Diffusion] rL258575: unordered_map: Reuse insert logic in emplace when possible, NFC

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

+cfe-commits

(This looks like a broken feature of Phab.  I suggest not using it,
since it hasn't put the patch context in this email, it didn't CC
the list, and it spawned a new thread instead of replying to the
commit:
http://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20160118/147763.html

I'll reply to that mail in a moment to link to this thread.)

> On 2016-Jan-22, at 20:13, Eric Fiselier <eric at efcs.ca> wrote:
> 
> EricWF added a subscriber: EricWF.
> EricWF raised a concern with this commit.
> EricWF added a comment.
> 
> @dexonsmith This change should have been reviewed before committing.

Sorry about this.  I had assumed LLVM's culture of post-commit
review.  I'll take things more slowly going forward.

> I think there are better ways to make `insert` and `emplace` work together. In particular I would like to put most of the logic in `<__hash_table>` not `<unordered_map>`.

Given that __hash_table and unordered_map have incompatible
concepts of "value_type", and the purpose of sharing them is to
optimize away mallocs when we can pull out the right parts of
value_type, I'm skeptical.

> Please revert this commit.

r258625

> /libcxx/trunk/include/unordered_map:930 I think we can just call `__table_.__insert_unique(std::forward<_Arg>(__arg))` for the one arg case and `__table_.

The purpose of this commit was to ensure that emplace gets the same
malloc optimizations as insert, so that the tests in this patch
pass:
http://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20160118/147766.html

I don't see how this alternate approach would accomplish that?

> /libcxx/trunk/include/unordered_map:932 I think this optimization should be limited to when "_Arg" is "value_type". 

The patch linked above optimizes mallocs whenever key_type matches.
I also have a follow-up (originally part of that patch) that
strengthens the optimization to whenever we can convert to key_type
(and destruct it) trivially.  It's simplest to forward to insert()
to guarantee that each behaves "as if" the other were called;
otherwise that patch needs to duplicate all the type-based dispatch
in two places.

> * `is_constructible` can cause a hard compile error when using Clang. I prefer not to use it when possible. 

That's terrible.  In what versions?  (If ToT, we should fix that...
can you point me at a PR?)

It's already used in the enable_if for `insert()`.  Does it break
there?  (My experience is that hard compile errors usually prevent
meta-functions from being used in enable_ifs; I'd assumed there
weren't any bugs in is_constructible since I saw the use below.)

Maybe I should pivot the malloc optimization; if we change
`insert()` to just call `emplace()` -- valid because of the
enable_if (I assume the bugs in is_constructible don't cause
miscompiles?) -- I can do the malloc optimization inside the
single-argument `emplace()`.  This would require more intrusive
changes to __hash_table of course, since I think your original
commit (r239666) only optimized the insert-side.  What do you
think?

> 
> 
> Users:
>  dexonsmith (Author)
>  EricWF (Auditor)
> 
> http://reviews.llvm.org/rL258575
> 
> 
>