-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Add Atomics.waitAsync host hook #4613
Conversation
For context, the purpose of this PR is to write out how the Atomics.waitAsync proposal could use HTML to schedule the resolution of a Promise. That proposal is currently at Stage 2; this PR shouldn't land until Atomics.waitAsync reaches Stage 3 (at which point we will have more information about implementer interest). |
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.
This looks good editorially and I think model-wise it's also fine. I guess there's a question if the host needs to be involved in the initial transfer of promiseCapibility across the agent boundary, but probably that's fine for now.
The promiseCapability is never inspected across agents; it's just put in a Record that's on a cross-agent list, and then pulled out of that and passed back to the original agent. Where do you think the host should be involved? |
It's mostly that the host is responsible for allocation of agents and their clusters, and coordinating some things between them, but JavaScript is also responsible for coordinating some things between them. Them both doing similar things seems wrong conceptually. |
@annevk The issue at hand that causes both the host and JS to coordinate agents is that JS wishes to impose a fairness constraint in |
It sounds like we should make a separate issue for this topic, because we're really talking about SharedArrayBuffer/Atomics in general (which is where these WaiterLists come from), not Atomics.waitAsync (which just extends them a little). |
Sorry, it's a rather abstract concern and I'd share ideas if I had them. Perhaps it's not possible to separate all concerns cleanly or perhaps JS will eventually have a sufficiently capable API to address all needs. [I missed @littledan's comment. That seems reasonable once someone has a more concrete handle on what to do, but agreed that we don't need to discuss it further here.] |
Note, this PR should be rephrased before it lands, along the lines of tc39/ecma262#1597 . |
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.
FYI, Chrome is planning to ship Atomics.waitAsync
in 87.
But we should still hold off on merging this, since the rule for merging is 2+ shipping implementations, is that right?
Per https://whatwg.org/working-mode#changes the rule is that it must have implementation support, which this feature has. Test coverage and implementation bugs (probably references to existing bugs) are also required. It seems this also needs rebasing. |
@littledan Per our offline discussion I tried to rebase + update your branch myself, but since I don't have the right bits for this repo or your repo, I can't push to your branch. |
@syg you should have an invite that will give you write access. |
09d97e3
to
6587654
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.
Small nit
Oh, I was not aware that IDL Promises are actually capabilities. I see, I'll revert then. |
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
What's the proposed commit message? (Please put it in a comment here. We'll use squash and merge.) And please detail the task source decision there a bit. I'll push a fixup commit for formatting. Are there web-platform-tests for this? (Maybe in general update OP a bit with the latest here.) |
Not yet. |
Proposed commit message:
Unrelatedly, we're still waiting on WPT, but once those land this should be good to go. |
Rebased with two host hooks following the latest pending-Stage-4 PR.
|
Resolved earlier threads that were out of date. |
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.
Thanks @syg! This looks good to me overall.
I guess we should wait for tc39/ecma262#3049 to land first. |
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.
As far as I can tell this all seems to work. An important consideration is that neither of the two newly-introduced jobs run arbitrary ECMAScript, and so we don't need to worry about incumbent/entry stuff, or scripting being disabled, like HostEnqueuePromiseJob and HostEnqueueFinalizationRegistryCleanupJob do.
As such, I'm a little worried about the generic names here being reused in the future for something that does run script, without us making the appropriate updates over here. But I'm OK with it for now; everyone's been very diligent so far.
source
Outdated
@@ -100766,6 +100786,26 @@ dictionary <dfn dictionary>PromiseRejectionEventInit</dfn> : <span>EventInit</sp | |||
</li> | |||
</ol> | |||
|
|||
<h6 id="hostenqueuetimeoutjob"><dfn>HostEnqueueTimeoutJob</dfn>(<var>job</var>, | |||
<var>milliseconds</var>)</h6> |
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'm surprised that unlike all the other HostEnqueueXJobs, this one doesn't take a realm. (Either directly, like HostEnqueueGenericJob and HostEnqueuePromiseJob, or indirectly, like HostEnqueueFinalizationRegistryCleanupJob.) I guess it's fine if that works for ECMAScript, though.
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.
Good point. It sounds beneficial to me to enforce discipline here to require all HostEnqueueFooJobs hooks to pass a realm. We need to balance churn here though. I'll drop by a future HTML triage meeting to chat about it if that's a proper venue.
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.
That works, but also happy to chat async, either here or in https://whatwg.org/chat during our overlap time.
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've added a realm argument to this hook.
@syg no particular rush, but I'm wondering what the latest status on this is? |
It was waiting on tc39/ecma262#3049 to land, and now it has, so this is also ready to land. |
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!
I guess we still need tests and implementer bugs filed. I'm not sure whether @syg can edit the OP or not, but placing them in a comment would suffice. I will merge after that is done.
The IPR checker notes that @littledan is not signed up to contribute to WHATWG Standards right now. However, he has no commits on this branch, so, we can disregard it.
If @littledan makes his membership of the Bloomberg org public that should work out. Note that if you merge this GitHub will at least partially attribute this to him so we probably want to be a little careful that @syg at least gets attributed. |
Done. WPT landed a while back, and I put up links to the browser vendor bugs. |
Implements HostResolveCrossAgentJob and HostEnqueueTimeoutJob.
9d84bf4
to
fdf691d
Compare
This host hook queues a task in the appropriate agent to resolve a Promise.
Corresponding PR: tc39/proposal-atomics-wait-async#7
(See WHATWG Working Mode: Changes for more details.)
/index.html ( diff )
/infrastructure.html ( diff )
/references.html ( diff )
/webappapis.html ( diff )
/index.html ( diff )
/infrastructure.html ( diff )
/webappapis.html ( diff )