From 20971e4c63032c9d174ba4bda70d72cf4217570f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Juli=C3=A1n=20Espina?= Date: Thu, 21 Sep 2023 23:06:06 +0000 Subject: [PATCH] Implement `Gc::new_cyclic` (#3292) * Add `Gc::new_cyclic` * Remove unrelated doc comment --- boa_gc/src/internals/ephemeron_box.rs | 33 ++++++++++++++++++++++---- boa_gc/src/pointers/ephemeron.rs | 3 ++- boa_gc/src/pointers/gc.rs | 34 +++++++++++++++++++++++++-- boa_gc/src/pointers/weak.rs | 8 ++++++- 4 files changed, 70 insertions(+), 8 deletions(-) diff --git a/boa_gc/src/internals/ephemeron_box.rs b/boa_gc/src/internals/ephemeron_box.rs index 361b9ebf8b4..3db54195135 100644 --- a/boa_gc/src/internals/ephemeron_box.rs +++ b/boa_gc/src/internals/ephemeron_box.rs @@ -100,6 +100,7 @@ struct Data { } impl EphemeronBox { + /// Creates a new `EphemeronBox` that tracks `key` and has `value` as its inner data. pub(crate) fn new(key: &Gc, value: V) -> Self { Self { header: EphemeronBoxHeader::new(), @@ -110,6 +111,14 @@ impl EphemeronBox { } } + /// Creates a new `EphemeronBox` with its inner data in the invalidated state. + pub(crate) fn new_empty() -> Self { + Self { + header: EphemeronBoxHeader::new(), + data: UnsafeCell::new(None), + } + } + /// Returns `true` if the two references refer to the same `EphemeronBox`. pub(crate) fn ptr_eq(this: &Self, other: &Self) -> bool { // Use .header to ignore fat pointer vtables, to work around @@ -121,8 +130,8 @@ impl EphemeronBox { /// /// # Safety /// - /// The garbage collector must not run between the call to this function and the eventual - /// drop of the returned reference, since that could free the inner value. + /// The caller must ensure there are no live mutable references to the ephemeron box's data + /// before calling this method. pub(crate) unsafe fn value(&self) -> Option<&V> { // SAFETY: the garbage collector ensures the ephemeron doesn't mutate until // finalization. @@ -134,8 +143,8 @@ impl EphemeronBox { /// /// # Safety /// - /// The garbage collector must not run between the call to this function and the eventual - /// drop of the returned reference, since that could free the inner value. + /// 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. @@ -153,6 +162,22 @@ impl EphemeronBox { self.header.mark(); } + /// Sets the inner data of the `EphemeronBox` to the specified key and value. + /// + /// # 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 set(&self, key: &Gc, value: V) { + // SAFETY: The caller must ensure setting the key and value of the ephemeron box is safe. + unsafe { + *self.data.get() = Some(Data { + key: key.inner_ptr(), + value, + }); + } + } + #[inline] pub(crate) fn inc_ref_count(&self) { self.header.ref_count.set(self.header.ref_count.get() + 1); diff --git a/boa_gc/src/pointers/ephemeron.rs b/boa_gc/src/pointers/ephemeron.rs index c8268f7cd98..fd5fbab026a 100644 --- a/boa_gc/src/pointers/ephemeron.rs +++ b/boa_gc/src/pointers/ephemeron.rs @@ -44,6 +44,7 @@ impl Ephemeron { impl Ephemeron { /// Creates a new `Ephemeron`. + #[must_use] pub fn new(key: &Gc, value: V) -> Self { let inner_ptr = Allocator::alloc_ephemeron(EphemeronBox::new(key, value)); Self { inner_ptr } @@ -72,7 +73,7 @@ impl Ephemeron { /// This function is unsafe because improper use may lead to memory corruption, double-free, /// or misbehaviour of the garbage collector. #[must_use] - const unsafe fn from_raw(inner_ptr: NonNull>) -> Self { + pub(crate) const unsafe fn from_raw(inner_ptr: NonNull>) -> Self { Self { inner_ptr } } } diff --git a/boa_gc/src/pointers/gc.rs b/boa_gc/src/pointers/gc.rs index 675cba7b005..c49ae8b0f48 100644 --- a/boa_gc/src/pointers/gc.rs +++ b/boa_gc/src/pointers/gc.rs @@ -1,8 +1,8 @@ use crate::{ finalizer_safe, - internals::GcBox, + internals::{EphemeronBox, GcBox}, trace::{Finalize, Trace}, - Allocator, + Allocator, Ephemeron, WeakGc, }; use std::{ cmp::Ordering, @@ -22,6 +22,7 @@ pub struct Gc { impl Gc { /// Constructs a new `Gc` with the given value. + #[must_use] pub fn new(value: T) -> Self { // Create GcBox and allocate it to heap. // @@ -34,6 +35,35 @@ impl Gc { } } + /// Constructs a new `Gc` while giving you a `WeakGc` to the allocation, to allow + /// constructing a T which holds a weak pointer to itself. + /// + /// Since the new `Gc` is not fully-constructed until `Gc::new_cyclic` returns, calling + /// [`upgrade`][WeakGc::upgrade] on the weak reference inside the closure will fail and result + /// in a `None` value. + #[must_use] + pub fn new_cyclic(data_fn: F) -> Self + where + F: FnOnce(&WeakGc) -> T, + { + // SAFETY: The newly allocated ephemeron is only live here, meaning `Ephemeron` is the + // sole owner of the allocation after passing it to `from_raw`, making this operation safe. + let weak = unsafe { + Ephemeron::from_raw(Allocator::alloc_ephemeron(EphemeronBox::new_empty())).into() + }; + + let gc = Self::new(data_fn(&weak)); + + // SAFETY: + // - `as_mut`: `weak` is properly initialized by `alloc_ephemeron` and cannot escape the + // `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()) } + + gc + } + /// Consumes the `Gc`, returning a wrapped raw pointer. /// /// To avoid a memory leak, the pointer must be converted back to a `Gc` using [`Gc::from_raw`]. diff --git a/boa_gc/src/pointers/weak.rs b/boa_gc/src/pointers/weak.rs index 38596b7f24b..1acd98b3db1 100644 --- a/boa_gc/src/pointers/weak.rs +++ b/boa_gc/src/pointers/weak.rs @@ -20,7 +20,8 @@ impl WeakGc { } } - /// Upgrade returns a `Gc` pointer for the internal value if valid, or None if the value was already garbage collected. + /// Upgrade returns a `Gc` pointer for the internal value if the pointer is still live, or `None` + /// if the value was already garbage collected. #[must_use] pub fn upgrade(&self) -> Option> { self.inner.value() @@ -31,6 +32,11 @@ impl WeakGc { pub fn is_upgradable(&self) -> bool { self.inner.has_value() } + + #[must_use] + pub(crate) const fn inner(&self) -> &Ephemeron> { + &self.inner + } } impl Clone for WeakGc {