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

Hard way to respect BTreeMap's minimum node length #75105

Merged
merged 1 commit into from
Aug 13, 2020

Conversation

ssomers
Copy link
Contributor

@ssomers ssomers commented Aug 3, 2020

Resolves #74834 the hard way (though not the hardest imaginable).

Benchmarks (which are all biased/realistic, inserting keys in ascending order) say:

benchcmp r0 r1 --threshold 10
 name                                        r0 ns/iter  r1 ns/iter  diff ns/iter   diff %  speedup
 btree::map::clone_slim_100_and_clear        2,183       2,723                540   24.74%   x 0.80
 btree::map::clone_slim_100_and_drain_all    3,652       4,173                521   14.27%   x 0.88
 btree::map::clone_slim_100_and_drain_half   3,320       3,940                620   18.67%   x 0.84
 btree::map::clone_slim_100_and_into_iter    2,154       2,717                563   26.14%   x 0.79
 btree::map::clone_slim_100_and_pop_all      3,372       3,870                498   14.77%   x 0.87
 btree::map::clone_slim_100_and_remove_all   5,111       5,647                536   10.49%   x 0.91
 btree::map::clone_slim_100_and_remove_half  3,259       3,821                562   17.24%   x 0.85
 btree::map::iter_0                          1,733       1,509               -224  -12.93%   x 1.15
 btree::map::iter_100                        2,714       3,739              1,025   37.77%   x 0.73
 btree::map::iter_10k                        3,728       4,269                541   14.51%   x 0.87
 btree::map::range_unbounded_unbounded       28,426      36,631             8,205   28.86%   x 0.78
 btree::map::range_unbounded_vs_iter         28,808      34,056             5,248   18.22%   x 0.85

This difference is not caused by the debug_assert-related code in the function splitpoint, it's the same without.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 3, 2020
@ssomers
Copy link
Contributor Author

ssomers commented Aug 3, 2020

Flabbergasted... not that I didn't immediately realize that 3 lines of code implement the 2nd rule in #74834 and it was worth a try, but that this extra code (covering a case that I think isn't benchmarked) restores performance:

benchcmp r0 r4qua --threshold 10
 name                                   r0 ns/iter  r4qua ns/iter  diff ns/iter   diff %  speedup
 btree::map::iter_0                     1,760       1,509                  -251  -14.26%   x 1.17
 btree::map::iter_100                   2,722       3,671                   949   34.86%   x 0.74
 btree::map::iter_10k                   3,737       4,269                   532   14.24%   x 0.88
 btree::map::range_unbounded_unbounded  28,928      37,173                8,245   28.50%   x 0.78
 btree::map::range_unbounded_vs_iter    28,808      38,526                9,718   33.73%   x 0.75

I benchmarked things again, and get the same relative results (though all the clone tests are 10% slower than they were 6 hours ago, before I rebooted my PC). So now this seems like a viable PR to me.

@ssomers ssomers marked this pull request as ready for review August 3, 2020 19:52
@bors
Copy link
Contributor

bors commented Aug 11, 2020

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

@Mark-Simulacrum
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Aug 13, 2020

📌 Commit 17ab457 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 Aug 13, 2020
@Mark-Simulacrum
Copy link
Member

@bors rollup=never

@bors
Copy link
Contributor

bors commented Aug 13, 2020

⌛ Testing commit 17ab457 with merge 81dc88f...

@bors
Copy link
Contributor

bors commented Aug 13, 2020

☀️ Test successful - checks-actions, checks-azure
Approved by: Mark-Simulacrum
Pushing 81dc88f to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 13, 2020
@bors bors merged commit 81dc88f into rust-lang:master Aug 13, 2020
@ssomers ssomers deleted the btree_respect_min_len_hard branch August 14, 2020 09:48
@cuviper cuviper added this to the 1.47.0 milestone May 2, 2024
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.

BTreeMap insert does not honor the B-tree invariant
5 participants