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

BTree: restore 64 bit initialization speed through fieldial distancing #82247

Closed
wants to merge 1 commit into from

Conversation

ssomers
Copy link
Contributor

@ssomers ssomers commented Feb 18, 2021

Second pass of #82226 to counter #81494.

r? @Mark-Simulacrum

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

ssomers commented Feb 18, 2021

benchcmp old new --threshold 5
 name                                           old ns/iter  new ns/iter  diff ns/iter   diff %  speedup
 btree::map::clone_fat_100_and_remove_all       190,495      179,645           -10,850   -5.70%   x 1.06
 btree::map::clone_slim_10k_and_remove_half     517,620      487,380           -30,240   -5.84%   x 1.06
 btree::map::find_rand_10_000                   59           56                     -3   -5.08%   x 1.05
 btree::map::find_seq_100                       13           12                     -1   -7.69%   x 1.08
 btree::map::find_seq_10_000                    41           35                     -6  -14.63%   x 1.17
 btree::map::first_and_last_0                   12           10                     -2  -16.67%   x 1.20
 btree::map::first_and_last_100                 41           44                      3    7.32%   x 0.93
 btree::map::first_and_last_10k                 69           60                     -9  -13.04%   x 1.15
 btree::map::iteration_1000                     4,033        4,356                 323    8.01%   x 0.93
 btree::map::iteration_100000                   514,610      473,460           -41,150   -8.00%   x 1.09
 btree::map::iteration_mut_100000               532,180      484,570           -47,610   -8.95%   x 1.10
 btree::map::range_included_included            507,554      472,265           -35,289   -6.95%   x 1.07
 btree::set::clone_100                          1,373        1,443                  70    5.10%   x 0.95
 btree::set::clone_10k_and_remove_half          468,700      439,195           -29,505   -6.30%   x 1.07
 btree::set::difference_random_100_vs_10k       3,157        2,441                -716  -22.68%   x 1.29
 btree::set::difference_staggered_100_vs_10k    2,297        2,836                 539   23.47%   x 0.81
 btree::set::intersection_random_100_vs_10k     2,823        2,276                -547  -19.38%   x 1.24
 btree::set::intersection_random_10k_vs_100     2,860        2,238                -622  -21.75%   x 1.28
 btree::set::intersection_staggered_100_vs_10k  2,024        2,669                 645   31.87%   x 0.76
 btree::set::intersection_staggered_10k_vs_10k  71,858       76,383              4,525    6.30%   x 0.94
 btree::set::is_subset_100_vs_100               578          713                   135   23.36%   x 0.81
 btree::set::is_subset_100_vs_10k               1,559        1,212                -347  -22.26%   x 1.29
 btree::set::is_subset_10k_vs_10k               59,541       71,046             11,505   19.32%   x 0.84

@cuviper
Copy link
Member

cuviper commented Feb 19, 2021

@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 19, 2021
@bors
Copy link
Contributor

bors commented Feb 19, 2021

⌛ Trying commit f61a84a with merge 51dfc29565d20b938c4c0206cf41f40215eec0f9...

@bors
Copy link
Contributor

bors commented Feb 20, 2021

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

@rust-timer
Copy link
Collaborator

Queued 51dfc29565d20b938c4c0206cf41f40215eec0f9 with parent 9b471a3, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (51dfc29565d20b938c4c0206cf41f40215eec0f9): 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 20, 2021
@Mark-Simulacrum
Copy link
Member

Benchmark results (both direct and via perf) don't look exactly positive to me; do you feel differently? That seems indicative of this not actually addressing the problem. Ultimately the regression on #81494 doesn't seem bad enough to justify complexity like this PR adds. I am inclined to close, personally, and stop investigating further - it seems like the differences are subtle and likely not readily explainable.

@Mark-Simulacrum Mark-Simulacrum added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 20, 2021
@ssomers
Copy link
Contributor Author

ssomers commented Feb 21, 2021

I don't know what these comparisons represent, what you're looking at, and I only ever see changes within the 1-3% variance cited in the report. The comparison for this PR looks even more bland than the others I see passing by.

In the microbenchmarks, on my machine, avoiding u16 does have a measurable effect. By the way, shrinking to u8 helps equally, which would have made this PR much simpler

Obviously avoiding the problem in btree doesn't actually address it. Assuming that there is a problem, that there is no intention to generate different instructions targeting stack or heap, and that the different instructions are a genuine hindrance on other CPUs than mine.

@ssomers ssomers closed this Feb 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants