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

> On 2016-Mar-16, at 12:31, Duncan P. N. Exon Smith > wrote: > >> >> On 2016-Mar-16, at 12:20, Eric Fiselier wrote: >> >> EricWF added a comment. >> >> Adding inline comments for the implementation. Comments on the tests to >> follow shortly. >> >> >>

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

EricWF added a comment. Adding inline comments for the implementation. Comments on the tests to follow shortly. Comment at: include/__hash_table:103 @@ -102,1 +102,3 @@ +template +struct __extract_key; Could you make `__extract_key` behave the same way as

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

> On 2016-Mar-16, at 15:41, Eric Fiselier wrote: > > EricWF accepted this revision. > EricWF added a comment. > This revision is now accepted and ready to land. > > LGTM after change in inline comment. > > > > Comment at: include/__hash_table:114 > @@ +113,3 @@

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

EricWF accepted this revision. EricWF added a comment. This revision is now accepted and ready to land. LGTM after change in inline comment. Comment at: include/__hash_table:114 @@ +113,3 @@ +template +struct __can_extract_key<_Pair, _Key, pair<_First, _Second>> +:

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

dexonsmith updated this revision to Diff 50883. dexonsmith added a comment. Eric sent me his preferred tests, which look fine to me. I've applied them to this patch. http://reviews.llvm.org/D16360 Files: include/__hash_table

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

On Wed, Mar 16, 2016 at 1:31 PM, Duncan P. N. Exon Smith < dexonsm...@apple.com> wrote: > > > On 2016-Mar-16, at 12:20, Eric Fiselier wrote: > > > > EricWF added a comment. > > > > Adding inline comments for the implementation. Comments on the tests to > follow shortly. > > > > > >

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

Great! I think we should proceed with your is_trivially_copyable idea and I'll see if I can come up with a pathological test case that breaks it. Hopefully we can find something that works. On Mar 17, 2016 2:54 PM, "Duncan P. N. Exon Smith via cfe-commits" < cfe-commits@lists.llvm.org> wrote: >

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

dexonsmith updated this revision to Diff 50869. dexonsmith added a comment. Eric and I had a quick chat on IRC. - The asymmetric usage of __extract_key and __can_extract_key was awkward, as Eric pointed out already. - However, a symmetric __extract_key would have caused Clang (and other

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

EricWF added a comment. No worries, I'll try and take a look this weekend. Thanks. http://reviews.llvm.org/D16360 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

dexonsmith updated this revision to Diff 50514. dexonsmith added a comment. A few updates: - Rebased on top of http://reviews.llvm.org/D18115. - Rewrote the test to match suggested style. - Updated the code to catch unordered_set::emplace as well, using a __can_extract_key trait and a companion

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

dexonsmith added a comment. Honestly I didn't think much about the testing style. I used r239666 as a reference and modified it for std::unordered_map. I'll redo the tests when I have a moment, based on the newer examples. Probably next week some time? http://reviews.llvm.org/D16360

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

Honestly I didn't think much about the testing style. I used r239666 as a reference and modified it for std::unordered_map. I'll redo the tests when I have a moment, based on the newer examples. Probably next week some time? > On 2016-Feb-19, at 17:53, Eric Fiselier wrote: > >

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

EricWF added a comment. @dexonsmith Actually is it OK if I contribute the tests for this patch? Your's are in no way bad, but I want to test this in a similar manner to `unord.map.modifiers/insert_allocator_requirments.pass.cpp` and it's not fair to ask you to do that. However if your willing

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

EricWF added a comment. Overall this patch is *almost* there. My only objection is that this optimization neglects "unordered_set". Optimally we would also catch "unordered_set.emplace(42)" Comment at: include/__hash_table:103 @@ -102,1 +102,3 @@ +template ::type>

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

dexonsmith updated this revision to Diff 47936. dexonsmith added a comment. Updated the patch to apply against r260513. The logic is much simpler now; thanks Eric for the cleanup. Let me know if there's anything else to do! http://reviews.llvm.org/D16360 Files: include/__hash_table

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

dexonsmith updated this revision to Diff 46603. dexonsmith added a comment. Eric, I think this addresses all of your review comments. There are a couple of prep NFC commits that I sent for review: http://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20160201/148661.html

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

Marshall, thanks for the link to #2464. That does look scary. Nevertheless, I think this -- and the other over-eager allocations in {unordered_,}{multi,}{map,set} -- are worth optimizing as long as we can do it in a standards-compliant way. It's a pretty serious regression in performance

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

mclow.lists added a comment. I don't have any comments on this code at this time, but I want to caution people that this part of the standard library is **extremely** carefully specified, and meeting all the requirements is a fiddly bit of work. For an example of this, look at LWG issue #2464,

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

EricWF added a comment. Alright, so this is looking really good. Once we land this would you be interested in applying the same optimization to `emplace` calls? Comment at: include/__hash_table:863 @@ -862,2 +862,3 @@ _LIBCPP_INLINE_VISIBILITY -pair

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

dexonsmith added a subscriber: dexonsmith. dexonsmith added a comment. Great, I should have time to clean this up tomorrow afternoon. Regarding emplace, this patch as is has tests for emplace, but they depend on r258575 being in tree. You asked me to revert that... I'll wait for your response

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

Great, I should have time to clean this up tomorrow afternoon. Regarding emplace, this patch as is has tests for emplace, but they depend on r258575 being in tree. You asked me to revert that... I'll wait for your response over in that thread:

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

EricWF added a comment. In http://reviews.llvm.org/D16360#334781, @dexonsmith wrote: > Great, I should have time to clean this up tomorrow afternoon. > > Regarding emplace, this patch as is has tests for emplace, but > they depend on r258575 being in tree. You asked me to revert > that...

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

> On 2016-Jan-21, at 23:14, Eric Fiselier wrote: > > EricWF added a comment. > >> - Did I successfully match the coding style? (I'm kind of lost without >> clang-format TBH.) > > > The style looks pretty good. I'll comment on any nits I have. > >> - Should I separate the

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

I'll upload a new patch in a moment. Replies inline below. > On 2016-Jan-21, at 23:12, Eric Fiselier 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. > >

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

dexonsmith updated this revision to Diff 45758. dexonsmith added a comment. Committed r258511 and r258575 as preps. Updated patch addresses review comments (and skips the trivially constructible parts). http://reviews.llvm.org/D16360 Files: include/__hash_table include/unordered_map

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

EricWF added a reviewer: mclow.lists. EricWF added a subscriber: mclow.lists. EricWF added a comment. Adding @mclow.lists as a reviewer. http://reviews.llvm.org/D16360 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

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

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. - It

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

EricWF added a comment. > - Did I successfully match the coding style? (I'm kind of lost without > clang-format TBH.) The style looks pretty good. I'll comment on any nits I have. > - Should I separate the change to __construct_node_hash() into a separate > prep commit? (I would if this were

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

dexonsmith created this revision. dexonsmith added a reviewer: EricWF. dexonsmith added a subscriber: cfe-commits. (Repost of: http://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20160118/147379.html on Phabricator as requested by Eric.) This is a follow-up to r239666: "Fix PR12999 -