Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BTreeMap: prefer bulk_steal functions over specialized ones #81115

Merged
merged 1 commit into from
Jan 19, 2021

Conversation

ssomers
Copy link
Contributor

@ssomers ssomers commented Jan 17, 2021

The steal_ functions (apart from their return value) are basically specializations of the more general bulk_steal_ functions. This PR removes the specializations. The library/alloc benchmarks say this is never slower and up to 6% faster.

r? @Mark-Simulacrum

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 17, 2021
@nagisa
Copy link
Member

nagisa commented Jan 17, 2021

Can you post some benchmark results?

@nagisa
Copy link
Member

nagisa commented Jan 17, 2021

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@bors
Copy link
Contributor

bors commented Jan 17, 2021

⌛ Trying commit c085f973181151124e1bf16e6c8ec01f9ef2e354 with merge 6937a6221a72897ad82bb2282e87e24eec9227e6...

@bors
Copy link
Contributor

bors commented Jan 17, 2021

☀️ Try build successful - checks-actions
Build commit: 6937a6221a72897ad82bb2282e87e24eec9227e6 (6937a6221a72897ad82bb2282e87e24eec9227e6)

@rust-timer
Copy link
Collaborator

Queued 6937a6221a72897ad82bb2282e87e24eec9227e6 with parent d51cf96, future comparison URL.

@rustbot label: +S-waiting-on-perf

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jan 17, 2021
@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (6937a6221a72897ad82bb2282e87e24eec9227e6): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jan 17, 2021
@ssomers
Copy link
Contributor Author

ssomers commented Jan 17, 2021

Can you post some benchmark results?
I can post all of them:

cargo-benchcmp.exe benchcmp old new
 name                                           old ns/iter  new ns/iter  diff ns/iter  diff %  speedup
 btree::map::clone_fat_100                      46,243       45,360               -883  -1.91%   x 1.02
 btree::map::clone_fat_100_and_clear            46,420       45,228             -1,192  -2.57%   x 1.03
 btree::map::clone_fat_100_and_drain_all        154,342      155,270               928   0.60%   x 0.99
 btree::map::clone_fat_100_and_drain_half       112,175      106,750            -5,425  -4.84%   x 1.05
 btree::map::clone_fat_100_and_into_iter        73,417       71,558             -1,859  -2.53%   x 1.03
 btree::map::clone_fat_100_and_pop_all          172,725      173,380               655   0.38%   x 1.00
 btree::map::clone_fat_100_and_remove_all       200,212      200,792               580   0.29%   x 1.00
 btree::map::clone_fat_100_and_remove_half      125,670      122,258            -3,412  -2.72%   x 1.03
 btree::map::clone_fat_val_100                  24,227       23,730               -497  -2.05%   x 1.02
 btree::map::clone_fat_val_100_and_clear        24,195       24,014               -181  -0.75%   x 1.01
 btree::map::clone_fat_val_100_and_drain_all    58,525       60,059              1,534   2.62%   x 0.97
 btree::map::clone_fat_val_100_and_drain_half   49,547       49,343               -204  -0.41%   x 1.00
 btree::map::clone_fat_val_100_and_into_iter    35,288       35,142               -146  -0.41%   x 1.00
 btree::map::clone_fat_val_100_and_pop_all      66,569       66,586                 17   0.03%   x 1.00
 btree::map::clone_fat_val_100_and_remove_all   74,548       76,977              2,429   3.26%   x 0.97
 btree::map::clone_fat_val_100_and_remove_half  55,038       52,698             -2,340  -4.25%   x 1.04
 btree::map::clone_slim_100                     2,169        2,133                 -36  -1.66%   x 1.02
 btree::map::clone_slim_100_and_clear           2,160        2,162                   2   0.09%   x 1.00
 btree::map::clone_slim_100_and_drain_all       3,562        3,533                 -29  -0.81%   x 1.01
 btree::map::clone_slim_100_and_drain_half      2,960        3,005                  45   1.52%   x 0.99
 btree::map::clone_slim_100_and_into_iter       2,171        2,146                 -25  -1.15%   x 1.01
 btree::map::clone_slim_100_and_pop_all         3,814        3,780                 -34  -0.89%   x 1.01
 btree::map::clone_slim_100_and_remove_all      4,937        4,967                  30   0.61%   x 0.99
 btree::map::clone_slim_100_and_remove_half     3,271        3,310                  39   1.19%   x 0.99
 btree::map::clone_slim_10k                     262,335      256,705            -5,630  -2.15%   x 1.02
 btree::map::clone_slim_10k_and_clear           262,770      257,716            -5,054  -1.92%   x 1.02
 btree::map::clone_slim_10k_and_drain_all       402,520      392,235           -10,285  -2.56%   x 1.03
 btree::map::clone_slim_10k_and_drain_half      367,500      359,985            -7,515  -2.04%   x 1.02
 btree::map::clone_slim_10k_and_into_iter       263,230      259,386            -3,844  -1.46%   x 1.01
 btree::map::clone_slim_10k_and_pop_all         436,470      430,215            -6,255  -1.43%   x 1.01
 btree::map::clone_slim_10k_and_remove_all      598,210      584,685           -13,525  -2.26%   x 1.02
 btree::map::clone_slim_10k_and_remove_half     561,210      539,070           -22,140  -3.95%   x 1.04
 btree::map::find_rand_100                      13           13                      0   0.00%   x 1.00
 btree::map::find_rand_10_000                   55           55                      0   0.00%   x 1.00
 btree::map::find_seq_100                       12           12                      0   0.00%   x 1.00
 btree::map::find_seq_10_000                    37           36                     -1  -2.70%   x 1.03
 btree::map::first_and_last_0                   10           10                      0   0.00%   x 1.00
 btree::map::first_and_last_100                 49           49                      0   0.00%   x 1.00
 btree::map::first_and_last_10k                 59           59                      0   0.00%   x 1.00
 btree::map::insert_rand_100                    43           44                      1   2.33%   x 0.98
 btree::map::insert_rand_10_000                 43           43                      0   0.00%   x 1.00
 btree::map::insert_seq_100                     49           50                      1   2.04%   x 0.98
 btree::map::insert_seq_10_000                  105          104                    -1  -0.95%   x 1.01
 btree::map::iter_0                             734          736                     2   0.27%   x 1.00
 btree::map::iter_1                             11,286       11,013               -273  -2.42%   x 1.02
 btree::map::iter_100                           11,755       11,757                  2   0.02%   x 1.00
 btree::map::iter_10k                           12,736       12,727                 -9  -0.07%   x 1.00
 btree::map::iter_1m                            13,712       13,731                 19   0.14%   x 1.00
 btree::map::iteration_1000                     3,479        3,428                 -51  -1.47%   x 1.01
 btree::map::iteration_100000                   499,040      499,310               270   0.05%   x 1.00
 btree::map::iteration_20                       61           65                      4   6.56%   x 0.94
 btree::map::iteration_mut_1000                 4,030        3,894                -136  -3.37%   x 1.03
 btree::map::iteration_mut_100000               537,210      537,305                95   0.02%   x 1.00
 btree::map::iteration_mut_20                   65           64                     -1  -1.54%   x 1.02
 btree::map::range_included_excluded            513,183      521,857             8,674   1.69%   x 0.98
 btree::map::range_included_included            457,003      438,920           -18,083  -3.96%   x 1.04
 btree::map::range_included_unbounded           153,795      147,034            -6,761  -4.40%   x 1.05
 btree::map::range_unbounded_unbounded          78,632       76,754             -1,878  -2.39%   x 1.02
 btree::map::range_unbounded_vs_iter            125,692      123,040            -2,652  -2.11%   x 1.02
 btree::set::clone_100                          1,891        1,865                 -26  -1.37%   x 1.01
 btree::set::clone_100_and_clear                1,892        1,869                 -23  -1.22%   x 1.01
 btree::set::clone_100_and_drain_all            3,009        2,952                 -57  -1.89%   x 1.02
 btree::set::clone_100_and_drain_half           2,579        2,483                 -96  -3.72%   x 1.04
 btree::set::clone_100_and_into_iter            1,891        1,858                 -33  -1.75%   x 1.02
 btree::set::clone_100_and_pop_all              2,617        2,607                 -10  -0.38%   x 1.00
 btree::set::clone_100_and_remove_all           3,531        3,605                  74   2.10%   x 0.98
 btree::set::clone_100_and_remove_half          2,568        2,522                 -46  -1.79%   x 1.02
 btree::set::clone_10k                          205,352      219,355            14,003   6.82%   x 0.94
 btree::set::clone_10k_and_clear                216,875      219,880             3,005   1.39%   x 0.99
 btree::set::clone_10k_and_drain_all            325,203      324,480              -723  -0.22%   x 1.00
 btree::set::clone_10k_and_drain_half           303,230      297,323            -5,907  -1.95%   x 1.02
 btree::set::clone_10k_and_into_iter            215,932      217,895             1,963   0.91%   x 0.99
 btree::set::clone_10k_and_pop_all              297,830      299,150             1,320   0.44%   x 1.00
 btree::set::clone_10k_and_remove_all           436,555      442,740             6,185   1.42%   x 0.99
 btree::set::clone_10k_and_remove_half          466,705      462,025            -4,680  -1.00%   x 1.01
 btree::set::difference_random_100_vs_100       837          874                    37   4.42%   x 0.96
 btree::set::difference_random_100_vs_10k       3,498        3,279                -219  -6.26%   x 1.07
 btree::set::difference_random_10k_vs_100       60,975       64,318              3,343   5.48%   x 0.95
 btree::set::difference_random_10k_vs_10k       192,414      191,706              -708  -0.37%   x 1.00
 btree::set::difference_staggered_100_vs_100    873          911                    38   4.35%   x 0.96
 btree::set::difference_staggered_100_vs_10k    2,210        2,216                   6   0.27%   x 1.00
 btree::set::difference_staggered_10k_vs_10k    84,267       87,997              3,730   4.43%   x 0.96
 btree::set::intersection_100_neg_vs_100_pos    17           16                     -1  -5.88%   x 1.06
 btree::set::intersection_100_neg_vs_10k_pos    18           17                     -1  -5.56%   x 1.06
 btree::set::intersection_100_pos_vs_100_neg    17           16                     -1  -5.88%   x 1.06
 btree::set::intersection_100_pos_vs_10k_neg    18           17                     -1  -5.56%   x 1.06
 btree::set::intersection_10k_neg_vs_100_pos    18           17                     -1  -5.56%   x 1.06
 btree::set::intersection_10k_neg_vs_10k_pos    19           18                     -1  -5.26%   x 1.06
 btree::set::intersection_10k_pos_vs_100_neg    18           17                     -1  -5.56%   x 1.06
 btree::set::intersection_10k_pos_vs_10k_neg    19           18                     -1  -5.26%   x 1.06
 btree::set::intersection_random_100_vs_100     636          626                   -10  -1.57%   x 1.02
 btree::set::intersection_random_100_vs_10k     3,333        3,060                -273  -8.19%   x 1.09
 btree::set::intersection_random_10k_vs_100     3,336        3,035                -301  -9.02%   x 1.10
 btree::set::intersection_random_10k_vs_10k     157,746      160,686             2,940   1.86%   x 0.98
 btree::set::intersection_staggered_100_vs_100  611          607                    -4  -0.65%   x 1.01
 btree::set::intersection_staggered_100_vs_10k  2,005        2,010                   5   0.25%   x 1.00
 btree::set::intersection_staggered_10k_vs_10k  60,707       61,326                619   1.02%   x 0.99
 btree::set::is_subset_100_vs_100               499          499                     0   0.00%   x 1.00
 btree::set::is_subset_100_vs_10k               1,999        1,821                -178  -8.90%   x 1.10
 btree::set::is_subset_10k_vs_100               2            2                       0   0.00%   x 1.00
 btree::set::is_subset_10k_vs_10k               58,491       58,305               -186  -0.32%   x 1.00

The benchmarks exercising the steal functions are the clone_*_and_drain_* and clone_*_and_remove_*, where you should subtract the clone_* baseline to see the actual difference. There's nothing significant, just a faint scent of improvement.

@Mark-Simulacrum
Copy link
Member

r=me with conflict resolved.

FWIW, it looks like steal is maybe not a great name for these functions - I have usually seen them called left/right rotations. (The intuition is that the elements on the left node go up to the parent and the parent goes down to the right child, i.e., rotating in some sense). Steal to me implies a return type containing key/value pairs but it's not the case here.

@ssomers
Copy link
Contributor Author

ssomers commented Jan 18, 2021

r=me with conflict resolved.

I didn't get notification of that conflict, and worse I didn't realize it was the same pop as the comment tweak in #81082, which got there because the same pop is needed for drain. So it's possible this returns in #81075.

FWIW, it looks like steal is maybe not a great name for these functions

I kind of like the name. I would have probably chosen a boring verb like "move" had I invented them.

Steal to me implies a return type containing key/value pairs but it's not the case here.

I only know the term from work stealing in scheduling, and it never implied any return type to me. But not you say that, it does remind me of one API long ago where "get_x" actually stole x. There "get" meant what it means in English: take, grab, seize, steal.

@Mark-Simulacrum
Copy link
Member

I think "move_left" or "steal_left" both imply to me action solely taken on the left node. left rotate (note: not rotate left) somewhat does not.

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Jan 18, 2021

📌 Commit 4775334 has been approved by Mark-Simulacrum

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 18, 2021
@ssomers
Copy link
Contributor Author

ssomers commented Jan 18, 2021

I did tentatively rename it to "steal_left_to_right" earlier, because I was confused by the direction: does it steal stuff from the left sibling, to be made the left part of the node, or does it steal the left part and leave the loot in the right sibling (after exchanging some of the loot in the middle to shake off the cops).

m-ou-se added a commit to m-ou-se/rust that referenced this pull request Jan 18, 2021
…Mark-Simulacrum

BTreeMap: prefer bulk_steal functions over specialized ones

The `steal_` functions (apart from their return value) are basically specializations of the more general `bulk_steal_` functions. This PR removes the specializations. The library/alloc benchmarks say this is never slower and up to 6% faster.

r? `@Mark-Simulacrum`
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 19, 2021
…laumeGomez

Rollup of 8 pull requests

Successful merges:

 - rust-lang#80382 (Improve search result tab handling)
 - rust-lang#81112 (Remove unused alloc::std::ops re-export.)
 - rust-lang#81115 (BTreeMap: prefer bulk_steal functions over specialized ones)
 - rust-lang#81147 (Fix structured suggestion for explicit `drop` call)
 - rust-lang#81161 (Remove inline script tags)
 - rust-lang#81164 (Fix typo in simplify.rs)
 - rust-lang#81166 (remove some outdated comments regarding  debug assertions)
 - rust-lang#81168 (Fixes rust-lang#81109 - Typo in pointer::wrapping_sub)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 6af6c40 into rust-lang:master Jan 19, 2021
@rustbot rustbot added this to the 1.51.0 milestone Jan 19, 2021
@ssomers ssomers deleted the btree_drainy_refactor_4 branch January 19, 2021 16:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants