search

microsoft / v8-jsi

React Native V8 JSI adapter
Other
157 stars 36 forks source link

Multi threaded support #147

Closed tudorms closed 2 years ago

tudorms commented 2 years ago

null

Microsoft Reviewers: Open in CodeFlow
mganandraj commented 2 years ago

isolate_->Enter();

In Chakra, we used to enter the VM on each methods in runtime and exit at the end of method .. With V8, it looks like the isolate stays bound to a single thread.. but we take a lock on each entry to a method, which i think ensures that the isolate is running something only in one thread ..

In V8, does it make sense to exit and enter the isolate on each entry point to the runtime ?

I hope other threads such as JIT/GC etc. are also respecting the locks ..

In reply to: 1273741990

Refers to: src/V8JsiRuntime.cpp:411 in 26bc84a. [](commit_id = 26bc84a567bcf3d005c59dd15bcb6d4c4c333b37, deletion_comment = False)

mganandraj commented 2 years ago

v8::Isolate *isolate = v8::Isolate::GetCurrent();

These methods can be called back during the remote-thread entry ? Does v8::Isolate::GetCurrent() return the correct isolate in that case ?

Refers to: src/V8JsiRuntime.cpp:69 in 26bc84a. [](commit_id = 26bc84a567bcf3d005c59dd15bcb6d4c4c333b37, deletion_comment = False)

tudorms commented 2 years ago

isolate_->Enter();

We do Enter and Exit on each entry point (Isolate::Scope is just a RAII that does Enter / Exit). This is needed here for simple cases where you create the Isolate and immediately use it for something (for example a few unit tests will fail if we remove it).

In reply to: 1273741990

Refers to: src/V8JsiRuntime.cpp:411 in 26bc84a. [](commit_id = 26bc84a567bcf3d005c59dd15bcb6d4c4c333b37, deletion_comment = False)

tudorms commented 2 years ago

v8::Isolate *isolate = v8::Isolate::GetCurrent();

I'm not sure this code is ever used. Tried setting enableMessageTracing = true but never hit this in the debugger.

In reply to: 1273743613

Refers to: src/V8JsiRuntime.cpp:69 in 26bc84a. [](commit_id = 26bc84a567bcf3d005c59dd15bcb6d4c4c333b37, deletion_comment = False)

vmoroz commented 2 years ago

It is a relatively big change. It would be nice to have an overview in the description of the PR.

vmoroz commented 2 years ago

Do we need a similar change for Node-API?

tudorms commented 2 years ago

Added it now (a bit ugly due to the opaque napienv thing, let me know if there's a cleaner approach).

In reply to: 1273855892

ghost commented 2 years ago

Hello @tudorms!

Because this pull request has the auto merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.