This is an archive of the discontinued LLVM Phabricator instance.

[2/2] Fix complexity of map insert_or_assign with a hint.
ClosedPublic

Authored by Mordante on Jun 1 2019, 9:30 AM.

Details

Summary

In bug 38722 Mitsuru Kariya reported the map operations insert_or_assign with a hint violates the complexity requirement. The function no longer uses a lower_bound, which caused the wrong complexity.

This fixes bug 38722.

Benchmarked with:
Run on (4 X 3400 MHz CPU s)
CPU Caches:
  L1 Data 32K (x4)
  L1 Instruction 32K (x4)
  L2 Unified 256K (x4)
  L3 Unified 6144K (x1)


Timing before:
-----------------------------------------------------------------------------------------------
Benchmark                                                    Time             CPU   Iterations
-----------------------------------------------------------------------------------------------
BM_InsertAssignHint_MapSize=10_ExistingElement            6.99 ns         6.98 ns    121900000
BM_InsertAssignHint_MapSize=100_ExistingElement           14.4 ns         14.4 ns     48800000
BM_InsertAssignHint_MapSize=1000_ExistingElement          43.0 ns         43.0 ns     17000000
BM_InsertAssignHint_MapSize=10000_ExistingElement         56.5 ns         56.5 ns     11000000
BM_InsertAssignHint_MapSize=100000_ExistingElement         116 ns          116 ns      6000000
BM_InsertAssignHint_MapSize=1000000_ExistingElement        154 ns          154 ns      5000000
BM_InsertAssignHint_MapSize=10_NewElement                 28.4 ns         28.4 ns     24690000
BM_InsertAssignHint_MapSize=100_NewElement                72.4 ns         72.4 ns     12600000
BM_InsertAssignHint_MapSize=1000_NewElement               74.3 ns         74.3 ns      9000000
BM_InsertAssignHint_MapSize=10000_NewElement              92.4 ns         92.4 ns      7000000
BM_InsertAssignHint_MapSize=100000_NewElement              151 ns          151 ns      5000000
BM_InsertAssignHint_MapSize=1000000_NewElement             194 ns          194 ns      4000000

-----------------------------------------------------------------------------------------------
Benchmark                                                    Time             CPU   Iterations
-----------------------------------------------------------------------------------------------
BM_InsertAssignHint_MapSize=10_ExistingElement            6.96 ns         6.96 ns    123030000
BM_InsertAssignHint_MapSize=100_ExistingElement           14.5 ns         14.4 ns     48600000
BM_InsertAssignHint_MapSize=1000_ExistingElement          43.0 ns         43.0 ns     17000000
BM_InsertAssignHint_MapSize=10000_ExistingElement         56.7 ns         56.7 ns     11000000
BM_InsertAssignHint_MapSize=100000_ExistingElement         116 ns          116 ns      6000000
BM_InsertAssignHint_MapSize=1000000_ExistingElement        153 ns          153 ns      5000000
BM_InsertAssignHint_MapSize=10_NewElement                 28.6 ns         28.6 ns     24600000
BM_InsertAssignHint_MapSize=100_NewElement                76.2 ns         76.2 ns     12600000
BM_InsertAssignHint_MapSize=1000_NewElement               74.6 ns         74.6 ns      9000000
BM_InsertAssignHint_MapSize=10000_NewElement              92.4 ns         92.4 ns      7000000
BM_InsertAssignHint_MapSize=100000_NewElement              151 ns          151 ns      4000000
BM_InsertAssignHint_MapSize=1000000_NewElement             194 ns          194 ns      3000000

-----------------------------------------------------------------------------------------------
Benchmark                                                    Time             CPU   Iterations
-----------------------------------------------------------------------------------------------
BM_InsertAssignHint_MapSize=10_ExistingElement            6.96 ns         6.96 ns    122840000
BM_InsertAssignHint_MapSize=100_ExistingElement           14.5 ns         14.5 ns     49000000
BM_InsertAssignHint_MapSize=1000_ExistingElement          43.0 ns         43.0 ns     17000000
BM_InsertAssignHint_MapSize=10000_ExistingElement         56.4 ns         56.4 ns     11000000
BM_InsertAssignHint_MapSize=100000_ExistingElement         116 ns          116 ns      6000000
BM_InsertAssignHint_MapSize=1000000_ExistingElement        154 ns          154 ns      4000000
BM_InsertAssignHint_MapSize=10_NewElement                 28.3 ns         28.3 ns     24830000
BM_InsertAssignHint_MapSize=100_NewElement                76.1 ns         76.1 ns     12500000
BM_InsertAssignHint_MapSize=1000_NewElement               74.7 ns         74.7 ns      9000000
BM_InsertAssignHint_MapSize=10000_NewElement              92.8 ns         92.8 ns      7000000
BM_InsertAssignHint_MapSize=100000_NewElement              150 ns          150 ns      4000000
BM_InsertAssignHint_MapSize=1000000_NewElement             197 ns          197 ns      3000000

Timing after:
-----------------------------------------------------------------------------------------------
Benchmark                                                    Time             CPU   Iterations
-----------------------------------------------------------------------------------------------
BM_InsertAssignHint_MapSize=10_ExistingElement            4.30 ns         4.30 ns    167310000
BM_InsertAssignHint_MapSize=100_ExistingElement           8.18 ns         8.18 ns     85400000
BM_InsertAssignHint_MapSize=1000_ExistingElement          8.11 ns         8.11 ns     87000000
BM_InsertAssignHint_MapSize=10000_ExistingElement         8.11 ns         8.11 ns     87000000
BM_InsertAssignHint_MapSize=100000_ExistingElement        8.28 ns         8.28 ns     83000000
BM_InsertAssignHint_MapSize=1000000_ExistingElement       8.63 ns         8.62 ns     82000000
BM_InsertAssignHint_MapSize=10_NewElement                 23.5 ns         23.4 ns     30110000
BM_InsertAssignHint_MapSize=100_NewElement                63.2 ns         63.0 ns     10000000
BM_InsertAssignHint_MapSize=1000_NewElement               46.7 ns         46.6 ns     15000000
BM_InsertAssignHint_MapSize=10000_NewElement              41.8 ns         41.7 ns     13000000
BM_InsertAssignHint_MapSize=100000_NewElement             42.3 ns         42.1 ns     15000000
BM_InsertAssignHint_MapSize=1000000_NewElement            47.9 ns         47.7 ns     14000000

-----------------------------------------------------------------------------------------------
Benchmark                                                    Time             CPU   Iterations
-----------------------------------------------------------------------------------------------
BM_InsertAssignHint_MapSize=10_ExistingElement            4.30 ns         4.30 ns    167500000
BM_InsertAssignHint_MapSize=100_ExistingElement           8.39 ns         8.39 ns     84800000
BM_InsertAssignHint_MapSize=1000_ExistingElement          8.14 ns         8.15 ns     86000000
BM_InsertAssignHint_MapSize=10000_ExistingElement         8.12 ns         8.12 ns     87000000
BM_InsertAssignHint_MapSize=100000_ExistingElement        8.37 ns         8.37 ns     85000000
BM_InsertAssignHint_MapSize=1000000_ExistingElement       8.61 ns         8.61 ns     82000000
BM_InsertAssignHint_MapSize=10_NewElement                 23.4 ns         23.4 ns     29830000
BM_InsertAssignHint_MapSize=100_NewElement                66.8 ns         66.8 ns     10000000
BM_InsertAssignHint_MapSize=1000_NewElement               46.6 ns         46.6 ns     14000000
BM_InsertAssignHint_MapSize=10000_NewElement              41.3 ns         41.4 ns     14000000
BM_InsertAssignHint_MapSize=100000_NewElement             41.7 ns         41.7 ns     14000000
BM_InsertAssignHint_MapSize=1000000_NewElement            48.5 ns         48.5 ns     11000000

-----------------------------------------------------------------------------------------------
Benchmark                                                    Time             CPU   Iterations
-----------------------------------------------------------------------------------------------
BM_InsertAssignHint_MapSize=10_ExistingElement            4.27 ns         4.27 ns    166850000
BM_InsertAssignHint_MapSize=100_ExistingElement           8.53 ns         8.53 ns     85100000
BM_InsertAssignHint_MapSize=1000_ExistingElement          8.31 ns         8.31 ns     86000000
BM_InsertAssignHint_MapSize=10000_ExistingElement         8.17 ns         8.18 ns     86000000
BM_InsertAssignHint_MapSize=100000_ExistingElement        8.73 ns         8.73 ns     82000000
BM_InsertAssignHint_MapSize=1000000_ExistingElement       8.98 ns         8.98 ns     81000000
BM_InsertAssignHint_MapSize=10_NewElement                 24.6 ns         24.6 ns     29720000
BM_InsertAssignHint_MapSize=100_NewElement                71.9 ns         71.9 ns     10000000
BM_InsertAssignHint_MapSize=1000_NewElement               46.1 ns         46.1 ns     13000000
BM_InsertAssignHint_MapSize=10000_NewElement              41.7 ns         41.8 ns     14000000
BM_InsertAssignHint_MapSize=100000_NewElement             42.1 ns         42.1 ns     13000000
BM_InsertAssignHint_MapSize=1000000_NewElement            48.6 ns         48.6 ns     13000000

Diff Detail

Event Timeline

Mordante created this revision.Jun 1 2019, 9:30 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 1 2019, 9:30 AM
Herald added a subscriber: christof. · View Herald Transcript

You're going to need several tests here.

Hints that are correct.
Hints that are wrong.
Hints that are end() <-- All the tests we have pass end(), as far as I can tell.
See "test/std/containers/associative/map/map.modifiers/insert_node_type_hint.pass.cpp"

It seems there already is a unit test "test/std/containers/associative/map/map.modifiers/insert_or_assign.pass.cpp". I'll add extra tests to this file.

ldionne requested changes to this revision.Mar 3 2020, 12:52 PM

@Mordante Ping! I'm willing to look at this with added tests.

This revision now requires changes to proceed.Mar 3 2020, 12:52 PM

Thanks for looking into these patches!

I thought I already had added the unit test, but seem I forgot to update the diff.

Mordante updated this revision to Diff 249015.Mar 8 2020, 11:22 AM

Added unit tests as requested and rebased against master.

ldionne requested changes to this revision.Sep 15 2020, 9:59 AM
ldionne added inline comments.
libcxx/include/map
1285

I think it would be better if __emplace_hint_unique_key_args returned a pair of bool and iterator, like __emplace_unique_key_args does. We could then reuse it to implement this functionality. WDYT?

This revision now requires changes to proceed.Sep 15 2020, 9:59 AM

I'll rebase the patch and then have a look at your suggestion.

Mordante marked an inline comment as done.Sep 16 2020, 11:17 AM
Mordante added inline comments.
libcxx/include/map
1285

I think it could work something like:

auto [__r, __inserted] = __tree_.__emplace_hint_unique_key_args(     
    __h.__i_, __k, _VSTD::move(__k), _VSTD::forward<_Vp>(__v));           
  
if (!__inserted)                                                          
  __r->__get_value().second = _VSTD::forward<_Vp>(__v);              

return __r;

I'm not fond of using __k and moving it in the same function call, but try_emplace(key_type&& __k, _Args&&... __args) uses the same method for the key. Since it's safe here and I see no reason why it would become unsafe in the future I don't mind.

I like that this approach keeps the header file smaller so I'll test this approach further.

ldionne added a reviewer: Restricted Project.Sep 16 2020, 11:34 AM
ldionne added inline comments.
libcxx/include/map
1285

Excellent. I like that it avoids code duplication, but I agree the move is a bit sneaky. Let's use that approach, though.

Mordante updated this revision to Diff 292562.Sep 17 2020, 10:47 AM
Mordante marked an inline comment as done.

Improves the code based on @ldionne's suggestion

  • __emplace_hint_unique_key_args returns a pair
  • insert_or_assign uses __emplace_hint_unique_key_args internally
  • removes the now unused code from the initial patch
  • removes unused #ifdefs for __emplace_hint_unique_key_args, they seem a copy paste and the code isn't used for C++03
ldionne requested changes to this revision.Sep 17 2020, 11:34 AM

This looks really good, thanks! Just one comment about structured bindings, and I think we're good to go.

libcxx/include/map
1276

You can't use structured bindings here, this is supposed to work before C++17.

This revision now requires changes to proceed.Sep 17 2020, 11:34 AM
Mordante marked an inline comment as done.Sep 17 2020, 11:54 AM
Mordante added inline comments.
libcxx/include/map
1276

I used the structured bindings since the code is ifdef'ed at line 1204 with #if _LIBCPP_STD_VER > 14 so I assumed it wasn't intended to be used prior to C++17. But no problem to use a pair.

ldionne accepted this revision.Sep 17 2020, 11:56 AM

LGTM, please:

  1. Reduce your commit message to get rid of the benchmark output
  2. Run: for std in c++03 c++11 c++14 c++17 c++2a; do <build-dir>/bin/llvm-lit -sv libcxx/test/std/containers/associative/map --param std=$std; done to make sure it all works

And the commit. Yay!

libcxx/include/map
1276

Oh, my apologies. In that case that LGTM.

This revision is now accepted and ready to land.Sep 17 2020, 11:56 AM
Mordante marked an inline comment as done.Sep 19 2020, 7:29 AM

Thanks for the review!

LGTM, please:

  1. Run: for std in c++03 c++11 c++14 c++17 c++2a; do <build-dir>/bin/llvm-lit -sv libcxx/test/std/containers/associative/map --param std=$std; done to make sure it all works

Thanks for this hint, clang tests al required C++ versions without overriding the std. It turns out the removed C++03 was required so I restored it before committing.

This revision was automatically updated to reflect the committed changes.