[Diffusion] rL258575: unordered_map: Reuse insert logic in emplace when possible, NFC
Duncan P. N. Exon Smith via cfe-commits
cfe-commits at lists.llvm.org
Sat Jan 23 07:55:50 PST 2016
(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
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.
> /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
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
> dexonsmith (Author)
> EricWF (Auditor)
More information about the cfe-commits