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

Add Atomics.waitAsync host hook #4613

Merged
merged 1 commit into from Jun 22, 2023
Merged

Conversation

littledan
Copy link
Contributor

@littledan littledan commented May 11, 2019

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 )

@littledan
Copy link
Contributor Author

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).

Copy link
Member

@annevk annevk left a 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.

source Outdated Show resolved Hide resolved
@annevk annevk added the do not merge yet Pull request must not be merged per rationale in comment label May 14, 2019
@littledan
Copy link
Contributor Author

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?

@annevk
Copy link
Member

annevk commented May 14, 2019

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.

@syg
Copy link
Collaborator

syg commented May 14, 2019

@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 Atomics.wait and Atomics.waitAsync. It would be bug prone to leave the scheduling algorithm to the host in all cases, or split the algorithm between JS and the host. And besides, we want it to be fair in the same way for all hosts. Any recommendations here?

@littledan
Copy link
Contributor Author

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).

@annevk
Copy link
Member

annevk commented May 14, 2019

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.]

source Outdated Show resolved Hide resolved
@littledan
Copy link
Contributor Author

Note, this PR should be rephrased before it lands, along the lines of tc39/ecma262#1597 .

Copy link
Collaborator

@syg syg left a 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?

source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
@annevk
Copy link
Member

annevk commented Sep 16, 2020

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.

@syg
Copy link
Collaborator

syg commented Sep 17, 2020

@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.

@annevk
Copy link
Member

annevk commented Sep 18, 2020

@syg you should have an invite that will give you write access.

source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
Copy link
Contributor Author

@littledan littledan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small nit

@syg
Copy link
Collaborator

syg commented Sep 22, 2020

Oh, I was not aware that IDL Promises are actually capabilities. I see, I'll revert then.

Copy link
Contributor Author

@littledan littledan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@annevk
Copy link
Member

annevk commented Sep 28, 2020

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.)

@annevk annevk changed the title [WIP] Add Atomics.waitAsync host hook Add Atomics.waitAsync host hook Sep 28, 2020
@syg
Copy link
Collaborator

syg commented Sep 28, 2020

Are there web-platform-tests for this? (Maybe in general update OP a bit with the latest here.)

Not yet. I'll try to adopt some of the test262 tests into WPT tests. @marjakh will write some WPT tests for the integration.

Base automatically changed from master to main January 15, 2021 07:57
@syg
Copy link
Collaborator

syg commented Apr 26, 2021

Proposed commit message:

Integrate with JavaScript Atomics.waitAsync

The role of HTML here is for facilitating resolving of Promises vended by Atomics.waitAsync across agents.

Co-authored-by: Shu-yu Guo <syg@chromium.org>

Unrelatedly, we're still waiting on WPT, but once those land this should be good to go.

@syg
Copy link
Collaborator

syg commented May 5, 2023

Rebased with two host hooks following the latest pending-Stage-4 PR.

  • HostEnqueueCrossAgentJob that enqueues a job in another agent, used for explicitly notified cross-agent Atomics.waitAsync promises.
  • HostEnqueueTimeoutJob that enqueues a job in the same agent after a timeout, used for Atomics.waitAsync promises with timeouts.

@syg
Copy link
Collaborator

syg commented May 5, 2023

Resolved earlier threads that were out of date.

Copy link
Member

@annevk annevk left a 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.

source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
@annevk
Copy link
Member

annevk commented May 12, 2023

I guess we should wait for tc39/ecma262#3049 to land first.

Copy link
Member

@domenic domenic left a 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>
Copy link
Member

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.

Copy link
Collaborator

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.

Copy link
Member

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.

Copy link
Collaborator

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.

@domenic
Copy link
Member

domenic commented Jun 1, 2023

@syg no particular rush, but I'm wondering what the latest status on this is?

@syg
Copy link
Collaborator

syg commented Jun 14, 2023

@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.

@domenic domenic added topic: script and removed do not merge yet Pull request must not be merged per rationale in comment labels Jun 16, 2023
Copy link
Member

@domenic domenic left a 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.

@annevk
Copy link
Member

annevk commented Jun 16, 2023

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.

@syg
Copy link
Collaborator

syg commented Jun 16, 2023

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.

Done. WPT landed a while back, and I put up links to the browser vendor bugs.

Implements HostResolveCrossAgentJob and HostEnqueueTimeoutJob.
@domenic domenic merged commit 8a32a33 into whatwg:main Jun 22, 2023
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

6 participants