Add simple async drop glue generation #121801

zetanumbers · 2024-02-29T13:41:37Z

This is a prototype of the async drop glue generation for some simple types. Async drop glue is intended to behave very similar to the regular drop glue except for being asynchronous. Currently it does not execute synchronous drops but only calls user implementations of AsyncDrop::async_drop associative function and awaits the returned future. It is not complete as it only recurses into arrays, slices, tuples, and structs and does not have same sensible restrictions as the old Drop trait implementation like having the same bounds as the type definition, while code assumes their existence (requires a future work).

This current design uses a workaround as it does not create any custom async destructor state machine types for ADTs, but instead uses types defined in the std library called future combinators (deferred_async_drop, chain, ready_unit).

Also I recommend reading my explainer.

This is a part of the MCP: Low level components for async drop work.

Feature completeness:

rustbot · 2024-02-29T13:41:45Z

r? @michaelwoerister

rustbot has assigned @michaelwoerister.
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

rustbot · 2024-02-29T14:36:47Z

The Miri subtree was changed

cc @rust-lang/miri

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

Some changes occurred in src/tools/rustfmt

cc @rust-lang/rustfmt

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

Some changes occurred to the core trait solver

cc @rust-lang/initiative-trait-system-refactor

petrochenkov · 2024-03-01T16:11:43Z

Some high level comments:

This PR adds many lang items, it would be good to document not just what they do, but why are they added

to avoid generating MIR manually and call something from the library instead (not strictly necessary, but makes life easier)
to get some type information that is not easily calculatable otherwise (?), e.g. why AsyncDestruct and its associated type exist in particular.
to execute something user-defined from the compiler (strictly necessary, like trait AsyncDrop)
etc

It would also be good to add test cases that are not yet supported in a test, to see what happens when they are encountered and add corresponding FIXMEs for people to see.
Preferable with some short comments explaining whether they are trivial to support or not, and why.
E.g. what if AsyncDrop is implemented or called for enums, closures, or generic parameters.

Also add a test for a struct (or tuple) with 2 fields, one with non-trivial async drop, and one with non-trivial sync drop.
This case is not yet supported, but it would be good to cover it with a failing test for visibility.

petrochenkov · 2024-03-01T16:15:06Z

This needs a review from someone who maintains MIR building/passes/transforms.
It's quite possible that some things here are not done idiomatically, and could be done simpler/better.
I oversaw this work, but neither @zetanumbers nor me had a previous experience with these parts of the compiler.

RalfJung · 2024-03-02T11:19:17Z

Great to see Miri tests already in the initial PR, that's much appreciated. :)

zetanumbers · 2024-03-04T13:31:32Z

I would say there's one specific problem about this API. async_drop_in_place gets a pointer to some object to construct object's async destructor. This async destructor has a lifetime, that comes from the T: 'a bound of async_drop_in_place, which may not be meaningfull. For example if T: 'static is true, then it may be that 'a = 'static even tho we probably meant lifetime of a reference to this object instead. This 'a argument came from the trivial implementation of async drop purely within the library so we would able to write <T as AsyncDrop>::Dropper<'a> within the implementation of impl<'a, T: AsyncDrop> AsyncDestruct<'a> for T {...}.

To move forward I can either remove this 'a parameter or change function signature to something like a fn async_drop_in_place<'a, T>(to_drop: &'a mut MaybeUninit<T>) .... Second one has the problem with MaybeUninit<T> requiring T: Sized, which is already bad for slices, but perhaps this could be replaced with ManuallyDrop<T>. First variant has been done once, and i'm actually surprised lifetime punning kinda works here, however there could still be a hypothetical problem with making some coroutines Unpin cause technically we still don't borrow anything there thus possibly allowing compiler to mark this coroutine as Unpin, while having a self-referencial pointer.

For now I've decided to stick with removing 'a since Unpin coroutines don't seem to be close in the future.

oli-obk · 2024-03-04T14:38:55Z

Did a partial review to get an overview. I think the main comment of this PR should mention that this does not yet do

Component 2: Calling async drop glue at the end of scope

or any other automatic drop glue handling. All that we get is that a type gets dropped asynchronously plus the glue needed for field types and builtin types

src/tools/rustfmt/config_proc_macro/src/attrs.rs

compiler/rustc_middle/src/ty/instance.rs

library/core/src/future/async_drop.rs

compiler/rustc_ty_utils/src/instance.rs

compiler/rustc_trait_selection/src/traits/project.rs

oli-obk · 2024-03-05T09:51:42Z

compiler/rustc_mir_transform/src/shim.rs

+        /// Empty strategy should generate this:
+        ///
+        /// ```ignore (illustrative)
+        /// ready_unit()
+        /// ```
+        Empty,


can we avoid this variant by never requesting the shim for these at all?

We would still need to be able to directly call async_drop_in_place(_raw)::<()> so we cannot "never request". This is true for regular drop glue too.

May be relevant: #121801 (comment)

compiler/rustc_mir_transform/src/shim.rs

oli-obk · 2024-03-05T09:56:40Z

compiler/rustc_mir_transform/src/shim.rs

+            }
+        }
+
+        GlueStrategy::Chain { has_surface_async_drop, elem_tys } => {


can we write most of this one in surface Rust somehow? Maybe we could generate a tuple of the various types that should be dropped and recursively invoke drop on them?

Sorry but I don't understand what you are describing. If you imagine a handwritten async destructor future for a tuple it would be an enum, for which each variant of it represents running an async destructor for some field, sequentially ordered one after the other + variants like Unresumed and Done.

oli-obk · 2024-03-05T09:57:50Z

I have not reviewed the shim impls yet, I'd prefer it if we could avoid such a complex shim

zetanumbers · 2024-03-05T14:10:13Z

I have not reviewed the shim impls yet, I'd prefer it if we could avoid such a complex shim

When it comes to GlueStrategy::Chain it shouldn't have been complex like that, but since I haven't yet managed to generate async destructor ADTs inside of the compiler I've used nested library types, which is suboptimal.

EDIT: However it will instead introduce ADT async destructors Future::poll shim.

zetanumbers · 2024-03-05T14:50:13Z

To reduce complexity of shim generation code I can try modeling it as nested expressions instead of directly creating basic blocks. Yeah, I should try doing that.

EDIT: going to prioritize this for now since making changes became a bit unwieldy.

EDIT2: Done in 16010ba :)

zetanumbers · 2024-03-05T14:56:42Z

Also this "constructor of async destructor" is confusing to say. Not sure what naming convention I should use instead.

traviscross · 2024-03-07T01:08:42Z

@rustbot labels +T-lang +I-lang-nominated

Let's discuss approval of this as an experiment.

@zetanumbers / @petrochenkov: If you could perhaps write up further details for the lang team to review as part of this nomination about the nature of this part of the experiment, what the motivation for it is, and how this fits into the larger body of work, that work be helpful.

rustbot · 2024-04-18T12:21:48Z

This PR changes Stable MIR

cc @oli-obk, @celinval, @ouz-a

zetanumbers · 2024-04-18T12:37:46Z

Uh-oh

This PR changes Stable MIR

zetanumbers · 2024-04-18T13:12:40Z

Sorry for adding changes last moment.

celinval · 2024-04-18T18:52:06Z

Uh-oh

This PR changes Stable MIR

The StableMIR change looks fine. Thanks for addressing it.

oli-obk · 2024-04-19T13:57:54Z

Made most matches on TyKind to be non exhaustive in 80c0b7e.

why? The equivalent matches on Drop things elsewhere are exhaustive

zetanumbers · 2024-04-19T15:08:25Z

Made most matches on TyKind to be non exhaustive in 80c0b7e.

why? The equivalent matches on Drop things elsewhere are exhaustive

Some TyKind variants are not supported yet, so I've made this branch to accept anything that either I didn't account for or is know but should be added in the future. Anyway people would have to figure out what case they should put their type in and it would be most probably the "todo" branch, so I would like to think I've made this choice myself not to spend everyone's time on those. But of course the "todo" branch should be removed in the future.

oli-obk · 2024-04-19T15:19:20Z

compiler/rustc_middle/src/ty/sty.rs

@@ -2319,6 +2319,10 @@ impl<'tcx> Ty<'tcx> {

    /// Returns the type of the async destructor of this type.
    pub fn async_destructor_ty(self, tcx: TyCtxt<'tcx>, param_env: ParamEnv<'tcx>) -> Ty<'tcx> {
+        if self.is_async_destructor_noop(tcx, param_env) || matches!(self.kind(), ty::Error(_)) {


prefer self.references_error() over matching for ty::Error

Neither drop glue nor discriminant kind do this. I will have to switch every match on ty::Error otherwise compiler may emit type errors.

I think I cannot add this case to be a part of is_async_destructor_noop because of this line, which differs from other similar types like ints:

rust/compiler/rustc_trait_selection/src/traits/project.rs

Line 1111 in e239e73

| ty::Error(_) => false,

But I'm not sure, maybe I should include it?

Hmm... I guess we can look if there's a general thing to improve here everywhere

compiler/rustc_middle/src/ty/util.rs

oli-obk · 2024-04-19T15:28:39Z

compiler/rustc_middle/src/ty/util.rs

+    /// Returning `false` means the type is known to not have `Drop`
+    /// implementation. Returning `true` means nothing -- could be
+    /// `Drop`, might not be.
+    fn could_have_surface_drop(self) -> bool {


how does this relate to is_trivially_const_drop and can we deduplicate some code?

is_trivially_const_drop recurses into array and tuple elements to determine if type can be trivially destructed, while could_have_surface_drop only checks if there's possible impl Drop for T. This function would return false for a tuple with any collection of types, while !is_trivially_const_drop for the same tuple may be true because of some element type could be an ADT. I could use could_have_surface_drop from is_trivially_const_drop since the former's negative implies the latter if it's ok.

Well, in contrast is_trivially_const_drop does not check/intend to check ManuallyDrop, so those could mean a bit different things after all.

compiler/rustc_trait_selection/src/solve/normalizes_to/mod.rs

…ruct_candidate

Co-authored-by: Oli Scherer <[email protected]>

oli-obk · 2024-04-22T14:04:47Z

@bors r+

bors · 2024-04-22T14:04:50Z

📌 Commit 67980dd has been approved by oli-obk

It is now in the queue for this repository.

compiler-errors · 2024-04-22T14:54:37Z

@bors rollup=never

bors · 2024-04-23T02:10:27Z

⌛ Testing commit 67980dd with merge aca749e...

bors · 2024-04-23T04:12:26Z

☀️ Test successful - checks-actions
Approved by: oli-obk
Pushing aca749e to master...

Explainer: https://zetanumbers.github.io/book/async-drop-design.html rust-lang/rust#121801

rust-timer · 2024-04-23T05:28:35Z

Finished benchmarking commit (aca749e): comparison URL.

Overall result: ❌ regressions - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	0.1%	[0.1%, 0.1%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

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)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-2.5%	[-2.5%, -2.5%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-2.5%	[-2.5%, -2.5%]	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: 672.713s -> 674.396s (0.25%)
Artifact size: 316.06 MiB -> 315.53 MiB (-0.17%)

Implement `needs_async_drop` in rustc and optimize async drop glue This PR expands on rust-lang#121801 and implements `Ty::needs_async_drop` which works almost exactly the same as `Ty::needs_drop`, which is needed for rust-lang#123948. Also made compiler's async drop code to look more like compiler's regular drop code, which enabled me to write an optimization where types which do not use `AsyncDrop` can simply forward async drop glue to `drop_in_place`. This made size of the async block from the [async_drop test](https://github.com/zetanumbers/rust/blob/67980dd6fb11917d23d01a19c2cf4cfc3978aac8/tests/ui/async-await/async-drop.rs) to decrease by 12%.

rustbot assigned michaelwoerister Feb 29, 2024

This comment has been minimized.

Sign in to view

zetanumbers marked this pull request as ready for review February 29, 2024 14:36

This comment has been minimized.

Sign in to view

oli-obk self-assigned this Mar 1, 2024

michaelwoerister removed their assignment Mar 1, 2024

This comment has been minimized.

Sign in to view

ytmimi reviewed Mar 5, 2024

View reviewed changes

src/tools/rustfmt/config_proc_macro/src/attrs.rs Outdated Show resolved Hide resolved

oli-obk requested changes Mar 5, 2024

View reviewed changes

rustbot 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 Mar 5, 2024

This comment has been minimized.

Sign in to view

zetanumbers marked this pull request as ready for review April 18, 2024 12:21

oli-obk requested changes Apr 19, 2024

View reviewed changes

rustbot added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Apr 19, 2024

zetanumbers and others added 3 commits April 22, 2024 15:41

Exhaustivelly match TyKind in consider_builtin_async_destruct_candidate

0881e3e

Fix copy-paste typo in the comment within consider_builtin_async_dest…

a9c7465

…ruct_candidate

Fix typo in the has_surface_drop's doc comment

67980dd

Co-authored-by: Oli Scherer <[email protected]>

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 22, 2024

bors added the merged-by-bors This PR was explicitly merged by bors. label Apr 23, 2024

bors merged commit aca749e into rust-lang:master Apr 23, 2024
13 checks passed

rustbot added this to the 1.79.0 milestone Apr 23, 2024

github-actions bot pushed a commit to rust-lang/miri that referenced this pull request Apr 23, 2024

Add simple async drop glue generation

3b9ec8e

Explainer: https://zetanumbers.github.io/book/async-drop-design.html rust-lang/rust#121801

zetanumbers deleted the async_drop_glue branch April 23, 2024 08:57

traviscross removed the I-lang-nominated Nominated for discussion during a lang team meeting. label Apr 25, 2024

zetanumbers mentioned this pull request May 3, 2024

Implement needs_async_drop in rustc and optimize async drop glue #124662

Merged

mpalmer mentioned this pull request May 6, 2024

Any suggested pattern for closing the Client on test failure? jonhoo/fantoccini#250

Open

zetanumbers mentioned this pull request Jun 14, 2024

Tracking Issue for async drop codegen #126482

Open

20 tasks

petrochenkov added the F-async_drop Async drop label Jun 17, 2024

Add simple async drop glue generation #121801

Add simple async drop glue generation #121801

Conversation

zetanumbers commented Feb 29, 2024 • edited Loading

rustbot commented Feb 29, 2024

This comment has been minimized.

rustbot commented Feb 29, 2024

This comment has been minimized.

petrochenkov commented Mar 1, 2024

petrochenkov commented Mar 1, 2024

This comment has been minimized.

This comment has been minimized.

RalfJung commented Mar 2, 2024

zetanumbers commented Mar 4, 2024

oli-obk commented Mar 4, 2024

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

zetanumbers Mar 5, 2024 • edited Loading

Choose a reason for hiding this comment

oli-obk commented Mar 5, 2024

zetanumbers commented Mar 5, 2024 • edited Loading

zetanumbers commented Mar 5, 2024 • edited Loading

zetanumbers commented Mar 5, 2024

This comment has been minimized.

This comment has been minimized.

traviscross commented Mar 7, 2024

rustbot commented Apr 18, 2024

zetanumbers commented Apr 18, 2024 • edited Loading

zetanumbers commented Apr 18, 2024

celinval commented Apr 18, 2024

oli-obk commented Apr 19, 2024

zetanumbers commented Apr 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zetanumbers Apr 19, 2024 • 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

oli-obk commented Apr 22, 2024

bors commented Apr 22, 2024

compiler-errors commented Apr 22, 2024

bors commented Apr 23, 2024

bors commented Apr 23, 2024

rust-timer commented Apr 23, 2024

Overall result: ❌ regressions - no action needed

zetanumbers commented Feb 29, 2024 •

edited

Loading

zetanumbers Mar 5, 2024 •

edited

Loading

zetanumbers commented Mar 5, 2024 •

edited

Loading

zetanumbers commented Mar 5, 2024 •

edited

Loading

zetanumbers commented Apr 18, 2024 •

edited

Loading

zetanumbers commented Apr 19, 2024 •

edited

Loading

zetanumbers Apr 19, 2024 •

edited

Loading