Skip to content

script: Add basic memory pressure reporting to SpiderMonkey#42180

Merged
sagudev merged 5 commits intoservo:mainfrom
sagudev:sm-report-mem
Jan 30, 2026
Merged

script: Add basic memory pressure reporting to SpiderMonkey#42180
sagudev merged 5 commits intoservo:mainfrom
sagudev:sm-report-mem

Conversation

@sagudev
Copy link
Copy Markdown
Member

@sagudev sagudev commented Jan 27, 2026

By reporting memory usage of rust objects back to SM it can trigger GC sooner and release more memory (I hope that we could use this to avoid OOB in webgpu in the future).

Currently we use simple size_of::<DomStruct>() + size_of::<Box<DomStruct>>() but it's possible to override reported memory with update_memory_size(self, nbytes); on reflector. This is mostly useful for canvases and buffers which usually have large associated data.

Testing: None, but I am wondering if we can connect this to about:memory.
Fixes: #42168

@sagudev sagudev requested a review from gterzian as a code owner January 27, 2026 06:44
@servo-highfive servo-highfive added the S-awaiting-review There is new code that needs to be reviewed. label Jan 27, 2026
@sagudev sagudev requested review from jdm and removed request for gterzian January 27, 2026 06:44
@jdm
Copy link
Copy Markdown
Member

jdm commented Jan 27, 2026

I have a counter proposal that doesn't increase the size of any DOM objects that don't have additional memory to report: https://github.com/servo/servo/compare/main...jdm:servo:assoc-memory?expand=1

@jdm jdm added S-awaiting-answer Someone asked a question that requires an answer. and removed S-awaiting-review There is new code that needs to be reviewed. labels Jan 27, 2026
@sagudev
Copy link
Copy Markdown
Member Author

sagudev commented Jan 27, 2026

I have a counter proposal that doesn't increase the size of any DOM objects that don't have additional memory to report: https://github.com/servo/servo/compare/main...jdm:servo:assoc-memory?expand=1

Interesting, the only problem I see is that the one that deals with actual associated memory needs to be extra careful. Let me think if we can do something more ergonomic.
IDEA: reflector has T that is () except in cases where we want to override the mem

@jdm
Copy link
Copy Markdown
Member

jdm commented Jan 27, 2026

I have a counter proposal that doesn't increase the size of any DOM objects that don't have additional memory to report: https://github.com/servo/servo/compare/main...jdm:servo:assoc-memory?expand=1

Interesting, the only problem I see is that the one that deals with actual associated memory needs to be extra careful. Let me think if we can do something more ergonomic. IDEA: reflector has T that is () except in cases where we want to override the mem

I started with a branch that implemented the Reflector + empty type idea and quickly ran into problems because the type needs to be specified in lots of places that can't know what type to use. I can share that branch if you'd like but I think it's a dead end.

@jdm
Copy link
Copy Markdown
Member

jdm commented Jan 28, 2026

The aborted branch:

Details
diff --git a/components/script/dom/bindings/reflector.rs b/components/script/dom/bindings/reflector.rs
index c888686974e..a58ad301f33 100644
--- a/components/script/dom/bindings/reflector.rs
+++ b/components/script/dom/bindings/reflector.rs
@@ -65,4 +65,12 @@ impl<T: DomGlobalGeneric<crate::DomTypeHolder>> DomGlobal for T {
     }
 }

+pub struct BoxedSelfSize<T>;
+
+impl<T> ReflectedObjectSize for BoxedSelfSize {
+    fn reflected_size(&self) -> usize {
+        std::mem::size_of::<T>() + std::mem::size_of::<Box<T>>()
+    }
+}
+
 pub(crate) use script_bindings::reflector::*;
diff --git a/components/script/dom/blob.rs b/components/script/dom/blob.rs
index 835ab7784a1..dbca3f40b8a 100644
--- a/components/script/dom/blob.rs
+++ b/components/script/dom/blob.rs
@@ -21,7 +21,7 @@ use crate::dom::bindings::codegen::Bindings::BlobBinding;
 use crate::dom::bindings::codegen::Bindings::BlobBinding::BlobMethods;
 use crate::dom::bindings::codegen::UnionTypes::ArrayBufferOrArrayBufferViewOrBlobOrString;
 use crate::dom::bindings::error::{Error, Fallible};
-use crate::dom::bindings::reflector::{DomGlobal, Reflector, reflect_dom_object_with_proto};
+use crate::dom::bindings::reflector::{BoxedSelfSize, DomGlobal, Reflector, reflect_dom_object_with_proto};
 use crate::dom::bindings::root::DomRoot;
 use crate::dom::bindings::serializable::Serializable;
 use crate::dom::bindings::str::DOMString;
@@ -40,7 +40,7 @@ pub(crate) struct Blob {
     blob_id: BlobId,
 }

-impl Blob {
+impl<Actual> Blob<Actual> {
     pub(crate) fn new(global: &GlobalScope, blob_impl: BlobImpl, can_gc: CanGc) -> DomRoot<Blob> {
         Self::new_with_proto(global, None, blob_impl, can_gc)
     }
@@ -90,7 +90,7 @@ impl Blob {
     }
 }

-impl Serializable for Blob {
+impl<Actual> Serializable for Blob<Actual> {
     type Index = BlobIndex;
     type Data = BlobImpl;

diff --git a/components/script_bindings/reflector.rs b/components/script_bindings/reflector.rs
index 390a356d0ea..6a7654c8617 100644
--- a/components/script_bindings/reflector.rs
+++ b/components/script_bindings/reflector.rs
@@ -2,6 +2,7 @@
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at https://mozilla.org/MPL/2.0/. */

+use js::jsapi::{AddAssociatedMemory, MemoryUse, RemoveAssociatedMemory};
 use js::jsapi::{Heap, JSObject};
 use js::rust::HandleObject;
 use malloc_size_of_derive::MallocSizeOf;
@@ -17,22 +18,23 @@ use crate::{DomTypes, JSTraceable};
 #[derive(MallocSizeOf)]
 #[cfg_attr(crown, crown::unrooted_must_root_lint::must_root)]
 // If you're renaming or moving this field, update the path in plugins::reflector as well
-pub struct Reflector {
+pub struct Reflector<T: ReflectedObjectSize = ()> {
     #[ignore_malloc_size_of = "defined and measured in rust-mozjs"]
     object: Heap<*mut JSObject>,
+    extra: T,
 }

-unsafe impl js::gc::Traceable for Reflector {
+unsafe impl<T: ReflectedObjectSize> js::gc::Traceable for Reflector<T> {
     unsafe fn trace(&self, _: *mut js::jsapi::JSTracer) {}
 }

-impl PartialEq for Reflector {
-    fn eq(&self, other: &Reflector) -> bool {
+impl<T: ReflectedObjectSize> PartialEq for Reflector<T> {
+    fn eq(&self, other: &Reflector<T>) -> bool {
         self.object.get() == other.object.get()
     }
 }

-impl Reflector {
+impl<T: ReflectedObjectSize> Reflector<T> {
     /// Get the reflector.
     #[inline]
     pub fn get_jsobject(&self) -> HandleObject<'_> {
@@ -57,13 +59,16 @@ impl Reflector {
     pub fn rootable(&self) -> &Heap<*mut JSObject> {
         &self.object
     }
+}

+impl Reflector<()> {
     /// Create an uninitialized `Reflector`.
     // These are used by the bindings and do not need `default()` functions.
     #[expect(clippy::new_without_default)]
     pub fn new() -> Reflector {
         Reflector {
             object: Heap::default(),
+            extra: (),
         }
     }
 }
@@ -90,12 +95,48 @@ pub trait MutDomObject: DomObject {
     unsafe fn init_reflector(&self, obj: *mut JSObject);
 }

-impl MutDomObject for Reflector {
+pub trait ReflectedObjectSize {
+    fn reflected_size(&self) -> usize;
+}
+
+impl ReflectedObjectSize for () {
+    fn reflected_size(&self) -> usize { 0 }
+}
+
+impl<T: ReflectedObjectSize> Drop for Reflector<T> {
+    fn drop(&mut self) {
+        unsafe {
+            RemoveAssociatedMemory(self.object.get(), self.extra.reflected_size(), MemoryUse::DOMBinding);
+        }
+    }
+}
+
+pub trait MutableReflectedObjectSize: ReflectedObjectSize {
+    fn set_reflected_size(&self, new_size: usize);
+}
+
+impl<T: ReflectedObjectSize> MutDomObject for Reflector<T> where Reflector<T>: DomObject {
     unsafe fn init_reflector(&self, obj: *mut JSObject) {
+        unsafe {
+            AddAssociatedMemory(obj, self.extra.reflected_size(), MemoryUse::DOMBinding);
+        }
         self.set_jsobject(obj)
     }
 }

+impl<T: MutableReflectedObjectSize> Reflector<T> {
+    fn update_size(&self, new_size: usize) {
+        unsafe {
+            RemoveAssociatedMemory(self.object.get(), self.extra.reflected_size(), MemoryUse::DOMBinding);
+        }
+        self.extra.set_reflected_size(new_size);
+        unsafe {
+            AddAssociatedMemory(self.object.get(), self.extra.reflected_size(), MemoryUse::DOMBinding);
+        }
+
+    }
+}
+
 pub trait DomGlobalGeneric<D: DomTypes>: DomObject {
     /// Returns the [`GlobalScope`] of the realm that the [`DomObject`] was created in.  If this
     /// object is a `Node`, this will be different from it's owning `Document` if adopted by. For

@sagudev sagudev marked this pull request as draft January 28, 2026 07:41
@sagudev
Copy link
Copy Markdown
Member Author

sagudev commented Jan 28, 2026

Ideally Reflector would know which T it has, but this becomes complicated with nested struct (this might be worth exploring in the future as I think it could help with avoiding some Upcast checks).

Anyway I was able to to do something reflector with AssociatedMemory type (with one transmute, but it is ok). There is still problem for those types that has overriden Drop impl, but that is something we should be able to solve easily enough.

There are also some interesting corner cases like WindowProxy, which are not really webidl types but are still dom_struct, so maybe we should move the drop code into proc_macro and and pass allow drop code as attribute (or write own trait for drops which get called from generated drop).

WDYT?

@sagudev sagudev marked this pull request as ready for review January 30, 2026 15:48
@servo-highfive servo-highfive added the S-awaiting-review There is new code that needs to be reviewed. label Jan 30, 2026
@sagudev sagudev removed the S-awaiting-answer Someone asked a question that requires an answer. label Jan 30, 2026
Comment on lines +134 to +139
impl DomObject for Reflector<AssociatedMemory> {
fn reflector(&self) -> &Reflector<()> {
// SAFETY: This is safe because we are only shortening the size of struct.
unsafe { std::mem::transmute(self) }
}
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aha, clever. I got stuck on figuring out how to avoid this.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, using this for Reflector that has AsociatedMemory will not work the way we want.

@servo-highfive servo-highfive removed the S-awaiting-review There is new code that needs to be reviewed. label Jan 30, 2026
@sagudev sagudev added this pull request to the merge queue Jan 30, 2026
@servo-highfive servo-highfive added the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Jan 30, 2026
Merged via the queue into servo:main with commit 8ed35f1 Jan 30, 2026
33 checks passed
@sagudev sagudev deleted the sm-report-mem branch January 30, 2026 18:59
@servo-highfive servo-highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Jan 30, 2026
github-merge-queue bot pushed a commit that referenced this pull request Feb 1, 2026
…ssociated memory in finalize instead of drop (#42271)

Reviewable per commits:

As noted in
#42180 (comment)
transmuting `&Reflector<AssociatedMemory>` to `&Reflector<()>` will
ignore AssociatedMemory information and thus it will not remove extra
associated memory. By returning `&Reflector<Self::ReflectorType>` from
`DomObject::reflector()` we will preserve this information and thus
correctly handle cases with associated memory. We also do not need
`overrideMemoryUsage` in bindings.conf anymore. 🎉

Instead of removing associated memory in drop code we should do it as
part of finalizers, otherwise we have problems in nested (inherited)
structs, where drop is run for both parent and child, but we only added
associated memory for child (on init_reflector) which already included
size of parent. The only exception here is promise, because it is RCed
and not finalized.

Testing: Tested locally that it fixes speedometer.
Fixes #42269

---------

Signed-off-by: sagudev <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Report memory pressure to SM

3 participants