-
Notifications
You must be signed in to change notification settings - Fork 352
rework scopes to make them stack allocated and remove runtime bookkeeping of scopes #1830
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
8604e76
to
7c40c04
Compare
overall this seems alright, but we should probably wait until we are near cutting another major release to merge it. |
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. |
#[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) } | ||
} | ||
|
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
a6fefac
to
820307b
Compare
There was a problem hiding this 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
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before this PR,
HandleScope
s (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:Local
didn't outlive the scope it was created fromThe 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:
zombie
scope workaroundThe practical effect is that creating a handle scope changes from:
to:
If you accept a
HandleScope
as a parameter in a function, you would change it from:to
(
v8::PinScope
is a type alias for a pinned handle scope).