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

Add A: 'static bound for Arc/Rc::pin_in #120092

Merged
merged 1 commit into from
Apr 12, 2024

Conversation

zetanumbers
Copy link
Contributor

Analogous to #79327
Needed to preserve pin's drop guarantee

@rustbot
Copy link
Collaborator

rustbot commented Jan 18, 2024

r? @joshtriplett

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

@rustbot rustbot 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. labels Jan 18, 2024
@joshtriplett
Copy link
Member

r? libs-api

@rustbot rustbot added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Feb 11, 2024
@rustbot rustbot assigned Amanieu and unassigned joshtriplett Feb 11, 2024
@Amanieu
Copy link
Member

Amanieu commented Feb 11, 2024

@bors r+

@bors
Copy link
Contributor

bors commented Feb 11, 2024

📌 Commit 656a388 has been approved by Amanieu

It is now in the queue for this repository.

@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 Feb 11, 2024
oli-obk added a commit to oli-obk/rust that referenced this pull request Feb 13, 2024
…r, r=Amanieu

Add `A: 'static` bound for `Arc/Rc::pin_in`

Analogous to rust-lang#79327
Needed to preserve pin's [drop guarantee](https://doc.rust-lang.org/std/pin/index.html#drop-guarantee)
geektype added a commit to geektype/rust that referenced this pull request Feb 13, 2024
…r, r=Amanieu

Add `A: 'static` bound for `Arc/Rc::pin_in`

Analogous to rust-lang#79327
Needed to preserve pin's [drop guarantee](https://doc.rust-lang.org/std/pin/index.html#drop-guarantee)
@RalfJung
Copy link
Member

This seems like something that needs a comment somewhere to explain these bounds -- someone just finding them in the code might be very confused about why they are needed.

@Amanieu
Copy link
Member

Amanieu commented Feb 14, 2024

@bors r-

@zetanumbers Can you add a comment explaining why this is needed? Also add it to the original Box::pin_in,

@bors bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Feb 14, 2024
@zetanumbers
Copy link
Contributor Author

zetanumbers commented Feb 15, 2024

So it's a bit tricky to argue what bounds should be on Arc::pin_in because I don't see any practical use of it (or Pin<Arc<T>> in general) right now.

So #79327 was justified because otherwise it violates pin's drop guarantee, which says that memory that holds pointee of a pinned (smart) pointer is allowed to be deallocated only after dropping the pointee, unless pointee is !Unpin. This guarantee to some extend can allow, for example, to send a pointer to a buffer allocated inside of the future's local state to write some data to it. It is also needed for hypothetical scoped async tasks and scoped thread join handles in particular to be able to safely capture references to future's local state (see proposal's zulip thread).

So why is there even Arc::pin? Arc does not implement DerefMut, so to be honest I hardly see any reasonable use case. Except for one hypothetical functionality:

/// This is kinda like `Arc::get_mut` but with `Pin`
fn pin_arc_get_mut(this: &mut Pin<Arc<T>>) -> Option<Pin<&mut T>> {
  // implementation
}

@zetanumbers
Copy link
Contributor Author

So I say it would probably will never be less beneficial to have this bound, since Pin<Arc<T>> is useless otherwise anyway. That unless someone already deemed it reasonable to implement that pin_arc_get_mut. :)

@zetanumbers
Copy link
Contributor Author

Actually I would like to add this Arc::get_pin_mut as a static method. I assume that would require RFC, right?

@Amanieu
Copy link
Member

Amanieu commented Feb 16, 2024

It would require an ACP, along with a real use case for it.

@Amanieu
Copy link
Member

Amanieu commented Feb 16, 2024

For this PR, I think just a simple sentence saying that Pin requires that the underlying allocation be leaked if a pinned object is leaked, which is only possible for A: 'static.

@Amanieu
Copy link
Member

Amanieu commented Apr 11, 2024

@bors r+

@bors
Copy link
Contributor

bors commented Apr 11, 2024

📌 Commit 656a388 has been approved by Amanieu

It is now in the queue for this repository.

@bors bors removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Apr 11, 2024
@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Apr 11, 2024
@bors
Copy link
Contributor

bors commented Apr 12, 2024

⌛ Testing commit 656a388 with merge 46961d2...

@bors
Copy link
Contributor

bors commented Apr 12, 2024

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

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Apr 12, 2024
@bors bors merged commit 46961d2 into rust-lang:master Apr 12, 2024
12 checks passed
@rustbot rustbot added this to the 1.79.0 milestone Apr 12, 2024
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (46961d2): 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.5% [4.5%, 4.5%] 1
Regressions ❌
(secondary)
3.0% [3.0%, 3.0%] 1
Improvements ✅
(primary)
-5.6% [-5.6%, -5.6%] 1
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) -0.5% [-5.6%, 4.5%] 2

Cycles

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)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-1.3% [-1.6%, -1.0%] 2
All ❌✅ (primary) - - 0

Binary size

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

Bootstrap: 676.479s -> 677.39s (0.13%)
Artifact size: 316.06 MiB -> 315.96 MiB (-0.03%)

@zetanumbers zetanumbers deleted the pin_in_static_allocator branch May 31, 2024 12:07
@zetanumbers
Copy link
Contributor Author

zetanumbers commented May 31, 2024

So why is there even Arc::pin? Arc does not implement DerefMut, so to be honest I hardly see any reasonable use case. Except for one hypothetical functionality:

/// This is kinda like `Arc::get_mut` but with `Pin`
fn pin_arc_get_mut(this: &mut Pin<Arc<T>>) -> Option<Pin<&mut T>> {
  // implementation
}

I have come to a such feature would be unsound. Actually I believe Arc::pin is practically useless since you can unpin any arc with simple Arc::clone. Thus Arc::pin should be deprecated I think. I should investigate code using it.

EDIT: After reading Pin documentation again, I've found that there's no safe way to get a reference to the smart pointer itself, so that checks out.

@RalfJung
Copy link
Member

RalfJung commented May 31, 2024 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants