From 371adb9714ae3893d4639707c7e5e1674ee29b16 Mon Sep 17 00:00:00 2001 From: Haled Odat <8566042+HalidOdat@users.noreply.github.com> Date: Fri, 1 Dec 2023 00:53:37 +0100 Subject: [PATCH 1/2] Reduce `WeakGc` memory usage Makes inner storage of `WeakGc` store `()` in place of `Ephemeron` value, insteadthe of a clone of the `Gc` which is already stored in the key. Reducing the size by the width of one pointer. --- boa_gc/src/internals/ephemeron_box.rs | 18 +++++++++++++++--- boa_gc/src/pointers/ephemeron.rs | 13 ++++++++++++- boa_gc/src/pointers/gc.rs | 2 +- boa_gc/src/pointers/weak.rs | 20 ++++++++++++++------ 4 files changed, 42 insertions(+), 11 deletions(-) diff --git a/boa_gc/src/internals/ephemeron_box.rs b/boa_gc/src/internals/ephemeron_box.rs index 3db54195135..13618524af5 100644 --- a/boa_gc/src/internals/ephemeron_box.rs +++ b/boa_gc/src/internals/ephemeron_box.rs @@ -139,21 +139,33 @@ impl EphemeronBox { data.as_ref().map(|data| &data.value) } - /// Returns a reference to the ephemeron's key or None. + /// Returns the pointer to the ephemeron's key or None. /// /// # Safety /// /// The caller must ensure there are no live mutable references to the ephemeron box's data /// before calling this method. - pub(crate) unsafe fn key(&self) -> Option<&GcBox> { + pub(crate) unsafe fn key_ptr(&self) -> Option>> { // SAFETY: the garbage collector ensures the ephemeron doesn't mutate until // finalization. unsafe { let data = &*self.data.get(); - data.as_ref().map(|data| data.key.as_ref()) + data.as_ref().map(|data| data.key) } } + /// Returns a reference to the ephemeron's key or None. + /// + /// # Safety + /// + /// The caller must ensure there are no live mutable references to the ephemeron box's data + /// before calling this method. + pub(crate) unsafe fn key(&self) -> Option<&GcBox> { + // SAFETY: the garbage collector ensures the ephemeron doesn't mutate until + // finalization. + unsafe { self.key_ptr().map(|data| data.as_ref()) } + } + /// Marks this `EphemeronBox` as live. /// /// This doesn't mark the inner value of the ephemeron. [`ErasedEphemeronBox::trace`] diff --git a/boa_gc/src/pointers/ephemeron.rs b/boa_gc/src/pointers/ephemeron.rs index fd5fbab026a..d1ace87cb54 100644 --- a/boa_gc/src/pointers/ephemeron.rs +++ b/boa_gc/src/pointers/ephemeron.rs @@ -2,7 +2,7 @@ use crate::{ finalizer_safe, internals::EphemeronBox, trace::{Finalize, Trace}, - Allocator, Gc, + Allocator, Gc, GcBox, }; use std::ptr::NonNull; @@ -33,6 +33,17 @@ impl Ephemeron { unsafe { self.inner_ptr.as_ref().value().cloned() } } + /// Gets the stored value of this `Ephemeron`, or `None` if the key was already garbage collected. + /// + /// This needs to return a clone of the value because holding a reference to it between + /// garbage collection passes could drop the underlying allocation, causing an Use After Free. + #[must_use] + pub fn key_ptr(&self) -> Option>> { + // SAFETY: this is safe because `Ephemeron` is tracked to always point to a valid pointer + // `inner_ptr`. + unsafe { self.inner_ptr.as_ref().key_ptr() } + } + /// Checks if the [`Ephemeron`] has a value. #[must_use] pub fn has_value(&self) -> bool { diff --git a/boa_gc/src/pointers/gc.rs b/boa_gc/src/pointers/gc.rs index c49ae8b0f48..7a15f063e72 100644 --- a/boa_gc/src/pointers/gc.rs +++ b/boa_gc/src/pointers/gc.rs @@ -59,7 +59,7 @@ impl Gc { // `unsafe` block. // - `set_kv`: `weak` is a newly created `EphemeronBox`, meaning it isn't possible to // collect it since `weak` is still live. - unsafe { weak.inner().inner_ptr().as_mut().set(&gc, gc.clone()) } + unsafe { weak.inner().inner_ptr().as_mut().set(&gc, ()) } gc } diff --git a/boa_gc/src/pointers/weak.rs b/boa_gc/src/pointers/weak.rs index 1acd98b3db1..e80744a004d 100644 --- a/boa_gc/src/pointers/weak.rs +++ b/boa_gc/src/pointers/weak.rs @@ -8,7 +8,7 @@ use std::hash::{Hash, Hasher}; #[derive(Debug, Trace, Finalize)] #[repr(transparent)] pub struct WeakGc { - inner: Ephemeron>, + inner: Ephemeron, } impl WeakGc { @@ -16,7 +16,7 @@ impl WeakGc { #[must_use] pub fn new(value: &Gc) -> Self { Self { - inner: Ephemeron::new(value, value.clone()), + inner: Ephemeron::new(value, ()), } } @@ -24,7 +24,15 @@ impl WeakGc { /// if the value was already garbage collected. #[must_use] pub fn upgrade(&self) -> Option> { - self.inner.value() + let inner_ptr = self.inner.key_ptr()?; + + // SAFETY: Returned pointer is valid, so this is safe. + unsafe { + inner_ptr.as_ref().inc_ref_count(); + } + + // SAFETY: The gc pointer's reference count has been incremented, so this is safe. + Some(unsafe { Gc::from_raw(inner_ptr) }) } /// Check if the [`WeakGc`] can be upgraded. @@ -34,7 +42,7 @@ impl WeakGc { } #[must_use] - pub(crate) const fn inner(&self) -> &Ephemeron> { + pub(crate) const fn inner(&self) -> &Ephemeron { &self.inner } } @@ -47,8 +55,8 @@ impl Clone for WeakGc { } } -impl From>> for WeakGc { - fn from(inner: Ephemeron>) -> Self { +impl From> for WeakGc { + fn from(inner: Ephemeron) -> Self { Self { inner } } } From e7062b01fa74c901e80206fd87b9e88983b3bf70 Mon Sep 17 00:00:00 2001 From: Haled Odat <8566042+HalidOdat@users.noreply.github.com> Date: Fri, 1 Dec 2023 06:48:51 +0100 Subject: [PATCH 2/2] Apply review --- boa_gc/src/pointers/ephemeron.rs | 20 +++++++++++++------- boa_gc/src/pointers/weak.rs | 13 ++++--------- 2 files changed, 17 insertions(+), 16 deletions(-) diff --git a/boa_gc/src/pointers/ephemeron.rs b/boa_gc/src/pointers/ephemeron.rs index d1ace87cb54..e687ab53ad4 100644 --- a/boa_gc/src/pointers/ephemeron.rs +++ b/boa_gc/src/pointers/ephemeron.rs @@ -2,7 +2,7 @@ use crate::{ finalizer_safe, internals::EphemeronBox, trace::{Finalize, Trace}, - Allocator, Gc, GcBox, + Allocator, Gc, }; use std::ptr::NonNull; @@ -33,15 +33,21 @@ impl Ephemeron { unsafe { self.inner_ptr.as_ref().value().cloned() } } - /// Gets the stored value of this `Ephemeron`, or `None` if the key was already garbage collected. - /// - /// This needs to return a clone of the value because holding a reference to it between - /// garbage collection passes could drop the underlying allocation, causing an Use After Free. + /// Gets the stored key of this `Ephemeron`, or `None` if the key was already garbage collected. + #[inline] #[must_use] - pub fn key_ptr(&self) -> Option>> { + pub fn key(&self) -> Option> { // SAFETY: this is safe because `Ephemeron` is tracked to always point to a valid pointer // `inner_ptr`. - unsafe { self.inner_ptr.as_ref().key_ptr() } + let key_ptr = unsafe { self.inner_ptr.as_ref().key_ptr() }?; + + // SAFETY: Returned pointer is valid, so this is safe. + unsafe { + key_ptr.as_ref().inc_ref_count(); + } + + // SAFETY: The gc pointer's reference count has been incremented, so this is safe. + Some(unsafe { Gc::from_raw(key_ptr) }) } /// Checks if the [`Ephemeron`] has a value. diff --git a/boa_gc/src/pointers/weak.rs b/boa_gc/src/pointers/weak.rs index e80744a004d..179d394c10a 100644 --- a/boa_gc/src/pointers/weak.rs +++ b/boa_gc/src/pointers/weak.rs @@ -13,6 +13,7 @@ pub struct WeakGc { impl WeakGc { /// Creates a new weak pointer for a garbage collected value. + #[inline] #[must_use] pub fn new(value: &Gc) -> Self { Self { @@ -22,20 +23,14 @@ impl WeakGc { /// Upgrade returns a `Gc` pointer for the internal value if the pointer is still live, or `None` /// if the value was already garbage collected. + #[inline] #[must_use] pub fn upgrade(&self) -> Option> { - let inner_ptr = self.inner.key_ptr()?; - - // SAFETY: Returned pointer is valid, so this is safe. - unsafe { - inner_ptr.as_ref().inc_ref_count(); - } - - // SAFETY: The gc pointer's reference count has been incremented, so this is safe. - Some(unsafe { Gc::from_raw(inner_ptr) }) + self.inner.key() } /// Check if the [`WeakGc`] can be upgraded. + #[inline] #[must_use] pub fn is_upgradable(&self) -> bool { self.inner.has_value()