Async drop box support#145316
Conversation
54fef57 to
cfc843d
Compare
|
r? @oli-obk |
|
|
cfc843d to
a5631d1
Compare
|
@azhogin any updates on this? if you are looking for a review kindly mark it as ready for review (which means the pr is no longer in draft mode ) so that reviewer(s) know it's ready for reviewing. |
The main question - is it ok to use internally dynamic allocation (Box or Rc) to implement dynamic (virtual) trait call. For dynamic async drop. When dynamic trait call of async drop header function returns Box'ed coroutine to be poll'ed. This mechanic may also be used for standart async functions in dyn traits. So, I need some recommendation from a person competent in async functions in the project - is it suitable solution to proceed it further? Maybe, @oli-obk? |
|
oof, sorry for not reviewing this. I need to go through my entire review list again. |
| /// Enables async drop code generation | ||
| (incomplete, async_drop, "1.88.0", Some(126482)), | ||
| /// Allows implementing `AsyncDrop`. For usage in standard library. | ||
| (incomplete, async_drop_lib, "1.88.0", Some(126482)), |
There was a problem hiding this comment.
this feature is not checked anywhere in the compiler in the first commit. Move this change to the second commit
|
Reminder, once the PR becomes ready for a review, use |
|
|
||
| let (instance, mut llfn) = match *callee.layout.ty.kind() { | ||
| ty::FnDef(def_id, generic_args) => { | ||
| if bx.tcx().is_lang_item(def_id, LangItem::AsyncDropInPlaceDyn) { |
There was a problem hiding this comment.
why do we need backend-specific logic? I would expect all of this to happen in shims, as the backends all already know how to process shims
| &[VtblEntry::MetadataDropInPlace, VtblEntry::MetadataSize, VtblEntry::MetadataAlign]; | ||
| pub const COMMON_VTABLE_ENTRIES: &'tcx [VtblEntry<'tcx>] = &[ | ||
| VtblEntry::MetadataDropInPlace, | ||
| VtblEntry::MetadataAsyncDropInPlace, |
There was a problem hiding this comment.
hmm... somewhat annoying that we would need to modify all vtables out there, even if a lot of code never uses async drop at all. Maybe there could be a solution akin to #156090 plus requiring + async Destruct bounds on the dyn Trait (and special casing Destruct to be specifyable that way). And rejecting the construction of dyn Trait from things that have async destructors and suggesting dyn Trait + async Destruct. tho that feels like it would have very annoying ripple effects in the ecosystem and probably just be very annoying without much value.
There was a problem hiding this comment.
The main issue I see is that this change happens on stable immediately, so all vtables get bigger with a field that is alway null
| continue; | ||
| } | ||
|
|
||
| let fut_place = Place::from(fut_local); |
There was a problem hiding this comment.
do this refactoring on its own on main or in a commit before the actual logic change
Support for Box async drop.
Support for dyn traits async drop.
This is a draft PR because of experimental solution for dyn traits support.
Dyn traits async drop requires additional vtable slot 'COMMON_VTABLE_ENTRIES_ASYNCDROPINPLACE', similar to 'COMMON_VTABLE_ENTRIES_DROPINPLACE'. This slot exists in any vtable, just without 'async_drop' feature it is set to null.
'AsyncDropInPlaceDyn' is lang item function declared in std library (in alloc):
This function converts 'async_drop_in_place::{closure}' to common 'Pin<Box<dyn Future<Output = ()>>>'.
And this future, provided by virtual call, is polled in the similar way as 'ordinary' async drop features.
Dyn async drop is expanded in StateTransform into AsyncDropInPlaceDyn lang_item call and is codegen'ed later to virtual call (in the similar way as virtual sync drop call, just with return value). Only implemented in ssa codegen yet.
async_drop_in_place<T>::{closure}is a special case for vtable generation, because such coroutine is its async drop future itself.To drop this coroutine we need to continue poll it. So, its async drop constructor function in vtable returns its address (from argument), boxed and pinned. 'async_drop_in_place_self' lang item function is used for it.
Fixes #143658.