From 400eb83d302eb6b1ced3f03bd431ec041eac2fec Mon Sep 17 00:00:00 2001 From: Jake Hughes Date: Tue, 26 Apr 2022 14:43:16 +0100 Subject: [PATCH 1/2] Prevent collections from unnecessarily finalizing elems Collection types such as Vec have drop methods which are only necessary if T requires dropping. This can be managed partially by implementing `NoFinalize` on `Vec`: unsafe impl NoFinalize for Vec {} However, this will return true for needs_finalize if `T: !Drop` but also doesn't implement `NoFinalize`. To get around this, we introduce a trait which is expected only to be used in the standard library called `OnlyFinalizeComponents`. Implementing this trait on a type indicates to the collector that it can ignore that type's drop method, but it must still recurse through the type's components to see if any of them need dropping. --- compiler/rustc_hir/src/lang_items.rs | 1 + compiler/rustc_middle/src/query/mod.rs | 4 ++ compiler/rustc_middle/src/ty/util.rs | 8 ++++ compiler/rustc_span/src/symbol.rs | 1 + compiler/rustc_ty_utils/src/common_traits.rs | 8 ++++ .../rustc_ty_utils/src/needs_finalizer.rs | 41 ++++++++++++++----- library/alloc/src/boxed.rs | 10 ++++- library/alloc/src/raw_vec.rs | 5 ++- library/alloc/src/vec/mod.rs | 8 +++- library/core/src/gc.rs | 10 +++++ library/core/src/ptr/unique.rs | 4 ++ src/test/ui/gc/needs_finalize.rs | 9 ++++ 12 files changed, 94 insertions(+), 15 deletions(-) diff --git a/compiler/rustc_hir/src/lang_items.rs b/compiler/rustc_hir/src/lang_items.rs index 1b97d9ccca948..6c9929816327b 100644 --- a/compiler/rustc_hir/src/lang_items.rs +++ b/compiler/rustc_hir/src/lang_items.rs @@ -215,6 +215,7 @@ language_item_table! { Drop, sym::drop, drop_trait, Target::Trait, GenericRequirement::None; NoFinalize, sym::no_finalize, no_finalize_trait, Target::Trait, GenericRequirement::None; + FlzComps, sym::flz_comps, flz_comps_trait, Target::Trait, GenericRequirement::None; NoTrace, sym::notrace, no_trace_trait, Target::Trait, GenericRequirement::None; Conservative, sym::conservative, conservative_trait, Target::Trait, GenericRequirement::None; diff --git a/compiler/rustc_middle/src/query/mod.rs b/compiler/rustc_middle/src/query/mod.rs index e53db4d4b6c66..005973bc3bc44 100644 --- a/compiler/rustc_middle/src/query/mod.rs +++ b/compiler/rustc_middle/src/query/mod.rs @@ -1176,6 +1176,10 @@ rustc_queries! { query is_collectable_raw(env: ty::ParamEnvAnd<'tcx, Ty<'tcx>>) -> bool { desc { "computing whether `{}` is `Collectable`", env.value } } + /// Query backing `TyS::must_check_component_tys_for_finalizer`. + query must_check_component_tys_for_finalizer_raw(env: ty::ParamEnvAnd<'tcx, Ty<'tcx>>) -> bool { + desc { "computing whether `{}` contains types which might need finalizing", env.value } + } /// Query backing `TyS::needs_drop`. query needs_drop_raw(env: ty::ParamEnvAnd<'tcx, Ty<'tcx>>) -> bool { desc { "computing whether `{}` needs drop", env.value } diff --git a/compiler/rustc_middle/src/ty/util.rs b/compiler/rustc_middle/src/ty/util.rs index 9a4c8019228b7..6e3f901a504bc 100644 --- a/compiler/rustc_middle/src/ty/util.rs +++ b/compiler/rustc_middle/src/ty/util.rs @@ -749,6 +749,14 @@ impl<'tcx> ty::TyS<'tcx> { tcx_at.is_collectable_raw(param_env.and(self)) } + pub fn must_check_component_tys_for_finalizer( + &'tcx self, + tcx_at: TyCtxtAt<'tcx>, + param_env: ty::ParamEnv<'tcx>, + ) -> bool { + tcx_at.must_check_component_tys_for_finalizer_raw(param_env.and(self)) + } + /// Fast path helper for testing if a type is `Freeze`. /// /// Returning true means the type is known to be `Freeze`. Returning diff --git a/compiler/rustc_span/src/symbol.rs b/compiler/rustc_span/src/symbol.rs index 84eadd97a8f94..e389f8bebb010 100644 --- a/compiler/rustc_span/src/symbol.rs +++ b/compiler/rustc_span/src/symbol.rs @@ -655,6 +655,7 @@ symbols! { float_to_int_unchecked, floorf32, floorf64, + flz_comps, fmaf32, fmaf64, fmt, diff --git a/compiler/rustc_ty_utils/src/common_traits.rs b/compiler/rustc_ty_utils/src/common_traits.rs index ac611d04f4da3..c520379808fc4 100644 --- a/compiler/rustc_ty_utils/src/common_traits.rs +++ b/compiler/rustc_ty_utils/src/common_traits.rs @@ -34,6 +34,13 @@ fn is_collectable_raw<'tcx>(tcx: TyCtxt<'tcx>, query: ty::ParamEnvAnd<'tcx, Ty<' is_item_raw(tcx, query, LangItem::Collectable) } +fn must_check_component_tys_for_finalizer_raw<'tcx>( + tcx: TyCtxt<'tcx>, + query: ty::ParamEnvAnd<'tcx, Ty<'tcx>>, +) -> bool { + is_item_raw(tcx, query, LangItem::FlzComps) +} + fn is_unpin_raw<'tcx>(tcx: TyCtxt<'tcx>, query: ty::ParamEnvAnd<'tcx, Ty<'tcx>>) -> bool { is_item_raw(tcx, query, LangItem::Unpin) } @@ -65,6 +72,7 @@ pub(crate) fn provide(providers: &mut ty::query::Providers) { is_no_trace_raw, is_no_finalize_raw, is_collectable_raw, + must_check_component_tys_for_finalizer_raw, is_unpin_raw, ..*providers }; diff --git a/compiler/rustc_ty_utils/src/needs_finalizer.rs b/compiler/rustc_ty_utils/src/needs_finalizer.rs index 245db2bed6e53..a4a358d829c89 100644 --- a/compiler/rustc_ty_utils/src/needs_finalizer.rs +++ b/compiler/rustc_ty_utils/src/needs_finalizer.rs @@ -14,8 +14,9 @@ fn needs_finalizer_raw<'tcx>(tcx: TyCtxt<'tcx>, query: ty::ParamEnvAnd<'tcx, Ty< if !tcx.sess.opts.gc_optimize_finalizers { return tcx.needs_drop_raw(query); } - let finalizer_fields = - move |adt_def: &ty::AdtDef| tcx.adt_finalizer_tys(adt_def.did).map(|tys| tys.iter()); + let finalizer_fields = move |adt_def: &ty::AdtDef, _must_recurse: bool| { + tcx.adt_finalizer_tys(adt_def.did).map(|tys| tys.iter()) + }; let res = NeedsDropTypes::new(tcx, query.param_env, query.value, finalizer_fields).next().is_some(); debug!("needs_finalize_raw({:?}) = {:?}", query, res); @@ -59,7 +60,7 @@ impl<'tcx, F> NeedsDropTypes<'tcx, F> { impl<'tcx, F, I> Iterator for NeedsDropTypes<'tcx, F> where - F: Fn(&ty::AdtDef) -> NeedsDropResult, + F: Fn(&ty::AdtDef, bool) -> NeedsDropResult, I: Iterator>, { type Item = NeedsDropResult>; @@ -76,10 +77,6 @@ where return Some(Err(AlwaysRequiresDrop)); } - if ty.is_no_finalize_modulo_regions(tcx.at(DUMMY_SP), self.param_env) { - return None; - } - let components = match needs_drop_components(ty, &tcx.data_layout) { Err(e) => return Some(Err(e)), Ok(components) => components, @@ -94,7 +91,14 @@ where for component in components { match *component.kind() { - _ if component.is_copy_modulo_regions(tcx.at(DUMMY_SP), self.param_env) => (), + _ if component.is_copy_modulo_regions(tcx.at(DUMMY_SP), self.param_env) + && !component.must_check_component_tys_for_finalizer( + tcx.at(DUMMY_SP), + self.param_env, + ) => + { + () + } ty::Closure(_, substs) => { queue_type(self, substs.as_closure().tupled_upvars_ty()); @@ -125,7 +129,22 @@ where // `ManuallyDrop`. If it's a struct or enum without a `Drop` // impl then check whether the field types need `Drop`. ty::Adt(adt_def, substs) => { - let tys = match (self.adt_components)(adt_def) { + if ty.is_no_finalize_modulo_regions(tcx.at(DUMMY_SP), self.param_env) { + continue; + } + + let must_recurse = component.must_check_component_tys_for_finalizer( + tcx.at(DUMMY_SP), + self.param_env, + ); + + if must_recurse { + for subst_ty in substs.types() { + queue_type(self, subst_ty); + } + } + + let tys = match (self.adt_components)(adt_def, must_recurse) { Err(e) => return Some(Err(e)), Ok(tys) => tys, }; @@ -166,11 +185,11 @@ fn adt_finalizer_tys( def_id: DefId, ) -> Result<&ty::List>, AlwaysRequiresDrop> { let adt_has_dtor = |adt_def: &ty::AdtDef| adt_def.destructor(tcx).is_some(); - let adt_components = move |adt_def: &ty::AdtDef| { + let adt_components = move |adt_def: &ty::AdtDef, must_recurse: bool| { if adt_def.is_manually_drop() { debug!("dt_finalizer_tys: `{:?}` is manually drop", adt_def); return Ok(Vec::new().into_iter()); - } else if adt_has_dtor(adt_def) { + } else if adt_has_dtor(adt_def) && !must_recurse { debug!("adt_finalizer_tys: `{:?}` implements `Drop`", adt_def); // Special case tuples return Err(AlwaysRequiresDrop); diff --git a/library/alloc/src/boxed.rs b/library/alloc/src/boxed.rs index 674cb5358d56a..43efb0a4a1e4d 100644 --- a/library/alloc/src/boxed.rs +++ b/library/alloc/src/boxed.rs @@ -139,7 +139,7 @@ use core::convert::{From, TryFrom}; use core::fmt; use core::future::Future; use core::gc::Collectable; -use core::gc::NoFinalize; +use core::gc::{NoFinalize, OnlyFinalizeComponents}; use core::hash::{Hash, Hasher}; #[cfg(not(no_global_oom_handling))] use core::iter::FromIterator; @@ -2014,7 +2014,13 @@ unsafe impl NoFinalize for Box {} unsafe impl NoFinalize for Box {} #[unstable(feature = "gc", issue = "none")] -unsafe impl Collectable for Box { +unsafe impl OnlyFinalizeComponents for Box {} + +#[unstable(feature = "gc", issue = "none")] +unsafe impl OnlyFinalizeComponents for Box {} + +#[unstable(feature = "gc", issue = "none")] +unsafe impl Collectable for Box { unsafe fn set_collectable(&self) { unsafe { crate::alloc::set_managed(self.0.as_ptr() as *mut u8); diff --git a/library/alloc/src/raw_vec.rs b/library/alloc/src/raw_vec.rs index edd8f87239eee..bafaf4846e5c7 100644 --- a/library/alloc/src/raw_vec.rs +++ b/library/alloc/src/raw_vec.rs @@ -2,7 +2,7 @@ use core::alloc::LayoutError; use core::cmp; -use core::gc::NoFinalize; +use core::gc::{NoFinalize, OnlyFinalizeComponents}; use core::intrinsics; use core::mem::{self, ManuallyDrop, MaybeUninit}; use core::ops::Drop; @@ -482,6 +482,9 @@ unsafe impl<#[may_dangle] T, A: Allocator> Drop for RawVec { } } +#[unstable(feature = "gc", issue = "none")] +unsafe impl OnlyFinalizeComponents for RawVec {} + // Central function for reserve error handling. #[cfg(not(no_global_oom_handling))] #[inline] diff --git a/library/alloc/src/vec/mod.rs b/library/alloc/src/vec/mod.rs index ef7f62d6bf85d..c9c6287cbb363 100644 --- a/library/alloc/src/vec/mod.rs +++ b/library/alloc/src/vec/mod.rs @@ -58,7 +58,7 @@ use core::cmp; use core::cmp::Ordering; use core::convert::TryFrom; use core::fmt; -use core::gc::NoFinalize; +use core::gc::{NoFinalize, OnlyFinalizeComponents}; use core::hash::{Hash, Hasher}; use core::intrinsics::{arith_offset, assume}; use core::iter; @@ -2806,6 +2806,12 @@ unsafe impl NoFinalize for Vec {} #[unstable(feature = "gc", issue = "none")] unsafe impl NoFinalize for Vec {} +#[unstable(feature = "gc", issue = "none")] +unsafe impl OnlyFinalizeComponents for Vec {} + +#[unstable(feature = "gc", issue = "none")] +unsafe impl OnlyFinalizeComponents for Vec {} + #[stable(feature = "rust1", since = "1.0.0")] unsafe impl<#[may_dangle] T, A: Allocator> Drop for Vec { fn drop(&mut self) { diff --git a/library/core/src/gc.rs b/library/core/src/gc.rs index 2bf37c22f2864..7eeaa5901798f 100644 --- a/library/core/src/gc.rs +++ b/library/core/src/gc.rs @@ -26,6 +26,16 @@ pub trait Conservative {} #[cfg_attr(not(bootstrap), lang = "no_finalize")] pub unsafe trait NoFinalize {} +#[cfg_attr(not(bootstrap), lang = "flz_comps")] +/// Prevents a type from being finalized by GC if none of the component types +/// need dropping. This can be thought of as a weaker version of `NoFinalize`. +/// +/// # Safety +/// +/// Unsafe because this should be used with care. Preventing drop from +/// running can lead to surprising behaviour. +pub unsafe trait OnlyFinalizeComponents {} + #[unstable(feature = "gc", issue = "none")] #[cfg_attr(not(bootstrap), lang = "notrace")] pub auto trait NoTrace {} diff --git a/library/core/src/ptr/unique.rs b/library/core/src/ptr/unique.rs index aa88ab2d9eab5..474a1912393d3 100644 --- a/library/core/src/ptr/unique.rs +++ b/library/core/src/ptr/unique.rs @@ -5,6 +5,7 @@ use crate::mem; use crate::ops::{CoerceUnsized, DispatchFromDyn}; use crate::gc::NoTrace; +use crate::gc::OnlyFinalizeComponents; /// A wrapper around a raw non-null `*mut T` that indicates that the possessor /// of this wrapper owns the referent. Useful for building abstractions like @@ -52,6 +53,9 @@ pub struct Unique { #[unstable(feature = "ptr_internals", issue = "none")] unsafe impl Send for Unique {} +#[unstable(feature = "gc", issue = "none")] +unsafe impl OnlyFinalizeComponents for Unique {} + /// `Unique` pointers are `Sync` if `T` is `Sync` because the data they /// reference is unaliased. Note that this aliasing invariant is /// unenforced by the type system; the abstraction using the diff --git a/src/test/ui/gc/needs_finalize.rs b/src/test/ui/gc/needs_finalize.rs index a07f05e38bf2a..f665ae7230ade 100644 --- a/src/test/ui/gc/needs_finalize.rs +++ b/src/test/ui/gc/needs_finalize.rs @@ -21,6 +21,9 @@ struct FinalizedContainer(T); struct MaybeFinalize(T); struct ExplicitNoFinalize; +// This struct doesn't need finalizing, but it's not annoted as such. +struct NonAnnotated(usize); + unsafe impl NoFinalize for ExplicitNoFinalize {} unsafe impl NoFinalize for HasDropNoFinalize {} @@ -63,6 +66,9 @@ static OUTER_NEEDS_FINALIZING: bool = mem::needs_finalizer::>(); static STATIC_MAYBE_FINALIZE_DROP_COMPONENTS: bool = mem::needs_finalizer::>(); +static VEC_COLLECTABLE_NO_DROP_ELEMENT: bool = mem::needs_finalizer::>(); +static BOX_COLLECTABLE_NO_DROP_ELEMENT: bool = mem::needs_finalizer::>(); + fn main() { assert!(!CONST_U8); assert!(!CONST_STRING); @@ -98,4 +104,7 @@ fn main() { assert!(!STATIC_MAYBE_FINALIZE_NO_COMPONENTS); assert!(STATIC_MAYBE_FINALIZE_DROP_COMPONENTS); + + assert!(!VEC_COLLECTABLE_NO_DROP_ELEMENT); + assert!(!BOX_COLLECTABLE_NO_DROP_ELEMENT); } From 6702b1fe9c7eb01fac36f03e19cc7d0f306e56ef Mon Sep 17 00:00:00 2001 From: Jake Hughes Date: Tue, 26 Apr 2022 14:47:10 +0100 Subject: [PATCH 2/2] Add NoFinalize support to standard library wrappers --- library/core/src/cell.rs | 4 ++++ library/core/src/option.rs | 4 ++++ 2 files changed, 8 insertions(+) diff --git a/library/core/src/cell.rs b/library/core/src/cell.rs index bc3f7167fac3a..bb74cb0f2dff5 100644 --- a/library/core/src/cell.rs +++ b/library/core/src/cell.rs @@ -194,6 +194,7 @@ use crate::cmp::Ordering; use crate::fmt::{self, Debug, Display}; +use crate::gc::NoFinalize; use crate::marker::Unsize; use crate::mem; use crate::ops::{CoerceUnsized, Deref, DerefMut}; @@ -240,6 +241,9 @@ pub struct Cell { #[stable(feature = "rust1", since = "1.0.0")] unsafe impl Send for Cell where T: Send {} +#[unstable(feature = "gc", issue = "none")] +unsafe impl NoFinalize for Cell where T: NoFinalize {} + // Note that this negative impl isn't strictly necessary for correctness, // as `Cell` wraps `UnsafeCell`, which is itself `!Sync`. // However, given how important `Cell`'s `!Sync`-ness is, diff --git a/library/core/src/option.rs b/library/core/src/option.rs index 1ec119a71e42c..32641e8f53cf2 100644 --- a/library/core/src/option.rs +++ b/library/core/src/option.rs @@ -500,6 +500,7 @@ #![stable(feature = "rust1", since = "1.0.0")] +use crate::gc::NoFinalize; use crate::iter::{FromIterator, FusedIterator, TrustedLen}; use crate::panicking::{panic, panic_str}; use crate::pin::Pin; @@ -2285,3 +2286,6 @@ impl Option> { } } } + +#[unstable(feature = "gc", issue = "none")] +unsafe impl NoFinalize for Option {}