Make BinaryHeap parametric over Allocator #99339

yanchith · 2022-07-16T17:34:03Z

Tracking issue: #32838
Related: rust-lang/wg-allocators#7

This parametrizes BinaryHeap with A, similarly to how other collections are parametrized.

A couple things I left out:

BinaryHeap::append

    Currently requires both structures to have the same allocator type. Could
    change, but depends on Vec::append, which has the same constraints.


impl<T: Ord> Default for BinaryHeap<T>

    Not parametrized, because there's nowhere to conjure the allocator from.


impl<T: Ord> FromIterator<T> for BinaryHeap<T>

    Not parametrized, because there's nowhere to conjure the allocator from.


impl<T: Ord, const N: usize> From<[T; N]> for BinaryHeap<T>

    Not parametrized, because there's nowhere to conjure the allocator from.


unsafe impl<I> AsVecIntoIter for IntoIter<I>

    AsVecIntoIter is not allocator aware, and I didn't dare change it without guidance. Is this something important?

I've seen very few tests for allocator_api in general, but I'd like to at least test this on some usage code in my projects before moving forward.

EDIT: Updated the list of impls and functions that are not affected by this. BinaryHeap no longer has a SpecExtend impl, and prior work made implementing Extend possible.

rust-highfive · 2022-07-16T17:34:06Z

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

Please see the contribution instructions for more information.

rustbot · 2022-07-16T17:34:06Z

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

Stabilizing library features
Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
Changing public documentation in ways that create new stability guarantees
Changing observable runtime behavior of library APIs

bors · 2023-01-14T09:18:41Z

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

anden3 · 2023-04-22T18:13:25Z

Hello @yanchith! I noticed this PR has a merge conflict, has there been any updates?

yanchith · 2023-04-24T15:41:27Z

Hi! Nope. Still waiting for review, or someone I could discuss the open questions with.

I meanwhile don't need this anymore, but if we decide to salvage this despite the merge conflicts, I am willing to push it through.

ChrisDenton · 2023-04-24T19:02:03Z

This is an API change, albeit an unstable one, so I'll tag this as waiting on a team decision.

@rustbot label +S-waiting-on-team +T-libs-api -T-libs

tgross35 · 2023-06-09T00:39:02Z

library/alloc/src/collections/binary_heap.rs

@@ -1195,7 +1250,7 @@ impl<T> BinaryHeap<T> {
    /// ```
    #[inline]
    #[stable(feature = "drain", since = "1.6.0")]
-    pub fn drain(&mut self) -> Drain<'_, T> {
+    pub fn drain(&mut self) -> Drain<'_, T, A> {
        Drain { iter: self.data.drain(..) }
    }



Suggested change

/// Returns a reference to the underlying allocator.

#[inline]

#[unstable(feature = "allocator_api", issue = "32838")]

pub fn allocator(&self) -> &A {

self.data.allocator()

}

As with https://doc.rust-lang.org/std/vec/struct.Vec.html#method.allocator

the8472

The iterator stuff looks mostly fine. It's not very sensitive to allocators in the first place.

unsafe impl<I> AsVecIntoIter for IntoIter<I>
AsVecIntoIter is not allocator aware, and I didn't dare change it without guidance.

That's fine, the specialization that uses it currently isn't allocator-aware either.

impl<T: Ord> SpecExtend<Vec<T>> for BinaryHeap<T>

error: specializing impl repeats parameter `A`. Is this something we can do?

Also tried two different allocators, e.g. A and OA, but it depends
on BinaryHeap::append, which requires allocators to be the same type.

No longer relevant since BinaryHeap no longer has a SpecExtend. You should update the PR comment.

impl<T: Ord, A: Allocator> SpecExtend<BinaryHeap<T, A>> for BinaryHeap<T, A>

error: specializing impl repeats parameter `A`. Is this something we can do?

Also tried two different allocators, e.g. A and OA, but it depends
on BinaryHeap::append, which requires allocators to be the same type.

Ditto.

the8472 · 2023-06-10T07:00:48Z

library/alloc/src/collections/binary_heap/mod.rs


 #[stable(feature = "binary_heap_extras_15", since = "1.5.0")]
-impl<T: Ord> From<Vec<T>> for BinaryHeap<T> {
+impl<T: Ord, A: Allocator> From<Vec<T, A>> for BinaryHeap<T, A> {


You can update the other direction too (on Vec).

And what about the one below? Can that be extended or are there inference failures?

You can update the other direction too (on Vec).

Isn't it updated? Line 1748?

And what about the one below? Can that be extended or are there inference failures?

impl<T: Ord, const N: usize> From<[T; N]> for BinaryHeap<T>? AFAIK that can't work, because there's no way to instantiate the allocator.

Isn't it updated? Line 1748?

My bad.

AFAIK that can't work, because there's no way to instantiate the allocator.

We could maybe do where A: Allocator + Default but there's no precedent for that so maybe that's best discussed in a separate PR.

the8472 · 2023-06-10T07:10:35Z

library/alloc/src/collections/binary_heap/mod.rs

+    /// Returns a reference to the underlying allocator.
+    #[unstable(feature = "allocator_api", issue = "32838")]
+    #[inline]
+    pub fn allocator(&self) -> &A {
+        self.data.allocator()
+    }


The iterators should have a method like this too.

I disagree, there's no reason for the iterator to care about the allocator. None of the iterators on Vec or HashMap have such a method.

https://doc.rust-lang.org/nightly/std/vec/struct.IntoIter.html#method.allocator
https://doc.rust-lang.org/nightly/std/vec/struct.Drain.html#method.allocator

Oh, I didn't realize we had added those. My bad.

Added to IntoIter, IntoIterSorted, Drain and DrainSorted. I left PeekMut out of if for now, but let me know, and I can add it too.

I wonder if there would be any benefit to something like trait Allocated { fn allocator(&self) -> &impl Allocator } so APIs could have a generic way to check this information.

Amanieu · 2023-06-12T18:04:38Z

@bors r+

bors · 2023-06-12T18:04:40Z

📌 Commit e0e355d has been approved by Amanieu

It is now in the queue for this repository.

bors · 2023-06-12T20:43:52Z

⌛ Testing commit e0e355d with merge 71b3326f9ee1a7b2bf4811b16db47edb9e409478...

bors · 2023-06-13T00:35:26Z

💔 Test failed - checks-actions

rust-log-analyzer · 2023-06-13T00:35:35Z

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

Amanieu · 2023-06-13T01:06:24Z

@bors retry

bors · 2023-06-13T03:23:16Z

⌛ Testing commit e0e355d with merge de1ff0a...

bors · 2023-06-13T06:13:34Z

☀️ Test successful - checks-actions
Approved by: Amanieu
Pushing de1ff0a to master...

bors · 2023-06-13T06:13:34Z

☀️ Test successful - checks-actions
Approved by: Amanieu
Pushing de1ff0a to master...

rust-timer · 2023-06-13T07:29:20Z

Finished benchmarking commit (de1ff0a): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	4.1%	[4.1%, 4.1%]	1
Regressions ❌ (secondary)	3.7%	[2.4%, 5.0%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	4.1%	[4.1%, 4.1%]	1

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 648.907s -> 648.481s (-0.07%)

klensy · 2023-06-13T07:55:06Z

cb5c011 Not nice, merge commit here.

yanchith · 2023-06-13T08:14:06Z

Oh no. Sorry. I thought the entire PR would get squashed, so I didn't pay it any attention.

yanchith added 3 commits July 16, 2022 18:38

Parametrize BinaryHeap with Allocator

a3122b8

Mark lifetimes explicitly

d48b281

Parametrize a few more things

24df5f2

rust-highfive assigned joshtriplett Jul 16, 2022

rustbot added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Jul 16, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 16, 2022

JohnCSimon 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 Aug 13, 2022

JohnCSimon 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 Oct 8, 2022

yanchith mentioned this pull request Oct 18, 2022

Tracking Issue for structs which needs an allocator rust-lang/wg-allocators#7

Open

15 tasks

anden3 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 Apr 22, 2023

m-ou-se assigned Amanieu and unassigned joshtriplett Jun 6, 2023

tgross35 reviewed Jun 9, 2023

View reviewed changes

yanchith added 5 commits June 9, 2023 11:22

Merge branch 'master' into binary-heap-ta

cb5c011

Reallocatorize after merge

7a9f282

Add allocator function

208da51

Pass tidy again

767a711

Don't explicitly name Global

4df4c0d

the8472 reviewed Jun 10, 2023

View reviewed changes

yanchith added 2 commits June 11, 2023 22:42

Remove explicit lifetimes

d9b6181

Impl allocator function for iterators

e0e355d

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jun 13, 2023

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 Jun 13, 2023

bors added merged-by-bors This PR was explicitly merged by bors. labels Jun 13, 2023

bors merged commit de1ff0a into rust-lang:master Jun 13, 2023

rustbot added this to the 1.72.0 milestone Jun 13, 2023

bors mentioned this pull request Jun 13, 2023

Expand in-place iteration specialization to Flatten, FlatMap and ArrayChunks #110353

Merged

yanchith deleted the binary-heap-ta branch June 13, 2023 08:07

klensy mentioned this pull request Jun 13, 2023

Add tidy check to deny merge commits #105058

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make BinaryHeap parametric over Allocator #99339

Make BinaryHeap parametric over Allocator #99339

yanchith commented Jul 16, 2022 •

edited

Loading

rust-highfive commented Jul 16, 2022

rustbot commented Jul 16, 2022

bors commented Jan 14, 2023

anden3 commented Apr 22, 2023

yanchith commented Apr 24, 2023

ChrisDenton commented Apr 24, 2023

tgross35 Jun 9, 2023

the8472 left a comment •

edited

Loading

the8472 Jun 10, 2023

yanchith Jun 11, 2023

the8472 Jun 11, 2023

the8472 Jun 10, 2023

Amanieu Jun 10, 2023

the8472 Jun 10, 2023

Amanieu Jun 10, 2023

yanchith Jun 11, 2023

tgross35 Jun 13, 2023

Amanieu commented Jun 12, 2023

bors commented Jun 12, 2023

bors commented Jun 12, 2023

bors commented Jun 13, 2023

rust-log-analyzer commented Jun 13, 2023

Amanieu commented Jun 13, 2023

bors commented Jun 13, 2023

bors commented Jun 13, 2023

bors commented Jun 13, 2023

rust-timer commented Jun 13, 2023

klensy commented Jun 13, 2023 •

edited

Loading

yanchith commented Jun 13, 2023

Make BinaryHeap parametric over Allocator #99339

Make BinaryHeap parametric over Allocator #99339

Conversation

yanchith commented Jul 16, 2022 • edited Loading

rust-highfive commented Jul 16, 2022

rustbot commented Jul 16, 2022

bors commented Jan 14, 2023

anden3 commented Apr 22, 2023

yanchith commented Apr 24, 2023

ChrisDenton commented Apr 24, 2023

Choose a reason for hiding this comment

the8472 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Amanieu commented Jun 12, 2023

bors commented Jun 12, 2023

bors commented Jun 12, 2023

bors commented Jun 13, 2023

rust-log-analyzer commented Jun 13, 2023

Amanieu commented Jun 13, 2023

bors commented Jun 13, 2023

bors commented Jun 13, 2023

bors commented Jun 13, 2023

rust-timer commented Jun 13, 2023

Overall result: no relevant changes - no action needed

klensy commented Jun 13, 2023 • edited Loading

yanchith commented Jun 13, 2023

yanchith commented Jul 16, 2022 •

edited

Loading

the8472 left a comment •

edited

Loading

klensy commented Jun 13, 2023 •

edited

Loading