Skip to content
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

V8 lockers #28

Merged
merged 2 commits into from
Feb 12, 2020
Merged

V8 lockers #28

merged 2 commits into from
Feb 12, 2020

Conversation

darind
Copy link
Collaborator

@darind darind commented Feb 12, 2020

In this PR we are introducing v8::Locker objects to the codebase that allow accessing the isolate from multiple threads. This removes the need from marshalling javascript callbacks to the main thread.

@cla-bot cla-bot bot added the cla: yes label Feb 12, 2020
@darind darind merged commit c5f3971 into master Feb 12, 2020
@darind darind deleted the darind/v8-locker branch February 12, 2020 15:19
@farfromrefug
Copy link
Contributor

Very interesting. That could mean js scripts could run on a multi thread environment?

@NathanaelA

This comment was marked as abuse.

@darind
Copy link
Collaborator Author

darind commented Feb 13, 2020

@farfromrefug, yes this PR enables scripts to run in a multi-threaded environment.

For example the following script will print false:

const queue = NSOperationQueue.alloc().init();
queue.addOperationWithBlock(() => {
    // running on a background thread
    console.log(NSThread.currentThread.isMainThread); 
});

Of course this doesn't mean that scripts can run in parallel. And that's the big difference between this javascript and the corresponding native code:

[[[NSOperationQueue alloc] init] addOperationWithBlock:^{
    NSLog(@"%s", [[NSThread currentThread] isMainThread] ? "true" : "false");
}];

We have already discussed it but let's repeat it again so that there are no confusions or false expectations:

V8 isolates are not reentrant by design.

You can never run javascript on the same isolate from multiple threads in parallel. In the javascript sample above, if the main thread is busy executing some script, the background thread will simply wait until it finishes before it is able to acquire the lock and run. And again while the background thread is running your callback, the main thread will not be able to execute any javascript (it can execute some other stuff, but not javascript because it will not be able to acquire the lock).

So the bottomline from this is that if you really need parallel javascript execution, the proper way to achieve this is to use Workers (separate isolates).

@darind
Copy link
Collaborator Author

darind commented Feb 13, 2020

@NathanaelA, yes this can be applied to the Android runtime as well.

The problem is that this will be a catastrophic breaking change rendering the modules and existing plugins useless because (at least in Android) authors have always relied that the code they write in their javascript callbacks will run on the main thread.

The reason we are doing it for the V8 iOS beta runtime is simply to make it behave like the existing iOS runtime.

@farfromrefug
Copy link
Contributor

@darind I know it does not mean parallel and it is not what i want.
What i am looking for is the current IOS runtime behavior: JS callback are run from their native calling thread. Which is awesome! For example it means you can have realtime camera processing.

Yes i want exactly that for android and we already discussed that many times. And yes that change would mean all plugins would have to change their code to make the actual "runOnMainThread" when necessary.
But i do think it is a must have and it can actually mean an increase in performance because of the removal of runOnUiThread which is currently done in the android runtime for all JS calls.

BTW it would be an imrpovement for map plugins for example. Mapbox, CartoSDK, ... have callback which are actually not called on main thread. And it is intended so. In {N} those callbacks (JS side) are done on the UI thread which can slow down the UI.
Even if it "pauses" the JS thread because JS can't run in parallel this would be a huge improvement to have and would allow apps which are not possible today.

@NathanaelA

This comment was marked as abuse.

@darind
Copy link
Collaborator Author

darind commented Feb 13, 2020

@farfromrefug, agree with you that this can be beneficial to the Android runtime as well, but we do not have the resources to change all existing plugins. And even if we can do it for our plugins, we cannot force third party authors to do it. I am not sure that you realising the impact of such change on the ecosystem. We are not only speaking of making the physical change in some github repo. The most difficult part is testing it. Also plugins can be combined making the job of testing even harder.

@NathanaelA, the change would be wayyyy too substantial to be a candidate for a feature flag.

@farfromrefug
Copy link
Contributor

@darind i get what you mean. Now could we run some tests to actually see how it goes? I mean how hard it is to implement just runtime wise? Do you think we could have a branch to test the change? And maybe it could even be a option like "multiThread" which would be false by default.

I would give a lot to try/have this :D

@darind
Copy link
Collaborator Author

darind commented Feb 13, 2020

@farfromrefug, yes we can have a branch with the new functionality.

This being said, do not expect to be able to create a {N} application from this branch because the modules will not work. You will need to manually remove all javascript code that has any relation with the modules from your app.

@NathanaelA

This comment was marked as abuse.

@darind
Copy link
Collaborator Author

darind commented Feb 14, 2020

Yes this is a feature that we can consider for a possible NS7 release.

You can track the progress of the Android runtime update here: NativeScript/android#1587

@darind
Copy link
Collaborator Author

darind commented Feb 14, 2020

@farfromrefug, @NathanaelA : an early preview of the functionality you've asked for is available in the darind/v8-locker branch.

Corresponding PR: NativeScript/android#1587

Any feedback is welcome.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants