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 from deno_respond #2664

Merged
merged 1 commit into from Jul 18, 2019

Conversation

@afinch7
Copy link
Contributor

commented Jul 18, 2019

This looks unnecessary, but I'm not 100% sure.

@ry

ry approved these changes Jul 18, 2019

Copy link
Collaborator

left a comment

LGTM - if it passes the tests it's fine by me. Thanks!

(It's possible this may introduce a segfault somewhere, but if so it's something we should be testing for. They're relatively easy crashes to figure out with a debug build.)

@ry ry merged commit 042484d into denoland:master Jul 18, 2019

3 checks passed

Travis CI - Pull Request Build Passed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
license/cla Contributor License Agreement is signed.
Details
@ry

This comment has been minimized.

Copy link
Collaborator

commented Jul 18, 2019

The benchmarks have been published - there is no noticeable effect from removing this.

@kevinkassimo

This comment has been minimized.

Copy link
Contributor

commented Jul 18, 2019

Also for this one: https://github.com/denoland/deno/blob/master/core/isolate.rs#L554
Do we need to hold the lock for the whole period of poll? (Haven't actually investigated myself)

Also I think deno_respond lock is safe to remove due to this outer lock

@ry

This comment has been minimized.

Copy link
Collaborator

commented Jul 18, 2019

@kevinkassimo The idea is that during poll we're calling into JS many times, so it's better to acquire the lock one for all of them rather than in each individual call.

(Looking at the Rust code now, I worry that the Rust compiler might try to free _locker before poll completes. Probably we should add drop(_locker) at the bottom.)

@afinch7

This comment has been minimized.

Copy link
Contributor Author

commented Jul 18, 2019

@kevinkassimo I don't think so. I tried it, and it seems to work fine.
@ry It looks like we only call into Js when we have completed ops, and only call twice when we overflow the shared queue.

ry referenced this pull request Jul 19, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants
You can’t perform that action at this time.