Skip to content

Conversation

nathanwhit
Copy link
Member

@nathanwhit nathanwhit commented Aug 6, 2025

Before this PR, HandleScopes (and other scope types) were stored in the heap and we did bookkeeping at runtime of the current stack of scopes. The reasons were primarily:

  • raw handle scopes need to be pinned in memory once initialized
  • we couldn't enforce at compile that a Local didn't outlive the scope it was created from
    The latter lead us to need a workaround where scopes actually were not exited when they were dropped, rather they were exited the next time we activated another scope.

This PR refactors the scope implementation to:

  • make scopes stack allocated, to reduce overhead of using them
  • remove the runtime bookkeeping
  • use lifetimes to prevent scopes from being dropped, so we can remove the zombie scope workaround

The practical effect is that creating a handle scope changes from:

let mut scope = v8::HandleScope::new(&mut isolate);
let local = v8::Integer::new(&mut scope, 123);

to:

let scope = std::pin::pin!(v8::HandleScope::new(&mut isolate));
let scope = scope.init();
let local = v8::Integer::new(&scope, 123);

// or shorter
v8::scope!(let scope, &mut isolate);
let local = v8::Integer::new(scope, 123);

If you accept a HandleScope as a parameter in a function, you would change it from:

fn takes_scope(s: &mut v8::HandleScope) {}

to

fn takes_scope(s: &mut v8::PinScope) {}

(v8::PinScope is a type alias for a pinned handle scope).

@nathanwhit nathanwhit force-pushed the scope-rework branch 2 times, most recently from 8604e76 to 7c40c04 Compare August 22, 2025 19:46
@nathanwhit nathanwhit changed the base branch from main to 137.2.1 August 22, 2025 21:14
@nathanwhit nathanwhit changed the base branch from 137.2.1 to main September 10, 2025 20:52
@nathanwhit nathanwhit changed the title scope rework rework scopes to make them stack allocated, and remove runtime bookkeeping of scopes Sep 16, 2025
@nathanwhit nathanwhit changed the title rework scopes to make them stack allocated, and remove runtime bookkeeping of scopes rework scopes to make them stack allocated and remove runtime bookkeeping of scopes Sep 16, 2025
@devsnek
Copy link
Member

devsnek commented Sep 16, 2025

overall this seems alright, but we should probably wait until we are near cutting another major release to merge it.

@bartlomieju
Copy link
Member

We talked about this PR during the meeting - we'd like to first fix Homebrew build and Windows debug symbols (denoland/deno_core#1191) before landing this one.

Comment on lines +778 to +878
#[repr(transparent)]
#[derive(Debug)]
pub struct Isolate(Opaque);
pub struct Isolate(NonNull<RealIsolate>);

#[repr(transparent)]
#[derive(Debug, Clone, Copy)]
pub struct UnsafeRawIsolatePtr(*mut RealIsolate);

impl UnsafeRawIsolatePtr {
pub fn null() -> Self {
Self(std::ptr::null_mut())
}

pub fn is_null(&self) -> bool {
self.0.is_null()
}
}

#[repr(C)]
pub struct RealIsolate(Opaque);

impl Isolate {
pub(crate) fn as_real_ptr(&self) -> *mut RealIsolate {
self.0.as_ptr()
}

pub unsafe fn as_raw_isolate_ptr(&self) -> UnsafeRawIsolatePtr {
UnsafeRawIsolatePtr(self.0.as_ptr())
}

#[inline]
pub unsafe fn from_raw_isolate_ptr(ptr: UnsafeRawIsolatePtr) -> Self {
Self(NonNull::new(ptr.0).unwrap())
}

#[inline]
pub unsafe fn from_raw_isolate_ptr_unchecked(
ptr: UnsafeRawIsolatePtr,
) -> Self {
Self(unsafe { NonNull::new_unchecked(ptr.0) })
}

pub unsafe fn from_raw_ptr_unchecked(ptr: *mut RealIsolate) -> Self {
Self(unsafe { NonNull::new_unchecked(ptr) })
}

pub unsafe fn from_raw_ptr(ptr: *mut RealIsolate) -> Self {
Self(NonNull::new(ptr).unwrap())
}

#[inline]
pub unsafe fn ref_from_raw_isolate_ptr(ptr: &UnsafeRawIsolatePtr) -> &Self {
if ptr.is_null() {
panic!("UnsafeRawIsolatePtr is null");
}
unsafe { &*(ptr as *const UnsafeRawIsolatePtr as *const Isolate) }
}

#[inline]
pub unsafe fn ref_from_raw_isolate_ptr_unchecked(
ptr: &UnsafeRawIsolatePtr,
) -> &Self {
unsafe { &*(ptr as *const UnsafeRawIsolatePtr as *const Isolate) }
}

#[inline]
pub unsafe fn ref_from_raw_isolate_ptr_mut(
ptr: &mut UnsafeRawIsolatePtr,
) -> &mut Self {
if ptr.is_null() {
panic!("UnsafeRawIsolatePtr is null");
}
unsafe { &mut *(ptr as *mut UnsafeRawIsolatePtr as *mut Isolate) }
}

#[inline]
pub unsafe fn ref_from_raw_isolate_ptr_mut_unchecked(
ptr: &mut UnsafeRawIsolatePtr,
) -> &mut Self {
unsafe { &mut *(ptr as *mut UnsafeRawIsolatePtr as *mut Isolate) }
}

#[inline]
pub(crate) unsafe fn from_non_null(ptr: NonNull<RealIsolate>) -> Self {
Self(ptr)
}

#[inline]
pub(crate) unsafe fn from_raw_ref(ptr: &NonNull<RealIsolate>) -> &Self {
// SAFETY: Isolate is a repr(transparent) wrapper around NonNull<RealIsolate>
unsafe { &*(ptr as *const NonNull<RealIsolate> as *const Isolate) }
}

#[inline]
pub(crate) unsafe fn from_raw_ref_mut(
ptr: &mut NonNull<RealIsolate>,
) -> &mut Self {
// SAFETY: Isolate is a repr(transparent) wrapper around NonNull<RealIsolate>
unsafe { &mut *(ptr as *mut NonNull<RealIsolate> as *mut Isolate) }
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Calling out these changes, as they're sort of orthogonal to the scope stuff and somewhat buried.

The idea is to only ever have raw pointers pointing to the actual isolate, and never create references to it. This allows us to avoid the aliasing invariants that would come with having an &mut RealIsolate. It also means that if the C++ does mutation through a the pointer, it is well defined even if the pointer came from an &Isolate.

Now, Isolate is just a wrapper around the raw pointer. We can still get the borrowing semantics we want, it's just a &mut NonNull<RealIsolate> instead of &mut RealIsolate. The only way to get an Isolate is from Isolate::new or unsafe code via UnsafeRawIsolatePtr (maybe should rename that)

Copy link
Member

@littledivy littledivy left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Member

@bartlomieju bartlomieju left a comment

Choose a reason for hiding this comment

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

LGTM, can you please address TODO from the description before landing?

Let's cut 140.2.0 after this is merged and update deno_core.

Copy link
Member

@littledivy littledivy left a comment

Choose a reason for hiding this comment

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

LGTM

@nathanwhit nathanwhit enabled auto-merge (squash) September 22, 2025 20:43
@nathanwhit nathanwhit merged commit 65fb5c1 into denoland:main Sep 22, 2025
18 checks passed
@nathanwhit nathanwhit deleted the scope-rework branch September 22, 2025 21:24
@nathanwhit nathanwhit restored the scope-rework branch September 22, 2025 21:25
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.

5 participants