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

Remove v8::Locker #272

Merged
merged 9 commits into from Feb 11, 2020
Merged

Remove v8::Locker #272

merged 9 commits into from Feb 11, 2020

Conversation

@ry
Copy link
Contributor

ry commented Feb 11, 2020

This patch clarifies that v8::Isolate is a single threaded creature
which can only be accessed from other threads in special circumstances.
To ensure optimal operation in Deno, we remove v8::Locker, which ought
to be unnecessary when a thread is dedicated to each Isolate and the
Isolates never move between threads.

There are valid use-cases for v8::Locker, and we hope to address them in
future revisions.

ry added 6 commits Feb 11, 2020
This patch clarifies that v8::Isolate is a single threaded creature,
which can only be accessed from other threads in special circumstances.
To ensure optimal operation in Deno, we remove v8::Locker, which ought
to be unnecessary when a thread is dedicated to each Isolate and the
Isolates never move between threads.

There are valid use-cases for v8::Locker, and we hope to address them in
future revisions.
@ry ry changed the title Remove v8::Locker add IsolateHandle Remove v8::Locker and add IsolateHandle Feb 11, 2020
@ry ry requested a review from piscisaureus Feb 11, 2020
src/isolate.rs Outdated Show resolved Hide resolved
src/isolate.rs Outdated Show resolved Hide resolved
src/isolate.rs Outdated Show resolved Hide resolved
src/isolate.rs Outdated Show resolved Hide resolved
@piscisaureus

This comment has been minimized.

Copy link
Contributor

piscisaureus commented Feb 11, 2020

Can this patch be split in two parts, one that removes locker, one that does everything else?

@ry

This comment has been minimized.

Copy link
Contributor Author

ry commented Feb 11, 2020

@piscisaureus As requested, I've split it into Locker removal only. There is a bit of entanglement, so a test was disabled. (Marked your comments regarding IsolateHandle resolved, since they're for another PR...)

@ry ry changed the title Remove v8::Locker and add IsolateHandle Remove v8::Locker Feb 11, 2020
Copy link
Contributor

piscisaureus left a comment

LGTM
Please note: I pushed extra commits.

@ry ry merged commit 32abe84 into denoland:master Feb 11, 2020
4 checks passed
4 checks passed
macos-latest macos-latest
Details
ubuntu-16.04 ubuntu-16.04
Details
windows-2019 windows-2019
Details
license/cla Contributor License Agreement is signed.
Details
@ry ry deleted the ry:remove_locker2 branch Feb 11, 2020
@ry ry mentioned this pull request Feb 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.