From 9a9a4e12d51c983db25e3517332f3f7346ef7b22 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Thu, 16 May 2019 04:35:34 +0200 Subject: [PATCH] style: Add refcount logging to servo_arc. Differential Revision: https://phabricator.services.mozilla.com/D32173 --- components/servo_arc/Cargo.toml | 1 + components/servo_arc/lib.rs | 68 +++++++++++++++++++++--- components/style/bloom.rs | 2 +- components/style/gecko/wrapper.rs | 8 +-- components/style/global_style_data.rs | 2 +- components/style/rule_tree/mod.rs | 9 ++-- components/style/shared_lock.rs | 8 +++ components/style/sharing/mod.rs | 8 ++- components/style/values/computed/list.rs | 2 +- components/style_traits/arc_slice.rs | 15 +++++- 10 files changed, 100 insertions(+), 23 deletions(-) diff --git a/components/servo_arc/Cargo.toml b/components/servo_arc/Cargo.toml index 58f1ea8ef0b..8eb84601b22 100644 --- a/components/servo_arc/Cargo.toml +++ b/components/servo_arc/Cargo.toml @@ -12,6 +12,7 @@ path = "lib.rs" [features] servo = ["serde"] +gecko = [] [dependencies] nodrop = {version = "0.1.8"} diff --git a/components/servo_arc/lib.rs b/components/servo_arc/lib.rs index e1496b7971a..79785bbb56a 100644 --- a/components/servo_arc/lib.rs +++ b/components/servo_arc/lib.rs @@ -42,7 +42,7 @@ use std::fmt; use std::hash::{Hash, Hasher}; use std::iter::{ExactSizeIterator, Iterator}; use std::marker::PhantomData; -use std::mem; +use std::mem::{self, align_of, size_of}; use std::ops::{Deref, DerefMut}; use std::os::raw::c_void; use std::process; @@ -115,7 +115,10 @@ pub struct Arc { /// Once the mutation is finished, you can call `.shareable()` and get a regular `Arc` /// out of it. /// -/// ```rust +/// Ignore the doctest below there's no way to skip building with refcount +/// logging during doc tests (see rust-lang/rust#45599). +/// +/// ```rust,ignore /// # use servo_arc::UniqueArc; /// let data = [1, 2, 3, 4, 5]; /// let mut x = UniqueArc::new(data); @@ -169,18 +172,35 @@ impl Arc { /// Construct an `Arc` #[inline] pub fn new(data: T) -> Self { - let x = Box::new(ArcInner { + let ptr = Box::into_raw(Box::new(ArcInner { count: atomic::AtomicUsize::new(1), data, - }); + })); + + #[cfg(all(feature = "gecko", debug_assertions))] + unsafe { + // FIXME(emilio): Would be so amazing to have + // std::intrinsics::type_name() around, so that we could also report + // a real size. + NS_LogCtor(ptr as *const _, b"ServoArc\0".as_ptr() as *const _, 8); + } + unsafe { Arc { - p: ptr::NonNull::new_unchecked(Box::into_raw(x)), + p: ptr::NonNull::new_unchecked(ptr), phantom: PhantomData, } } } + /// Construct an intentionally-leaked arc. + #[inline] + pub fn new_leaked(data: T) -> Self { + let arc = Self::new(data); + arc.mark_as_intentionally_leaked(); + arc + } + /// Convert the Arc to a raw pointer, suitable for use across FFI /// /// Note: This returns a pointer to the data T, which is offset in the allocation. @@ -290,9 +310,29 @@ impl Arc { unsafe { &*self.ptr() } } - // Non-inlined part of `drop`. Just invokes the destructor. + #[inline(always)] + fn record_drop(&self) { + #[cfg(all(feature = "gecko", debug_assertions))] + unsafe { + NS_LogDtor(self.ptr() as *const _, b"ServoArc\0".as_ptr() as *const _, 8); + } + } + + /// Marks this `Arc` as intentionally leaked for the purposes of refcount + /// logging. + /// + /// It's a logic error to call this more than once, but it's not unsafe, as + /// it'd just report negative leaks. + #[inline(always)] + pub fn mark_as_intentionally_leaked(&self) { + self.record_drop(); + } + + // Non-inlined part of `drop`. Just invokes the destructor and calls the + // refcount logging machinery if enabled. #[inline(never)] unsafe fn drop_slow(&mut self) { + self.record_drop(); let _ = Box::from_raw(self.ptr()); } @@ -308,6 +348,12 @@ impl Arc { } } +#[cfg(all(feature = "gecko", debug_assertions))] +extern "C" { + fn NS_LogCtor(aPtr: *const std::os::raw::c_void, aTypeName: *const std::os::raw::c_char, aSize: u32); + fn NS_LogDtor(aPtr: *const std::os::raw::c_void, aTypeName: *const std::os::raw::c_char, aSize: u32); +} + impl Clone for Arc { #[inline] fn clone(&self) -> Self { @@ -612,7 +658,6 @@ impl Arc> { F: FnOnce(Layout) -> *mut u8, I: Iterator + ExactSizeIterator, { - use std::mem::{align_of, size_of}; assert_ne!(size_of::(), 0, "Need to think about ZST"); let inner_align = align_of::>>(); @@ -700,6 +745,15 @@ impl Arc> { ); } + #[cfg(all(feature = "gecko", debug_assertions))] + unsafe { + if !is_static { + // FIXME(emilio): Would be so amazing to have + // std::intrinsics::type_name() around. + NS_LogCtor(ptr as *const _, b"ServoArc\0".as_ptr() as *const _, 8) + } + } + // Return the fat Arc. assert_eq!( size_of::(), diff --git a/components/style/bloom.rs b/components/style/bloom.rs index d75d548ff71..c730547986b 100644 --- a/components/style/bloom.rs +++ b/components/style/bloom.rs @@ -19,7 +19,7 @@ thread_local! { /// such that they can be reused across style traversals. StyleBloom is responsible /// for ensuring that the bloom filter is zeroed when it is dropped. static BLOOM_KEY: Arc> = - Arc::new(AtomicRefCell::new(BloomFilter::new())); + Arc::new_leaked(AtomicRefCell::new(BloomFilter::new())); } /// A struct that allows us to fast-reject deep descendant selectors avoiding diff --git a/components/style/gecko/wrapper.rs b/components/style/gecko/wrapper.rs index a1443d1ad8d..75254f0890a 100644 --- a/components/style/gecko/wrapper.rs +++ b/components/style/gecko/wrapper.rs @@ -1736,7 +1736,7 @@ impl<'le> TElement for GeckoElement<'le> { PropertyDeclaration::TextAlign(SpecifiedTextAlign::MozCenterOrInherit), Importance::Normal, ); - let arc = Arc::new(global_style_data.shared_lock.wrap(pdb)); + let arc = Arc::new_leaked(global_style_data.shared_lock.wrap(pdb)); ApplicableDeclarationBlock::from_declarations(arc, ServoCascadeLevel::PresHints) }; static ref TABLE_COLOR_RULE: ApplicableDeclarationBlock = { @@ -1745,7 +1745,7 @@ impl<'le> TElement for GeckoElement<'le> { PropertyDeclaration::Color(SpecifiedColor(Color::InheritFromBodyQuirk.into())), Importance::Normal, ); - let arc = Arc::new(global_style_data.shared_lock.wrap(pdb)); + let arc = Arc::new_leaked(global_style_data.shared_lock.wrap(pdb)); ApplicableDeclarationBlock::from_declarations(arc, ServoCascadeLevel::PresHints) }; static ref MATHML_LANG_RULE: ApplicableDeclarationBlock = { @@ -1754,7 +1754,7 @@ impl<'le> TElement for GeckoElement<'le> { PropertyDeclaration::XLang(SpecifiedLang(atom!("x-math"))), Importance::Normal, ); - let arc = Arc::new(global_style_data.shared_lock.wrap(pdb)); + let arc = Arc::new_leaked(global_style_data.shared_lock.wrap(pdb)); ApplicableDeclarationBlock::from_declarations(arc, ServoCascadeLevel::PresHints) }; static ref SVG_TEXT_DISABLE_ZOOM_RULE: ApplicableDeclarationBlock = { @@ -1763,7 +1763,7 @@ impl<'le> TElement for GeckoElement<'le> { PropertyDeclaration::XTextZoom(SpecifiedZoom(false)), Importance::Normal, ); - let arc = Arc::new(global_style_data.shared_lock.wrap(pdb)); + let arc = Arc::new_leaked(global_style_data.shared_lock.wrap(pdb)); ApplicableDeclarationBlock::from_declarations(arc, ServoCascadeLevel::PresHints) }; }; diff --git a/components/style/global_style_data.rs b/components/style/global_style_data.rs index 90936ff7172..e19cab37bfa 100644 --- a/components/style/global_style_data.rs +++ b/components/style/global_style_data.rs @@ -125,7 +125,7 @@ lazy_static! { }; /// Global style data pub static ref GLOBAL_STYLE_DATA: GlobalStyleData = GlobalStyleData { - shared_lock: SharedRwLock::new(), + shared_lock: SharedRwLock::new_leaked(), options: StyleSystemOptions::default(), }; } diff --git a/components/style/rule_tree/mod.rs b/components/style/rule_tree/mod.rs index d653b6be99d..d7497a7813f 100644 --- a/components/style/rule_tree/mod.rs +++ b/components/style/rule_tree/mod.rs @@ -755,8 +755,7 @@ unsafe impl Sync for RuleTree {} unsafe impl Send for RuleTree {} // On Gecko builds, hook into the leak checking machinery. -#[cfg(feature = "gecko")] -#[cfg(debug_assertions)] +#[cfg(all(feature = "gecko", debug_assertions))] mod gecko_leak_checking { use super::RuleNode; use std::mem::size_of; @@ -789,15 +788,13 @@ mod gecko_leak_checking { #[inline(always)] fn log_new(_ptr: *const RuleNode) { - #[cfg(feature = "gecko")] - #[cfg(debug_assertions)] + #[cfg(all(feature = "gecko", debug_assertions))] gecko_leak_checking::log_ctor(_ptr); } #[inline(always)] fn log_drop(_ptr: *const RuleNode) { - #[cfg(feature = "gecko")] - #[cfg(debug_assertions)] + #[cfg(all(feature = "gecko", debug_assertions))] gecko_leak_checking::log_dtor(_ptr); } diff --git a/components/style/shared_lock.rs b/components/style/shared_lock.rs index 073c696fa3d..3931a841bab 100644 --- a/components/style/shared_lock.rs +++ b/components/style/shared_lock.rs @@ -71,6 +71,14 @@ impl SharedRwLock { } } + /// Create a new global shared lock (gecko). + #[cfg(feature = "gecko")] + pub fn new_leaked() -> Self { + SharedRwLock { + cell: Some(Arc::new_leaked(AtomicRefCell::new(SomethingZeroSizedButTyped))), + } + } + /// Create a new read-only shared lock (gecko). #[cfg(feature = "gecko")] pub fn read_only() -> Self { diff --git a/components/style/sharing/mod.rs b/components/style/sharing/mod.rs index 3ce2a717dc8..5f5d1c6cab5 100644 --- a/components/style/sharing/mod.rs +++ b/components/style/sharing/mod.rs @@ -485,8 +485,12 @@ type SharingCache = SharingCacheBase>; type TypelessSharingCache = SharingCacheBase; type StoredSharingCache = Arc>; -thread_local!(static SHARING_CACHE_KEY: StoredSharingCache = - Arc::new(AtomicRefCell::new(TypelessSharingCache::default()))); +thread_local! { + // TODO(emilio): Looks like a few of these should just be Rc> or + // something. No need for atomics in the thread-local code. + static SHARING_CACHE_KEY: StoredSharingCache = + Arc::new_leaked(AtomicRefCell::new(TypelessSharingCache::default())); +} /// An LRU cache of the last few nodes seen, so that we can aggressively try to /// reuse their styles. diff --git a/components/style/values/computed/list.rs b/components/style/values/computed/list.rs index 3536aa656fe..bdcf62ba2f8 100644 --- a/components/style/values/computed/list.rs +++ b/components/style/values/computed/list.rs @@ -10,7 +10,7 @@ pub use crate::values::specified::list::MozListReversed; pub use crate::values::specified::list::{QuotePair, Quotes}; lazy_static! { - static ref INITIAL_QUOTES: crate::ArcSlice = crate::ArcSlice::from_iter( + static ref INITIAL_QUOTES: crate::ArcSlice = crate::ArcSlice::from_iter_leaked( vec![ QuotePair { opening: "\u{201c}".to_owned().into(), diff --git a/components/style_traits/arc_slice.rs b/components/style_traits/arc_slice.rs index 6a151e3dc5a..bbbac1a0757 100644 --- a/components/style_traits/arc_slice.rs +++ b/components/style_traits/arc_slice.rs @@ -40,7 +40,7 @@ impl Deref for ArcSlice { lazy_static! { // ThinArc doesn't support alignments greater than align_of::. static ref EMPTY_ARC_SLICE: ArcSlice = { - ArcSlice(ThinArc::from_header_and_iter(ARC_SLICE_CANARY, iter::empty())) + ArcSlice::from_iter_leaked(iter::empty()) }; } @@ -74,6 +74,19 @@ impl ArcSlice { ArcSlice(ThinArc::from_header_and_iter(ARC_SLICE_CANARY, items)) } + /// Creates an Arc for a slice using the given iterator to generate the + /// slice, and marks the arc as intentionally leaked from the refcount + /// logging point of view. + #[inline] + pub fn from_iter_leaked(items: I) -> Self + where + I: Iterator + ExactSizeIterator, + { + let thin_arc = ThinArc::from_header_and_iter(ARC_SLICE_CANARY, items); + thin_arc.with_arc(|a| a.mark_as_intentionally_leaked()); + ArcSlice(thin_arc) + } + /// Creates a value that can be passed via FFI, and forgets this value /// altogether. #[inline]