Implement extract_if for BinaryHeap

[Nokel81]

View all comments

• Implement using the sort and rebuild on drop method as maintaining the heap invariant during each extraction would cost more on average

resolves #42849

I have seen #104210 and I took away from reading the comments there that this might be accepted if it does the sort-in-place and then rebuild on drop semantics

[rustbot]

r? @Mark-Simulacrum

rustbot has assigned @Mark-Simulacrum.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: libs
• libs expanded to 7 candidates

[Mark-Simulacrum]

Nominating for libs-api -- I'd recommend you wait until they discuss before continuing with changes. I think the two topics are (a) do we support extract_if over a subset of the elements, and (2) do we want this at all, given that it can be implemented just as efficiently and fairly easily for the general case on stable today?

View changes since this review

[Mark-Simulacrum]

Looking through this implementation, it seems to me that we end up replicating Vec::extract_if pretty closely. Maybe we can use it directly, possibly refactoring the impl to allow using it on a passed-in &mut Vec<T, A> for each call (essentially separating the state and the Vec being iterated)?

Alternatively, it's possible that the right pattern is to tell users that the right pattern for extract_if is:

let vec = BinaryHeap::into_sorted_vec(mem::take(heap)); // take only needed if you have only &mut.
vec.extract_if(...);
*heap = BinaryHeap::from(vec);

AFAICT, that is equally efficient to the implementation here, and we can't do better since we give &mut T access to the elements: we have to assume all elements were changed by the pass over them, so we can't rebuild more efficiently than re-sifting the whole heap. (Even though in the common case removals happened but probably no changes to the elements occurred).

[Nokel81]

I had considered trying to directly reuse the Vec::extract_if impl directly, but I was unable to figure out how to allocate an empty Vec<T> in an arbitrary allocator given only a reference to the allocator

[Nokel81]

I have found a way to reuse the Vec::extract_if implementation, PTAL

[rustbot]

Reminder, once the PR becomes ready for a review, use @rustbot ready.

[joshtriplett]

We talked about this in today's @rust-lang/libs-api meeting.

1. We do want this.
2. This shouldn't take a range or operate on subsets.
3. This should not sort first; it should iterate in arbitrary order.

[Nokel81]

@joshtriplett Thanks for the reply! I'll update the implementation to not sort on creation but still to rebuild on drop. What about providing &mut access to each element?

[Amanieu]

Providing a &mut T is fine, this is what we do for Vec as well. In fact the implementation should be a wrapper around Vec::extract_if to avoid code duplication.

[scottmcm]

Is this really worth having over just using into_vec, doing the extract, then rebuilding the heap? If this could do something smart then sure, but it sounds like it's not.

(At most a take_vec method, though you can just take(&mut heap).into_vec() so that's not really needed.)

[Nokel81]

Currently we cannot take the binary heap since vectors using custom allocators do not implement Default (I plan on opening a PR for that soon)