Various memory measurement improvements (#36834)

The two significant changes here are 1) a commit that frees memory used
to perform memory reporting once the reporting is complete, 2) memory
reporting for the system font service. There are various other commits
that remove `#[ignore_malloc_size_of]` attributes for data that we are
now able to measure, but they do not significantly change our
measurements when testing servo.org.

Testing: Comparing the output of about:memory on servo.org.

---------

Signed-off-by: Josh Matthews <josh@joshmatthews.net>

Cargo.lock

@@ -2194,6 +2194,7 @@ dependencies = [
  "net_traits",
  "num-traits",
  "parking_lot",
+ "profile_traits",
  "range",
  "serde",
  "servo_allocator",

components/fonts/Cargo.toml

@@ -38,6 +38,7 @@ memmap2 = { workspace = true }
 net_traits = { workspace = true }
 num-traits = { workspace = true }
 parking_lot = { workspace = true }
+profile_traits = { workspace = true }
 range = { path = "../range" }
 serde = { workspace = true }
 servo_arc = { workspace = true }

components/fonts/font_store.rs

@@ -5,8 +5,8 @@
 use std::collections::HashMap;
 use std::sync::Arc;
 
-use atomic_refcell::AtomicRefCell;
 use log::warn;
+use malloc_size_of_derive::MallocSizeOf;
 use parking_lot::RwLock;
 use style::stylesheets::DocumentStyleSheet;
 use style::values::computed::{FontStyle, FontWeight};
@@ -15,7 +15,7 @@ use crate::font::FontDescriptor;
 use crate::font_template::{FontTemplate, FontTemplateRef, FontTemplateRefMethods, IsOblique};
 use crate::system_font_service::{FontIdentifier, LowercaseFontFamilyName};
 
-#[derive(Default)]
+#[derive(Default, MallocSizeOf)]
 pub struct FontStore {
     pub(crate) families: HashMap<LowercaseFontFamilyName, FontTemplates>,
     web_fonts_loading_for_stylesheets: Vec<(DocumentStyleSheet, usize)>,
@@ -134,7 +134,7 @@ impl FontStore {
 ///
 /// This optimization is taken from:
 /// <https://searchfox.org/mozilla-central/source/gfx/thebes/gfxFontEntry.cpp>.
-#[derive(Clone, Debug, Default)]
+#[derive(Clone, Debug, Default, MallocSizeOf)]
 struct SimpleFamily {
     regular: Option<FontTemplateRef>,
     bold: Option<FontTemplateRef>,
@@ -190,7 +190,7 @@ impl SimpleFamily {
     }
 }
 /// A list of font templates that make up a given font family.
-#[derive(Clone, Debug)]
+#[derive(Clone, Debug, MallocSizeOf)]
 pub struct FontTemplates {
     pub(crate) templates: Vec<FontTemplateRef>,
     simple_family: Option<SimpleFamily>,
@@ -263,7 +263,7 @@ impl FontTemplates {
             }
         }
 
-        let new_template = Arc::new(AtomicRefCell::new(new_template));
+        let new_template = FontTemplateRef::new(new_template);
         self.templates.push(new_template.clone());
         self.update_simple_family(new_template);
     }

components/fonts/font_template.rs

@@ -3,7 +3,7 @@
  * file, You can obtain one at https://mozilla.org/MPL/2.0/. */
 
 use std::fmt::{Debug, Error, Formatter};
-use std::ops::RangeInclusive;
+use std::ops::{Deref, RangeInclusive};
 use std::sync::Arc;
 
 use atomic_refcell::AtomicRefCell;
@@ -20,7 +20,21 @@ use crate::system_font_service::{
 };
 
 /// A reference to a [`FontTemplate`] with shared ownership and mutability.
-pub type FontTemplateRef = Arc<AtomicRefCell<FontTemplate>>;
+#[derive(Clone, Debug, MallocSizeOf)]
+pub struct FontTemplateRef(#[conditional_malloc_size_of] Arc<AtomicRefCell<FontTemplate>>);
+
+impl FontTemplateRef {
+    pub fn new(template: FontTemplate) -> Self {
+        Self(Arc::new(AtomicRefCell::new(template)))
+    }
+}
+
+impl Deref for FontTemplateRef {
+    type Target = Arc<AtomicRefCell<FontTemplate>>;
+    fn deref(&self) -> &Self::Target {
+        &self.0
+    }
+}
 
 /// Describes how to select a font from a given family. This is very basic at the moment and needs
 /// to be expanded or refactored when we support more of the font styling parameters.

components/fonts/system_font_service.rs

@@ -6,16 +6,19 @@ use std::borrow::ToOwned;
 use std::cell::OnceCell;
 use std::collections::HashMap;
 use std::ops::{Deref, RangeInclusive};
-use std::sync::Arc;
 use std::{fmt, thread};
 
 use app_units::Au;
-use atomic_refcell::AtomicRefCell;
 use compositing_traits::CrossProcessCompositorApi;
 use ipc_channel::ipc::{self, IpcReceiver, IpcSender};
 use log::debug;
+use malloc_size_of::MallocSizeOf as MallocSizeOfTrait;
 use malloc_size_of_derive::MallocSizeOf;
 use parking_lot::{Mutex, RwLock};
+use profile_traits::mem::{
+    ProcessReports, ProfilerChan, Report, ReportKind, ReportsChan, perform_memory_report,
+};
+use profile_traits::path;
 use serde::{Deserialize, Serialize};
 use servo_config::pref;
 use servo_url::ServoUrl;
@@ -66,11 +69,12 @@ pub enum SystemFontServiceMessage {
     ),
     GetFontKey(IpcSender<FontKey>),
     GetFontInstanceKey(IpcSender<FontInstanceKey>),
+    CollectMemoryReport(ReportsChan),
     Exit(IpcSender<()>),
     Ping,
 }
 
-#[derive(Default)]
+#[derive(Default, MallocSizeOf)]
 struct ResolvedGenericFontFamilies {
     default: OnceCell<LowercaseFontFamilyName>,
     serif: OnceCell<LowercaseFontFamilyName>,
@@ -84,6 +88,7 @@ struct ResolvedGenericFontFamilies {
 /// The system font service. There is one of these for every Servo instance. This is a thread,
 /// responsible for reading the list of system fonts, handling requests to match against
 /// them, and ensuring that only one copy of system font data is loaded at a time.
+#[derive(MallocSizeOf)]
 pub struct SystemFontService {
     port: IpcReceiver<SystemFontServiceMessage>,
     local_families: FontStore,
@@ -118,8 +123,12 @@ impl SystemFontServiceProxySender {
 }
 
 impl SystemFontService {
-    pub fn spawn(compositor_api: CrossProcessCompositorApi) -> SystemFontServiceProxySender {
+    pub fn spawn(
+        compositor_api: CrossProcessCompositorApi,
+        memory_profiler_sender: ProfilerChan,
+    ) -> SystemFontServiceProxySender {
         let (sender, receiver) = ipc::channel().unwrap();
+        let memory_reporter_sender = sender.clone();
 
         thread::Builder::new()
             .name("SystemFontService".to_owned())
@@ -138,7 +147,13 @@ impl SystemFontService {
 
                 cache.fetch_new_keys();
                 cache.refresh_local_families();
-                cache.run();
+
+                memory_profiler_sender.run_with_memory_reporting(
+                    || cache.run(),
+                    "system-fonts".to_owned(),
+                    memory_reporter_sender,
+                    SystemFontServiceMessage::CollectMemoryReport,
+                );
             })
             .expect("Thread spawning failed");
 
@@ -172,6 +187,9 @@ impl SystemFontService {
                     self.fetch_new_keys();
                     let _ = result_sender.send(self.free_font_instance_keys.pop().unwrap());
                 },
+                SystemFontServiceMessage::CollectMemoryReport(report_sender) => {
+                    self.collect_memory_report(report_sender);
+                },
                 SystemFontServiceMessage::Ping => (),
                 SystemFontServiceMessage::Exit(result) => {
                     let _ = result.send(());
@@ -181,6 +199,17 @@ impl SystemFontService {
         }
     }
 
+    fn collect_memory_report(&self, report_sender: ReportsChan) {
+        perform_memory_report(|ops| {
+            let reports = vec![Report {
+                path: path!["system-fonts"],
+                kind: ReportKind::ExplicitSystemHeapSize,
+                size: self.size_of(ops),
+            }];
+            report_sender.send(ProcessReports::new(reports));
+        });
+    }
+
     #[cfg_attr(
         feature = "tracing",
         tracing::instrument(skip_all, fields(servo_profiling = true), level = "trace")
@@ -528,11 +557,7 @@ impl SystemFontServiceProxy {
             panic!("SystemFontService has already exited.");
         };
 
-        let templates: Vec<_> = templates
+        let templates: Vec<_> = templates.into_iter().map(FontTemplateRef::new).collect();
-            .into_iter()
-            .map(AtomicRefCell::new)
-            .map(Arc::new)
-            .collect();
         self.templates.write().insert(cache_key, templates.clone());
 
         templates

components/fonts/tests/font.rs

@@ -5,14 +5,13 @@
 use std::fs::File;
 use std::io::Read;
 use std::path::PathBuf;
-use std::sync::Arc;
 
 use app_units::Au;
 use euclid::num::Zero;
 use fonts::platform::font::PlatformFont;
 use fonts::{
-    Font, FontData, FontDescriptor, FontIdentifier, FontTemplate, PlatformFontMethods,
+    Font, FontData, FontDescriptor, FontIdentifier, FontTemplate, FontTemplateRef,
-    ShapingFlags, ShapingOptions,
+    PlatformFontMethods, ShapingFlags, ShapingOptions,
 };
 use servo_url::ServoUrl;
 use style::properties::longhands::font_variant_caps::computed_value::T as FontVariantCaps;
@@ -42,13 +41,7 @@ fn make_font(path: PathBuf) -> Font {
         variant: FontVariantCaps::Normal,
         pt_size: Au::from_px(24),
     };
-    Font::new(
+    Font::new(FontTemplateRef::new(template), descriptor, Some(data), None).unwrap()
-        Arc::new(atomic_refcell::AtomicRefCell::new(template)),
-        descriptor,
-        Some(data),
-        None,
-    )
-    .unwrap()
 }
 
 #[test]

components/fonts/tests/font_context.rs

@@ -137,6 +137,7 @@ mod font_context {
                         break;
                     },
                     SystemFontServiceMessage::Ping => {},
+                    SystemFontServiceMessage::CollectMemoryReport(..) => {},
                 }
             }
         }

components/layout/layout_impl.rs

@@ -4,9 +4,8 @@
 
 #![allow(unsafe_code)]
 
-use std::cell::{Cell, LazyCell, RefCell};
+use std::cell::{Cell, RefCell};
-use std::collections::{HashMap, HashSet};
+use std::collections::HashMap;
-use std::ffi::c_void;
 use std::fmt::Debug;
 use std::process;
 use std::sync::{Arc, LazyLock};
@@ -95,10 +94,6 @@ use crate::{BoxTree, FragmentTree};
 static STYLE_THREAD_POOL: Mutex<&style::global_style_data::STYLE_THREAD_POOL> =
     Mutex::new(&style::global_style_data::STYLE_THREAD_POOL);
 
-thread_local!(static SEEN_POINTERS: LazyCell<RefCell<HashSet<*const c_void>>> = const {
-    LazyCell::new(|| RefCell::new(HashSet::new()))
-});
-
 /// A CSS file to style the user agent stylesheet.
 static USER_AGENT_CSS: &[u8] = include_bytes!("./stylesheets/user-agent.css");
 

components/malloc_size_of/lib.rs

@@ -50,6 +50,7 @@ use std::cell::OnceCell;
 use std::collections::BinaryHeap;
 use std::hash::{BuildHasher, Hash};
 use std::ops::Range;
+use std::rc::Rc;
 use std::sync::Arc;
 
 use style::values::generics::length::GenericLengthPercentageOrAuto;
@@ -577,6 +578,28 @@ impl<T: MallocSizeOf> MallocConditionalSizeOf for Arc<T> {
     }
 }
 
+impl<T> MallocUnconditionalShallowSizeOf for Rc<T> {
+    fn unconditional_shallow_size_of(&self, ops: &mut MallocSizeOfOps) -> usize {
+        unsafe { ops.malloc_size_of(Rc::as_ptr(self)) }
+    }
+}
+
+impl<T: MallocSizeOf> MallocUnconditionalSizeOf for Rc<T> {
+    fn unconditional_size_of(&self, ops: &mut MallocSizeOfOps) -> usize {
+        self.unconditional_shallow_size_of(ops) + (**self).size_of(ops)
+    }
+}
+
+impl<T: MallocSizeOf> MallocConditionalSizeOf for Rc<T> {
+    fn conditional_size_of(&self, ops: &mut MallocSizeOfOps) -> usize {
+        if ops.have_seen_ptr(Rc::as_ptr(self)) {
+            0
+        } else {
+            self.unconditional_size_of(ops)
+        }
+    }
+}
+
 /// If a mutex is stored directly as a member of a data type that is being measured,
 /// it is the unique owner of its contents and deserves to be measured.
 ///
@@ -709,6 +732,12 @@ impl<T> MallocSizeOf for ipc_channel::ipc::IpcSender<T> {
     }
 }
 
+impl<T> MallocSizeOf for ipc_channel::ipc::IpcReceiver<T> {
+    fn size_of(&self, _ops: &mut MallocSizeOfOps) -> usize {
+        0
+    }
+}
+
 impl MallocSizeOf for ipc_channel::ipc::IpcSharedMemory {
     fn size_of(&self, _ops: &mut MallocSizeOfOps) -> usize {
         self.len()
@@ -749,7 +778,10 @@ malloc_size_of_is_0!(std::time::SystemTime);
 malloc_size_of_is_0!(style::data::ElementData);
 malloc_size_of_is_0!(style::font_face::SourceList);
 malloc_size_of_is_0!(style::properties::ComputedValues);
+malloc_size_of_is_0!(style::properties::declaration_block::PropertyDeclarationBlock);
 malloc_size_of_is_0!(style::queries::values::PrefersColorScheme);
+malloc_size_of_is_0!(style::stylesheets::Stylesheet);
+malloc_size_of_is_0!(style::values::specified::source_size_list::SourceSizeList);
 malloc_size_of_is_0!(taffy::Layout);
 malloc_size_of_is_0!(unicode_bidi::Level);
 malloc_size_of_is_0!(unicode_script::Script);
@@ -773,6 +805,7 @@ malloc_size_of_is_webrender_malloc_size_of!(webrender_api::BorderStyle);
 malloc_size_of_is_webrender_malloc_size_of!(webrender_api::BoxShadowClipMode);
 malloc_size_of_is_webrender_malloc_size_of!(webrender_api::ColorF);
 malloc_size_of_is_webrender_malloc_size_of!(webrender_api::ExtendMode);
+malloc_size_of_is_webrender_malloc_size_of!(webrender_api::FontKey);
 malloc_size_of_is_webrender_malloc_size_of!(webrender_api::FontInstanceKey);
 malloc_size_of_is_webrender_malloc_size_of!(webrender_api::GlyphInstance);
 malloc_size_of_is_webrender_malloc_size_of!(webrender_api::GradientStop);
@@ -817,6 +850,14 @@ where
     }
 }
 
+impl<T> MallocSizeOf for style::shared_lock::Locked<T> {
+    fn size_of(&self, _ops: &mut MallocSizeOfOps) -> usize {
+        // TODO: fix this implementation when Locked derives MallocSizeOf.
+        0
+        //<style::shared_lock::Locked<T> as stylo_malloc_size_of::MallocSizeOf>::size_of(self, ops)
+    }
+}
+
 impl<T: MallocSizeOf> MallocSizeOf for atomic_refcell::AtomicRefCell<T> {
     fn size_of(&self, ops: &mut MallocSizeOfOps) -> usize {
         self.borrow().size_of(ops)

components/script/dom/bindings/cell.rs

@@ -10,6 +10,7 @@ pub(crate) use std::cell::{Ref, RefCell, RefMut};
 
 #[cfg(feature = "refcell_backtrace")]
 pub(crate) use accountable_refcell::{Ref, RefCell, RefMut, ref_filter_map};
+use malloc_size_of::{MallocConditionalSizeOf, MallocSizeOfOps};
 #[cfg(not(feature = "refcell_backtrace"))]
 pub(crate) use ref_filter_map::ref_filter_map;
 
@@ -24,6 +25,12 @@ pub(crate) struct DomRefCell<T> {
     value: RefCell<T>,
 }
 
+impl<T: MallocConditionalSizeOf> MallocConditionalSizeOf for DomRefCell<T> {
+    fn conditional_size_of(&self, ops: &mut MallocSizeOfOps) -> usize {
+        self.value.borrow().conditional_size_of(ops)
+    }
+}
+
 // Functionality specific to Servo's `DomRefCell` type
 // ===================================================
 

components/script/dom/element.rs

@@ -179,7 +179,7 @@ pub struct Element {
     /// <https://dom.spec.whatwg.org/#concept-element-is-value>
     #[no_trace]
     is: DomRefCell<Option<LocalName>>,
-    #[ignore_malloc_size_of = "Arc"]
+    #[conditional_malloc_size_of]
     #[no_trace]
     style_attribute: DomRefCell<Option<Arc<Locked<PropertyDeclarationBlock>>>>,
     attr_list: MutNullableDom<NamedNodeMap>,

components/script/dom/htmlimageelement.rs

@@ -97,6 +97,7 @@ enum ParseState {
     AfterDescriptor,
 }
 
+#[derive(MallocSizeOf)]
 pub(crate) struct SourceSet {
     image_sources: Vec<ImageSource>,
     source_size: SourceSizeList,
@@ -111,13 +112,13 @@ impl SourceSet {
     }
 }
 
-#[derive(Clone, Debug, PartialEq)]
+#[derive(Clone, Debug, MallocSizeOf, PartialEq)]
 pub struct ImageSource {
     pub url: String,
     pub descriptor: Descriptor,
 }
 
-#[derive(Clone, Debug, PartialEq)]
+#[derive(Clone, Debug, MallocSizeOf, PartialEq)]
 pub struct Descriptor {
     pub width: Option<u32>,
     pub density: Option<f64>,
@@ -145,7 +146,7 @@ struct ImageRequest {
     parsed_url: Option<ServoUrl>,
     source_url: Option<USVString>,
     blocker: DomRefCell<Option<LoadBlocker>>,
-    #[ignore_malloc_size_of = "Arc"]
+    #[conditional_malloc_size_of]
     #[no_trace]
     image: Option<Arc<Image>>,
     #[no_trace]
@@ -162,7 +163,6 @@ pub(crate) struct HTMLImageElement {
     pending_request: DomRefCell<ImageRequest>,
     form_owner: MutNullableDom<HTMLFormElement>,
     generation: Cell<u32>,
-    #[ignore_malloc_size_of = "SourceSet"]
     source_set: DomRefCell<SourceSet>,
     last_selected_source: DomRefCell<Option<USVString>>,
     #[ignore_malloc_size_of = "promises are hard"]

components/script/dom/htmllinkelement.rs

@@ -98,7 +98,7 @@ pub(crate) struct HTMLLinkElement {
     #[no_trace]
     relations: Cell<LinkRelations>,
 
-    #[ignore_malloc_size_of = "Arc"]
+    #[conditional_malloc_size_of]
     #[no_trace]
     stylesheet: DomRefCell<Option<Arc<Stylesheet>>>,
     cssom_stylesheet: MutNullableDom<CSSStyleSheet>,

components/script/dom/htmlscriptelement.rs

@@ -281,19 +281,18 @@ pub(crate) enum ScriptType {
 pub(crate) struct CompiledSourceCode {
     #[ignore_malloc_size_of = "SM handles JS values"]
     pub(crate) source_code: Stencil,
-    #[ignore_malloc_size_of = "Rc is hard"]
+    #[conditional_malloc_size_of = "Rc is hard"]
     pub(crate) original_text: Rc<DOMString>,
 }
 
-#[derive(JSTraceable)]
+#[derive(JSTraceable, MallocSizeOf)]
 pub(crate) enum SourceCode {
-    Text(Rc<DOMString>),
+    Text(#[conditional_malloc_size_of] Rc<DOMString>),
     Compiled(CompiledSourceCode),
 }
 
 #[derive(JSTraceable, MallocSizeOf)]
 pub(crate) struct ScriptOrigin {
-    #[ignore_malloc_size_of = "Rc is hard"]
     code: SourceCode,
     #[no_trace]
     url: ServoUrl,

components/script/dom/htmlstyleelement.rs

@@ -34,7 +34,7 @@ use crate::stylesheet_loader::{StylesheetLoader, StylesheetOwner};
 #[dom_struct]
 pub(crate) struct HTMLStyleElement {
     htmlelement: HTMLElement,
-    #[ignore_malloc_size_of = "Arc"]
+    #[conditional_malloc_size_of]
     #[no_trace]
     stylesheet: DomRefCell<Option<Arc<Stylesheet>>>,
     cssom_stylesheet: MutNullableDom<CSSStyleSheet>,

components/servo/lib.rs

@@ -1044,7 +1044,11 @@ fn create_constellation(
     );
 
     let system_font_service = Arc::new(
-        SystemFontService::spawn(compositor_proxy.cross_process_compositor_api.clone()).to_proxy(),
+        SystemFontService::spawn(
+            compositor_proxy.cross_process_compositor_api.clone(),
+            mem_profiler_chan.clone(),
+        )
+        .to_proxy(),
     );
 
     let (canvas_create_sender, canvas_ipc_sender) = CanvasPaintThread::start(

components/shared/compositing/lib.rs

@@ -14,6 +14,7 @@ use embedder_traits::{
 use euclid::Rect;
 use ipc_channel::ipc::IpcSender;
 use log::warn;
+use malloc_size_of_derive::MallocSizeOf;
 use pixels::Image;
 use strum_macros::IntoStaticStr;
 use style_traits::CSSPixel;
@@ -188,7 +189,7 @@ pub struct CompositionPipeline {
 }
 
 /// A mechanism to send messages from ScriptThread to the parent process' WebRender instance.
-#[derive(Clone, Deserialize, Serialize)]
+#[derive(Clone, Deserialize, MallocSizeOf, Serialize)]
 pub struct CrossProcessCompositorApi(pub IpcSender<CompositorMsg>);
 
 impl CrossProcessCompositorApi {

components/shared/profile/mem.rs

@@ -279,7 +279,6 @@ thread_local!(static SEEN_POINTERS: LazyCell<RefCell<HashSet<*const c_void>>> =
 /// The function is expected to call all the desired [MallocSizeOf::size_of]
 /// for allocations reachable from the current thread.
 pub fn perform_memory_report<F: FnOnce(&mut MallocSizeOfOps)>(f: F) {
-    SEEN_POINTERS.with(|pointers| pointers.borrow_mut().clear());
     let seen_pointer = move |ptr| SEEN_POINTERS.with(|pointers| !pointers.borrow_mut().insert(ptr));
     let mut ops = MallocSizeOfOps::new(
         servo_allocator::usable_size,
@@ -287,4 +286,9 @@ pub fn perform_memory_report<F: FnOnce(&mut MallocSizeOfOps)>(f: F) {
         Some(Box::new(seen_pointer)),
     );
     f(&mut ops);
+    SEEN_POINTERS.with(|pointers| {
+        let mut pointers = pointers.borrow_mut();
+        pointers.clear();
+        pointers.shrink_to_fit();
+    });
 }

components/url/lib.rs

@@ -35,7 +35,7 @@ pub enum UrlError {
 }
 
 #[derive(Clone, Deserialize, Eq, Hash, MallocSizeOf, Ord, PartialEq, PartialOrd, Serialize)]
-pub struct ServoUrl(#[ignore_malloc_size_of = "Arc"] Arc<Url>);
+pub struct ServoUrl(#[conditional_malloc_size_of] Arc<Url>);
 
 impl ServoUrl {
     pub fn from_url(url: Url) -> Self {