(Again) Attempt to implement Locker for Isolate
#1620
Conversation
|
|
src/locker.rs
Outdated
|
|
||
| #[repr(C)] | ||
| #[derive(Debug)] | ||
| struct LockerHandle(Opaque); |
There was a problem hiding this comment.
Update: Remove the unused LockerHandle.
src/locker.rs
Outdated
|
|
||
| impl<'a> Locker<'a> { | ||
| /// Claims the isolate, this should only be used from a shared isolate. | ||
| pub(crate) fn new(isolate: &'a mut Isolate) -> Self { |
There was a problem hiding this comment.
i think this is UB, the locker should be acquired before a mutable reference is created.
There was a problem hiding this comment.
Update: set parameter to isolate: &Isolate (use unsafe cast to &mut Isolate in this new function).
src/locker.rs
Outdated
|
|
||
| #[repr(C)] | ||
| #[derive(Debug)] | ||
| pub(super) struct Locker([MaybeUninit<usize>; 2]); |
There was a problem hiding this comment.
with src/binding.hpp and crate::binding you can use the actual C++ definition instead of something that happens to hopefully be the correct size.
There was a problem hiding this comment.
Update: Use binding to set Locker size.
(When build in my local machine, there's an error report cannot find the v8__Locker__SIZE I newly added in binding.hpp, I'm trying to build it from source again)
src/isolate.rs
Outdated
|
|
||
| impl Drop for SharedIsolate { | ||
| fn drop(&mut self) { | ||
| let isolate = self.internal_unsafe_isolate_mut(); |
There was a problem hiding this comment.
it might be better of SharedIsolate just contains an OwnedIsolate, since the drop logic needs to stay in sync anyway.
There was a problem hiding this comment.
Update:
- Make SharedIsolate just contains an OwnedIsolate.
- Remove the
Droptrait for SharedIsolate, because I think SharedIsolate can automatically/implicitly implement it when it only contains aUnsafeCell<OwnedIsolate>.
NOTE: Actually I found this PR is out of my capabilities, i.e. I'm not 100% sure about what some code is doing. So please be careful about it.
| @@ -212,6 +212,10 @@ pub struct Global<T> { | |||
| isolate_handle: IsolateHandle, | |||
| } | |||
|
|
|||
| // Globals can go between threads as long as a locker was used to acquire a SharedIsolate. | |||
| unsafe impl<T> Send for Global<T> {} | |||
| unsafe impl<T> Sync for Global<T> {} | |||
There was a problem hiding this comment.
In @Earthmark 's previous PR #1411 , there is a comment saying, simply add Send and Sync trait to Global can be risk.
Could you also please give comments?
There was a problem hiding this comment.
I found there is a mutex with isolate inside the IsolateAnnex, see:
Line 1490 in b04640d
This code looks it wants to keep the data race on the isolate, makes me feel like using the Locker C++ api is the correct way to implement this part. Once this done, seems a Isolate can natively support Send and Sync traits, i.e. safely be sent between threads.
Correct?
There was a problem hiding this comment.
I completely forgot I had this change out here, thank you for picking it back up.
Let me know if you have questions about what I was trying to do.
I'm pretty sure the comment on the original pr about globals stands.
Globals moving between threads is a bit scary if lockers are not used, although it's kind of a landmine either way because the global needs to be made into a local with the same isolate that made it.
If lockers are not used, the global can only be used on the isolate that made it, which will be single threaded.
This usage constraint kind of enforces that the global is single threaded, but a global being send means it could be moved to another thread accidently (like if it's bundled in an async method), then dropped, which I think I vaguely recall could be a problem. (I haven't looked at this in months, and I'm writing this on my phone).
I think there are two paths with globals:
verify that dropping a global on the wrong thread won't explode (even in the lockers-not-used case)
OR
Make some other thread safe wrapper for globals that locked isolates can produce, that has whatever mechanisms are needed to not explode.
That being said, haven't looked at this in months.
I'm not sure if it's acceptable to make every isolate internally use lockers, they are re-entrant, but adding lockers to every API may add an unacceptable performance cost for users who are not using lockers.
I'm pretty sure lockers block as well, which is a bit nasty if someone wasn't expecting a hard block from an API call.
There was a problem hiding this comment.
Agree, too many API (in rusty_v8) are designed to run in a single thread at the beginning. And JavaScript language itself doesn't support such concepts like multi-thread and mutex.
So just keep it !Send and !Sync is more compatible with V8 and JavaScript language.
The best practice of using v8 along with multi-thread or async runtime should be: put v8 engine inside a single thread, and communicate with outer world with channels.
I close this PR now.
linrongbin16
changed the title
Locker for IsolateLocker for Isolate
|
The CI failure looks like a network issue when building V8 engine, not related to my code change. But I cannot re-run it, somebody help me? |
Resolve conflicts in #1411.
cc @Earthmark