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: disentangle Drop implementation from IntoIter #81486

Merged
merged 1 commit into from
Feb 12, 2021

Conversation

ssomers
Copy link
Contributor

@ssomers ssomers commented Jan 28, 2021

No longer require every BTreeMap to dig up its last leaf edge before dying. This speeds up the clone_ benchmarks by 25% for normal keys and values (far less for huge values).

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 28, 2021
@ssomers ssomers force-pushed the btree_separate_drop branch from bd0f6de to 0faf35a Compare January 29, 2021 12:32
@ssomers
Copy link
Contributor Author

ssomers commented Jan 29, 2021

Last commit conforms to #81073, dismisses two ManualDrop, introduces finalize.

@ssomers ssomers force-pushed the btree_separate_drop branch 2 times, most recently from bd5b605 to b6012c3 Compare January 31, 2021 15:21
@ssomers
Copy link
Contributor Author

ssomers commented Jan 31, 2021

I wasn't happy with the rather primitive code in finalize that belongs in navigate.rs, so ended up slightly rearranging the deallocating code. Now the benchmarks are more spectacular, though not for huge values:

benchcmp old new --threshold 10
 name                                           old ns/iter  new ns/iter  diff ns/iter   diff %  speedup
 btree::map::clone_fat_100                      45,646       72,143             26,497   58.05%   x 0.63
 btree::map::clone_fat_100_and_clear            45,567       81,445             35,878   78.74%   x 0.56
 btree::map::clone_fat_100_and_drain_half       106,640      124,207            17,567   16.47%   x 0.86
 btree::map::clone_fat_val_100                  23,640       38,426             14,786   62.55%   x 0.62
 btree::map::clone_fat_val_100_and_clear        23,687       41,178             17,491   73.84%   x 0.58
 btree::map::clone_fat_val_100_and_drain_half   48,727       57,688              8,961   18.39%   x 0.84
 btree::map::clone_fat_val_100_and_into_iter    34,402       41,846              7,444   21.64%   x 0.82
 btree::map::clone_fat_val_100_and_remove_half  52,027       61,435              9,408   18.08%   x 0.85
 btree::map::clone_slim_100                     2,184        1,696                -488  -22.34%   x 1.29
 btree::map::clone_slim_100_and_clear           5,088        1,670              -3,418  -67.18%   x 3.05
 btree::map::clone_slim_100_and_drain_all       6,518        3,560              -2,958  -45.38%   x 1.83
 btree::map::clone_slim_100_and_drain_half      5,963        2,722              -3,241  -54.35%   x 2.19
 btree::map::clone_slim_100_and_into_iter       5,100        1,655              -3,445  -67.55%   x 3.08
 btree::map::clone_slim_100_and_pop_all         6,765        3,803              -2,962  -43.78%   x 1.78
 btree::map::clone_slim_100_and_remove_all      7,826        4,944              -2,882  -36.83%   x 1.58
 btree::map::clone_slim_100_and_remove_half     6,131        3,031              -3,100  -50.56%   x 2.02
 btree::map::clone_slim_10k                     262,765      201,860           -60,905  -23.18%   x 1.30
 btree::map::clone_slim_10k_and_clear           262,713      204,480           -58,233  -22.17%   x 1.28
 btree::map::clone_slim_10k_and_into_iter       261,446      206,657           -54,789  -20.96%   x 1.27
 btree::map::clone_slim_10k_and_remove_half     574,250      512,200           -62,050  -10.81%   x 1.12
 btree::map::find_seq_100                       15           12                     -3  -20.00%   x 1.25
 btree::map::first_and_last_100                 47           42                     -5  -10.64%   x 1.12
 btree::set::clone_100                          1,883        1,352                -531  -28.20%   x 1.39
 btree::set::clone_100_and_clear                1,920        1,384                -536  -27.92%   x 1.39
 btree::set::clone_100_and_into_iter            1,928        1,410                -518  -26.87%   x 1.37
 btree::set::clone_100_and_remove_half          2,685        2,248                -437  -16.28%   x 1.19
 btree::set::clone_10k                          225,550      157,252           -68,298  -30.28%   x 1.43
 btree::set::clone_10k_and_clear                224,802      165,220           -59,582  -26.50%   x 1.36
 btree::set::clone_10k_and_into_iter            224,367      167,968           -56,399  -25.14%   x 1.34
 btree::set::clone_10k_and_remove_half          501,320      447,430           -53,890  -10.75%   x 1.12
 btree::set::difference_random_100_vs_10k       2,261        2,830                 569   25.17%   x 0.80
 btree::set::difference_staggered_100_vs_10k    2,804        2,150                -654  -23.32%   x 1.30
 btree::set::intersection_random_100_vs_10k     2,136        2,650                 514   24.06%   x 0.81
 btree::set::intersection_random_10k_vs_100     2,163        2,576                 413   19.09%   x 0.84
 btree::set::intersection_staggered_100_vs_10k  2,597        1,980                -617  -23.76%   x 1.31
 btree::set::is_subset_100_vs_10k               1,619        1,182                -437  -26.99%   x 1.37

@bors
Copy link
Contributor

bors commented Feb 7, 2021

☔ The latest upstream changes (presumably #81853) made this pull request unmergeable. Please resolve the merge conflicts.

@ssomers ssomers force-pushed the btree_separate_drop branch from fd9918e to 4230d99 Compare February 7, 2021 19:07
@Mark-Simulacrum
Copy link
Member

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

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

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Feb 9, 2021
@bors
Copy link
Contributor

bors commented Feb 9, 2021

⌛ Trying commit 4230d999ee1d76ab09380e172dc418f9f925be6b with merge c3d1be590b1ba9ceb83fe3737f5cf2e767f72377...

@bors
Copy link
Contributor

bors commented Feb 9, 2021

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

@rust-timer
Copy link
Collaborator

Queued c3d1be590b1ba9ceb83fe3737f5cf2e767f72377 with parent 0fc6756, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (c3d1be590b1ba9ceb83fe3737f5cf2e767f72377): 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 Feb 9, 2021
@ssomers ssomers force-pushed the btree_separate_drop branch from 4230d99 to 3045b75 Compare February 9, 2021 12:55
@ssomers
Copy link
Contributor Author

ssomers commented Feb 9, 2021

Rebased and tweaked a few comments.

@Mark-Simulacrum
Copy link
Member

@bors r+

Performance seems largely "noise" I guess, hard to say precisely. Change seems good from sort of "first principles" and I like that it removes the macros, at least partially; that seems like a win.

@bors
Copy link
Contributor

bors commented Feb 12, 2021

📌 Commit 3045b75 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 Feb 12, 2021
@bors
Copy link
Contributor

bors commented Feb 12, 2021

⌛ Testing commit 3045b75 with merge a118ee2...

@bors
Copy link
Contributor

bors commented Feb 12, 2021

☀️ Test successful - checks-actions
Approved by: Mark-Simulacrum
Pushing a118ee2 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Feb 12, 2021
@bors bors merged commit a118ee2 into rust-lang:master Feb 12, 2021
@rustbot rustbot added this to the 1.52.0 milestone Feb 12, 2021
@ssomers ssomers deleted the btree_separate_drop branch February 12, 2021 10:15
ssomers added a commit to ssomers/rust that referenced this pull request Aug 2, 2021
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 16, 2021
…r=Mark-Simulacrum

BTree: merge the complication introduced by rust-lang#81486 and rust-lang#86031

Also:
- Deallocate the last few tree nodes as soon as an `into_iter` iterator steps beyond the end, instead of waiting around for the drop of the iterator (just to share more code).
- Symmetric code for backward iteration.
- Mark unsafe the methods on dying handles, modelling dying handles after raw pointers: it's the caller's responsibility to use them safely.

r? `@Mark-Simulacrum`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. 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.

7 participants