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

Optimize BinaryHeap bounds checking #36072

Merged
merged 1 commit into from
Sep 3, 2016
Merged

Conversation

arthurprs
Copy link
Contributor

I was experimenting with d-ary binary heaps during the weekend (dead end) and I found that we could get some good improvements by removing bounds checking. Specially due to the panic-safe additional code, llvm can't really optimize them out.

 name                         d_ary_heap:: ns/iter  std___heap:: ns/iter  diff ns/iter  diff % 
 bench_build_insert           148,610               236,960                     88,350  59.45% 
 bench_from_vec               243,846               299,719                     55,873  22.91% 
 bench_insert_2000_empty      4,512                 7,517                        3,005  66.60% 
 bench_insert_2000_prefilled  28,665                32,605                       3,940  13.74% 
 bench_pop_2000               111,515               128,005                     16,490  14.79% 
 bench_pop_all                2,759,945             3,317,626                  557,681  20.21% 
 peek_mut                     23,186                23,635                         449   1.94% 
 pop_modify_push              41,573                43,822                       2,249   5.41%


test d_ary_heap::bench_build_insert          ... bench:     148,610 ns/iter (+/- 10,687)
test d_ary_heap::bench_from_vec              ... bench:     243,846 ns/iter (+/- 16,500)
test d_ary_heap::bench_insert_2000_empty     ... bench:       4,512 ns/iter (+/- 136)
test d_ary_heap::bench_insert_2000_prefilled ... bench:      28,665 ns/iter (+/- 1,347)
test d_ary_heap::bench_pop_2000              ... bench:     111,515 ns/iter (+/- 104,677)
test d_ary_heap::bench_pop_all               ... bench:   2,759,945 ns/iter (+/- 173,838)
test d_ary_heap::peek_mut                    ... bench:      23,186 ns/iter (+/- 106,254)
test d_ary_heap::pop_modify_push             ... bench:      41,573 ns/iter (+/- 3,313)
test std___heap::bench_build_insert          ... bench:     236,960 ns/iter (+/- 16,955)
test std___heap::bench_from_vec              ... bench:     299,719 ns/iter (+/- 6,354)
test std___heap::bench_insert_2000_empty     ... bench:       7,517 ns/iter (+/- 372)
test std___heap::bench_insert_2000_prefilled ... bench:      32,605 ns/iter (+/- 2,433)
test std___heap::bench_pop_2000              ... bench:     128,005 ns/iter (+/- 11,787)
test std___heap::bench_pop_all               ... bench:   3,317,626 ns/iter (+/- 238,968)
test std___heap::peek_mut                    ... bench:      23,635 ns/iter (+/- 1,420)
test std___heap::pop_modify_push             ... bench:      43,822 ns/iter (+/- 3,788)

Test code: https://github.com/arthurprs/heap-experiments

@rust-highfive
Copy link
Collaborator

r? @aturon

(rust_highfive has picked a reviewer for you, use r? to override)

@Aatch
Copy link
Contributor

Aatch commented Aug 28, 2016

Could you add some comments explaining why the code is safe. As in, why we can assume the index is valid? The comments on the functions themselves should probably reflect the new preconditions and be updated since they won't panic in those cases now.

@arthurprs
Copy link
Contributor Author

Updated the PR with @Aatch suggestions.

I also took the liberty of annotating the rest of Hole<> functions with inline and removing the always, this looks cleaner and more consistent.

@Aatch
Copy link
Contributor

Aatch commented Aug 31, 2016

@bors r+

@bors
Copy link
Contributor

bors commented Aug 31, 2016

📌 Commit 175d434 has been approved by Aatch

@bors
Copy link
Contributor

bors commented Sep 2, 2016

⌛ Testing commit 175d434 with merge abc03ef...

@bors
Copy link
Contributor

bors commented Sep 2, 2016

💔 Test failed - auto-win-gnu-32-opt-rustbuild

@alexcrichton
Copy link
Member

@bors: retry

On Fri, Sep 2, 2016 at 8:02 AM, bors notifications@github.com wrote:

💔 Test failed - auto-win-gnu-32-opt-rustbuild
https://buildbot.rust-lang.org/builders/auto-win-gnu-32-opt-rustbuild/builds/2375


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#36072 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAD95Epip6mm_z-13xfqsm9GRh1RQhLlks5qmDqOgaJpZM4Ju75f
.

@bors
Copy link
Contributor

bors commented Sep 2, 2016

⌛ Testing commit 175d434 with merge db57bcd...

@bors
Copy link
Contributor

bors commented Sep 2, 2016

💔 Test failed - auto-win-msvc-64-cargotest

@alexcrichton
Copy link
Member

@bors: retry

@alexcrichton
Copy link
Member

@bors: Retry

On Fri, Sep 2, 2016 at 9:13 AM, bors notifications@github.com wrote:

💔 Test failed - auto-win-msvc-64-cargotest
https://buildbot.rust-lang.org/builders/auto-win-msvc-64-cargotest/builds/1712


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#36072 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAD95NSdzh7XzwhIL08BQKyUdH-jSpskks5qmEsOgaJpZM4Ju75f
.

@bors
Copy link
Contributor

bors commented Sep 3, 2016

⌛ Testing commit 175d434 with merge 01b35d8...

bors added a commit that referenced this pull request Sep 3, 2016
Optimize BinaryHeap bounds checking

I was experimenting with d-ary binary heaps during the weekend (dead end) and I found that we could get some good improvements by removing bounds checking. Specially due to the panic-safe additional code, llvm can't really optimize them out.

```
 name                         d_ary_heap:: ns/iter  std___heap:: ns/iter  diff ns/iter  diff %
 bench_build_insert           148,610               236,960                     88,350  59.45%
 bench_from_vec               243,846               299,719                     55,873  22.91%
 bench_insert_2000_empty      4,512                 7,517                        3,005  66.60%
 bench_insert_2000_prefilled  28,665                32,605                       3,940  13.74%
 bench_pop_2000               111,515               128,005                     16,490  14.79%
 bench_pop_all                2,759,945             3,317,626                  557,681  20.21%
 peek_mut                     23,186                23,635                         449   1.94%
 pop_modify_push              41,573                43,822                       2,249   5.41%

test d_ary_heap::bench_build_insert          ... bench:     148,610 ns/iter (+/- 10,687)
test d_ary_heap::bench_from_vec              ... bench:     243,846 ns/iter (+/- 16,500)
test d_ary_heap::bench_insert_2000_empty     ... bench:       4,512 ns/iter (+/- 136)
test d_ary_heap::bench_insert_2000_prefilled ... bench:      28,665 ns/iter (+/- 1,347)
test d_ary_heap::bench_pop_2000              ... bench:     111,515 ns/iter (+/- 104,677)
test d_ary_heap::bench_pop_all               ... bench:   2,759,945 ns/iter (+/- 173,838)
test d_ary_heap::peek_mut                    ... bench:      23,186 ns/iter (+/- 106,254)
test d_ary_heap::pop_modify_push             ... bench:      41,573 ns/iter (+/- 3,313)
test std___heap::bench_build_insert          ... bench:     236,960 ns/iter (+/- 16,955)
test std___heap::bench_from_vec              ... bench:     299,719 ns/iter (+/- 6,354)
test std___heap::bench_insert_2000_empty     ... bench:       7,517 ns/iter (+/- 372)
test std___heap::bench_insert_2000_prefilled ... bench:      32,605 ns/iter (+/- 2,433)
test std___heap::bench_pop_2000              ... bench:     128,005 ns/iter (+/- 11,787)
test std___heap::bench_pop_all               ... bench:   3,317,626 ns/iter (+/- 238,968)
test std___heap::peek_mut                    ... bench:      23,635 ns/iter (+/- 1,420)
test std___heap::pop_modify_push             ... bench:      43,822 ns/iter (+/- 3,788)
```

Test code: https://github.com/arthurprs/heap-experiments
@bors bors merged commit 175d434 into rust-lang:master Sep 3, 2016
@bluss bluss added the relnotes Marks issues that should be documented in the release notes of the next release. label Sep 8, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
relnotes Marks issues that should be documented in the release notes of the next release.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants