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.

Sign up for GitHub

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

Jump to bottom

Change BinaryHeap::append rebuild heuristic #77435

Merged
merged 1 commit into from
Jan 16, 2021
Merged

Change BinaryHeap::append rebuild heuristic #77435

merged 1 commit into from
Jan 16, 2021
+8 −2

Conversation

hanmertens
Copy link
Contributor

This is faster, see #77433.

Fixes #77433

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @withoutboats (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 1, 2020
@LeSeulArtichaut LeSeulArtichaut added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 16, 2020
@jyn514
Copy link
Member

jyn514 commented Oct 30, 2020

r? @scottmcm

#77433 (comment)

@jyn514 jyn514 added the I-slow Issue: Problems and improvements with respect to performance of generated code. label Oct 30, 2020
@scottmcm scottmcm mentioned this pull request Nov 8, 2020
Closed
@camelid camelid added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 20, 2020
@scottmcm scottmcm 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 Nov 20, 2020
@scottmcm
Copy link
Member

Given your comment in #77433 (comment) that

I guess it doesn't make much sense to merge #77435 as-is because of that.

I've marked this as waiting-on author for now.

Once you've made any changes you think are necessary, please run the following to mark it as needing review again:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

@JohnCSimon
Copy link
Member

Ping from triage -
@hanmertens Can you please post your status on this PR?

@hanmertens
Copy link
Contributor Author

As discussed in the comments of #77433 the heuristic should be changed (and not removed, as this PR currently does). I've done some benchmarks and I'll update the issue with that later this week. I hope that will shed some light on what the heuristic should be, then the PR can be updated.

@crlf0710
Copy link
Member

@hanmertens Ping from triage, any updates on this?

@hanmertens
Change rebuild heuristic in BinaryHeap::append
32a20f4 
See rust-lang#77433 for why the new heuristic was chosen.

Fixes rust-lang#77433
jyn514
jyn514 reviewed Jan 15, 2021
View reviewed changes
library/alloc/src/collections/binary_heap.rs
@@ -630,10 +630,16 @@ impl<T: Ord> BinaryHeap<T> {
// and about 2 * (len1 + len2) comparisons in the worst case
// while `extend` takes O(len2 * log(len1)) operations
// and about 1 * len2 * log_2(len1) comparisons in the worst case,
// assuming len1 >= len2.
// assuming len1 >= len2. For larger heaps, the crossover point
// no longer follows this reasoning and was determined empirically.
#[inline]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this still be marked #[inline] now that it's longer?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's only used once, so that should still be fine I think.

@hanmertens
Copy link
Contributor Author

I haven't received negative feedback on #77433 (comment), so I just pushed the heuristic I proposed there. TLDR; it's not optimal but better than the current one.

@scottmcm scottmcm added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 15, 2021
@scottmcm
Copy link
Member

Thanks! Seems reasonable to me, based on the other issue.

@bors r+ rollup=maybe

(I don't think the compiler uses binary heaps enough for this to affect its perf, so doesn't need to be =never.)

@bors
Copy link
Contributor

bors commented Jan 15, 2021

📌 Commit 32a20f4 has been approved by scottmcm

@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 15, 2021
@jyn514
Copy link
Member

jyn514 commented Jan 15, 2021

(I don't think the compiler uses binary heaps enough for this to affect its perf, so doesn't need to be =never.)

> rg BinaryHeap compiler/ | wc -l
0

You are correct 😆

@jyn514
Copy link
Member

jyn514 commented Jan 15, 2021

In fact the only place BinaryHeap is used in-tree is in tests for clippy and miri, and a few related clippy lints.

JohnTitor added a commit to JohnTitor/rust that referenced this pull request Jan 16, 2021
@JohnTitor
Rollup merge of rust-lang#77435 - hanmertens:binary_heap_append, r=sc…
d0f8553 
…ottmcm

Always use extend in BinaryHeap::append

This is faster, see rust-lang#77433.

Fixes rust-lang#77433
@JohnTitor JohnTitor mentioned this pull request Jan 16, 2021
Closed
@bors
Copy link
Contributor

bors commented Jan 16, 2021

⌛ Testing commit 32a20f4 with merge 410a546...

@bors
Copy link
Contributor

bors commented Jan 16, 2021

☀️ Test successful - checks-actions
Approved by: scottmcm
Pushing 410a546 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jan 16, 2021
@bors bors merged commit 410a546 into rust-lang:master Jan 16, 2021
@rustbot rustbot added this to the 1.51.0 milestone Jan 16, 2021
@bors bors mentioned this pull request Jan 16, 2021
Merged
@hanmertens hanmertens deleted the binary_heap_append branch January 17, 2021 16:42
@hanmertens hanmertens changed the title Always use extend in BinaryHeap::append Change BinaryHeap::append rebuild heuristic Jan 17, 2021
clint-white added a commit to clint-white/binary-heap-plus-rs that referenced this pull request Aug 7, 2022
@clint-white
Port changes in BinaryHeap::append
051c6ec 
This commit ports the change in the rebuild heuristic used by
`BinaryHeap::append()` that was added in rust-lang/rust#77435: "Change
BinaryHeap::append rebuild heuristic".  See also the discussion in
rust-lang/rust#77433: "Suboptimal performance of BinaryHeap::append" for
more information on how the new heuristic was chosen.

It also ports the new private method `.rebuild_tail()` now used by
`std::collections::BinaryHeap::append()` from rust-lang/rust#78681:
"Improve rebuilding behaviour of BinaryHeap::retain".

Note that Rust 1.60.0 adds the clippy lint `manual_bits` which warns
against code used here.  We suppress the lint instead of following the
upstream patch which now uses `usize::BITS`, since this was stabilized
in Rust 1.53.0 and this crate's MSRV is currently 1.36.0.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jyn514 jyn514 jyn514 left review comments

Assignees

@scottmcm scottmcm

Labels
I-slow Issue: Problems and improvements with respect to performance of generated code. 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. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.51.0
Development

Successfully merging this pull request may close these issues.

Suboptimal performance of BinaryHeap::append