script: Add basic memory pressure reporting to SpiderMonkey#42180
script: Add basic memory pressure reporting to SpiderMonkey#42180sagudev merged 5 commits intoservo:mainfrom
Conversation
|
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. |
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. |
|
The aborted branch: Detailsdiff --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 |
|
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? |
d6462b3 to
4fdf459
Compare
Signed-off-by: sagudev <[email protected]>
Signed-off-by: sagudev <[email protected]>
Signed-off-by: sagudev <[email protected]>
Signed-off-by: sagudev <[email protected]>
Signed-off-by: sagudev <[email protected]>
4fdf459 to
5f09140
Compare
| 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) } | ||
| } | ||
| } |
There was a problem hiding this comment.
Aha, clever. I got stuck on figuring out how to avoid this.
There was a problem hiding this comment.
Hm, using this for Reflector that has AsociatedMemory will not work the way we want.
…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]>
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 withupdate_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