-
Notifications
You must be signed in to change notification settings - Fork 303
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
Update v8 to include Promise Context Tagging patch #806
Conversation
6cd27dd
to
f3ff4c1
Compare
Note: The PR still needs tests added to ensure it's working correctly. I'll likely add the tests to the internal PR since it'll be easier. |
+ PromiseContextScope(const PromiseContextScope&) = delete; | ||
+ PromiseContextScope(PromiseContextScope&&) = delete; | ||
+ PromiseContextScope& operator=(const PromiseContextScope&) = delete; | ||
+ PromiseContextScope& operator=(PromiseContextScope&&) = delete; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
V8 would typically use DISALLOW_IMPLICIT_CONSTRUCTORS(PromiseContextScope)
for all but the move constructor here (not fanaticially used everywhere, jfyi).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall this patch LGTM. I've run v8 tests against the new patch 0012 for release and debug builds with no issues.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but probably best to have approval from Kenton for greater familiarity with most of the parts here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any performance impact for workers that don't use this?
Can we write a test case? I think the easiest way to do this would be to set up a test worker with a service binding (perhaps to an alternate entrypoint of itself). The test can then initiate multiple subrequests to the service binding and on the receiving end verify that they can synchronize with each other.
f3ff4c1
to
9d130d6
Compare
Internal PR updated with a test. |
9d130d6
to
c7d3b47
Compare
c7d3b47
to
7b3c5bb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand the V8 code so will trust @ohodson's review there.
Implements a mechanism in v8 that allows tagging a Promise with a creation context, then using that to implement cross-request promise awaits.
The way the mechanism works is fairly straightforward:
There are plenty of edge cases to work through to ensure that things are working but the fundamental idea appears to work with very little performance impact. I'll tag you both once I have the relevant PRs open.
The simple test case that I have been using to test that things work is simply:
Then using simply curl localhost:8080 & curl localhost:8080 to queue up simultaneous requests should be enough to verify that it basically works. I was using autocannon to throw more concurrent load to test that things work with gc. If all goes well, every request should be handled successfully and the fetch should only ever happen once.