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

Normative: Add Atomics.waitAsync #3049

Merged
merged 1 commit into from
Jun 14, 2023
Merged

Normative: Add Atomics.waitAsync #3049

merged 1 commit into from
Jun 14, 2023

Conversation

syg
Copy link
Contributor

@syg syg commented Apr 21, 2023

  • Shipped in Chrome 87 and Safari 16.4.
  • HTML PR to implement hooks.
  • test262

@syg syg added normative change Affects behavior required to correctly evaluate some ECMAScript source text pending stage 4 This proposal has not yet achieved stage 4, but may otherwise be ready to merge. proposal This is related to a specific proposal, and will be closed/merged when the proposal reaches stage 4. editor call to be discussed in the next editor call labels Apr 21, 2023
spec.html Outdated Show resolved Hide resolved
@syg syg requested a review from a team April 21, 2023 00:15
spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
spec.html Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
@syg syg removed the editor call to be discussed in the next editor call label Apr 26, 2023
spec.html Outdated
<p>At any time, an ECMAScript implementation may perform the following steps atomically:</p>
<emu-alg>
1. Let _now_ be the time value (UTC) identifying the current time.
1. For each WaiterList _WL_, do
Copy link
Collaborator

Choose a reason for hiding this comment

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

So every WaiterList in every agent cluster?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

...yeah?

spec.html Outdated
<emu-alg>
1. Let _now_ be the time value (UTC) identifying the current time.
1. For each WaiterList _WL_, do
1. Perform EnterCriticalSection(_WL_).
Copy link
Collaborator

Choose a reason for hiding this comment

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

The first step of EnterCriticalSection asserts that the surrounding agent is not in the critical section for any WaiterList Record, so there's a couple problems:

  • As it stands, there's no surrounding agent.
  • Even if there were, you haven't ensured that it's not in any critical section.

Also, it seems odd to me that an agent would enter the critical section of a WaiterList Record that it isn't waiting on.


The second step of EnterCriticalSection says to wait until no agent is in _WL_'s critical section, so it's unclear how these steps can be performed atomically.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why isn't there a surrounding agent? This runs in some agent.

Good call on the "atomically" thing, which seems to be wrong since the critical section already ensures mutual exclusion.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Why isn't there a surrounding agent? This runs in some agent.

"While an agent's executing thread executes jobs, the agent is the surrounding agent for the code in those jobs." Are these steps being performed as part of a job?

It seems wrong that one agent in one agent cluster would be tweaking WaiterLists in all agent clusters. I would expect that an agent can't affect any agent cluster other than its own.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh agent cluster. Sure we can qualify it as "intra agent cluster" but that seems definitionally true. WaiterLists are associated with SAB blocks, so how could an agent have gotten WaiterLists from another agent cluster?

This is the same thing as whatever finalization registry is doing, which is admittedly handwavy but I don't really see a problem. I don't think there's any confusion there that the set of non-live objects S is across agents, just like here it's not across agent clusters.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"While an agent's executing thread executes jobs, the agent is the surrounding agent for the code in those jobs." Are these steps being performed as part of a job?

...yes, sure?

Copy link
Collaborator

Choose a reason for hiding this comment

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

any agent at any time can check if any async waiter in its agent cluster has had its timeout expired, and if so, call the host hook.

Ah, via NotifyWaiter, got it.

The Job execution guarantees are for the enqueued Job that resolves the promise, not for the code that calls the host to enqueue the Job.

(That's assuming that the latter isn't itself a job, which I gather is how you're thinking about it now. But earlier, you indicated it was a job, so that's what I based my question on.)

So, updated question: if the algorithm of "Processing Model of Atomics.waitAsync" isn't part of a job, then the job-execution guarantees don't apply, so how does it satisfy EnterCriticalSection's assertion that the surrounding agent is not in any critical section?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, updated question: if the algorithm of "Processing Model of Atomics.waitAsync" isn't part of a job, then the job-execution guarantees don't apply, so how does it satisfy EnterCriticalSection's assertion that the surrounding agent is not in any critical section?

Yeah, that's true, there's no re-entrancy guard currently.

We can make those steps have job-like guarantees without it making a job, I guess? Another alternative formulation is perhaps to do away with the current host hook entirely and directly encode the the timeout and promise fulfillment into the "processing model". Something like,

At any time in an agent _agent_, when the execution context stack is empty, an implementation may
perform the following steps atomically:

1. For each WaiterList _WL_, do
  1. Enter critical section for _WL_.
  1. For each _waiter_ in _WL_ whose [[AgentSignifier]] is _agent_'s signifier, do
    1. If timeout expired, resolve the [[PromiseCapability]]. (We can do this directly since it's the right agent.)
  1. Exit the critical section for _WL_.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We can make those steps have job-like guarantees without it making a job, I guess?

But then why not just say it is a job? I.e., is there something about the definition/handling of a job that needs to not apply to this algorithm?

Another alternative formulation is perhaps to do away with the current host hook entirely and directly encode the the timeout and promise fulfillment into the "processing model".
[...]
1. If timeout expired, resolve the [[PromiseCapability]]. (We can do this directly since it's the right agent.)

So that 'resolve' would be something like:
Call(_waiter_.[[PromiseCapability]].[[Resolve]], *undefined*, « *"timed-out"* »).
?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But then why not just say it is a job? I.e., is there something about the definition/handling of a job that needs to not apply to this algorithm?

We can. My initial hesitation was from how the HostEnqueueResolveInAgentJob hook was originally formulated. All that job does is to resolve the promise in the right agent, because an agent can only resolve its own promises. So to make the timer watchdog thing itself a job, like HostEnqueueWatchdogTimerJobForSomeWaiter, we'd either need 2 jobs or something more complicated, like a requirement that the host re-enqueue the job if the timer is not yet expired.

The HostEnqueueWatchdogTimerJobForSomeWaiter thing is also fine by me, and is probably closer to how real implementations work. Now that I've talked through it, I think that version is the cleanest on top of our existing machinery without having to modify much.

So that 'resolve' would be something like:
Call(waiter.[[PromiseCapability]].[[Resolve]], undefined, « "timed-out" »).
?

Yep.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I refactored the timeout thing to a Job and removed the "processing model" section entirely. PTAL.

spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
@syg syg added the editor call to be discussed in the next editor call label Apr 28, 2023
@@ -11982,12 +11982,12 @@ <h1>InitializeHostDefinedRealm ( ): either a normal completion containing ~unuse
<emu-clause id="sec-agents">
<h1>Agents</h1>

<p>An <dfn id="agent" variants="agents">agent</dfn> comprises a set of ECMAScript execution contexts, an execution context stack, a running execution context, an <dfn id="agent-record" variants="Agent Records">Agent Record</dfn>, and an <dfn id="executing-thread" variants="executing threads">executing thread</dfn>. Except for the executing thread, the constituents of an agent belong exclusively to that agent.</p>
<p>An agent's executing thread executes a job on the agent's execution contexts independently of other agents, except that an executing thread may be used as the executing thread by multiple agents, provided none of the agents sharing the thread have an Agent Record whose [[CanBlock]] field is *true*.</p>
<p>An <dfn id="agent" variants="agents">agent</dfn> comprises a set of ECMAScript execution contexts, an execution context stack, a running execution context, an <dfn id="agent-record" variants="Agent Records">Agent Record</dfn>, and an <dfn id="executing-thread" variants="executing threads">executing thread</dfn>. Except for the executing thread, Shared Data Blocks, and specification values related to Shared Data Blocks, the constituents of an agent belong exclusively to that agent.</p>
Copy link
Collaborator

Choose a reason for hiding this comment

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

This insertion is odd. Shared Data Blocks etc weren't just listed as constituents of an agent, so it doesn't make sense to say that they're among the constituents that aren't exclusive to the agent.

I think it would be more helpful to expand the Agent Clusters section to say that every Shared Data Block etc 'belongs to' an agent cluster. (Or, an agent cluster comprises zero or more Shared Data Blocks, WaiterList Records, Waiter Records, whatever.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good suggestion, I've added a list of spec values to the agent cluster section.

<p>An <dfn id="agent" variants="agents">agent</dfn> comprises a set of ECMAScript execution contexts, an execution context stack, a running execution context, an <dfn id="agent-record" variants="Agent Records">Agent Record</dfn>, and an <dfn id="executing-thread" variants="executing threads">executing thread</dfn>. Except for the executing thread, the constituents of an agent belong exclusively to that agent.</p>
<p>An agent's executing thread executes a job on the agent's execution contexts independently of other agents, except that an executing thread may be used as the executing thread by multiple agents, provided none of the agents sharing the thread have an Agent Record whose [[CanBlock]] field is *true*.</p>
<p>An <dfn id="agent" variants="agents">agent</dfn> comprises a set of ECMAScript execution contexts, an execution context stack, a running execution context, an <dfn id="agent-record" variants="Agent Records">Agent Record</dfn>, and an <dfn id="executing-thread" variants="executing threads">executing thread</dfn>. Except for the executing thread, Shared Data Blocks, and specification values related to Shared Data Blocks, the constituents of an agent belong exclusively to that agent.</p>
<p>An agent's executing thread executes code on the agent's execution contexts independently of other agents, except that an executing thread may be used as the executing thread by multiple agents, provided none of the agents sharing the thread have an Agent Record whose [[CanBlock]] field is *true*.</p>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you explain your thinking behind this change and the one below?

I tried writing a comment, but I'm having trouble without knowing your intent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The intention is to make this section more generic than about jobs. The history wrt jobs IIRC was that we had a very generic job queue scheduler system that no embedder actually used. This was dead code that no host used, so it was very misleading, and we removed it. So when agents talk about "jobs" I'm pretty sure they're talking about this old notion of a job that has been ripped out.

Since then we've redefined a "Job" to be something more specific: an Abstract Closure that's given to the host to schedule and run. So it seems odd to me to say that "surrounding agent" is only well-defined when an agent is executing a Job. The initial script evaluation, for instance, is not part of a Job in the Abstract Closure sense.

So my intention here was to make the notion of agent executing JS code to be more broad than just the restricted notion of Job. I think "executes code" suffices to broad enough. I had originally thought something like "evaluating ECMAScript code", but I want the "surrounding agent" notion to be well-defined in the waitAsync timer stuff, which is arguably not executing ECMAScript code. Thoughts?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks.

I think "executes code" suffices to broad enough. I had originally thought something like "evaluating ECMAScript code", but I want the "surrounding agent" notion to be well-defined in the waitAsync timer stuff, which is arguably not executing ECMAScript code. Thoughts?

In the spec, the word "code" (outside of "code unit" and "code point") almost always means ECMAScript source code anyway, so "executes code" doesn't have the meaning you want. I think "executes algorithmic steps" would be better. (You could maybe drop the "algorithmic" for subsequent uses.)

Suggested change
<p>An agent's executing thread executes code on the agent's execution contexts independently of other agents, except that an executing thread may be used as the executing thread by multiple agents, provided none of the agents sharing the thread have an Agent Record whose [[CanBlock]] field is *true*.</p>
<p>An agent's executing thread executes algorithmic steps on the agent's execution contexts independently of other agents, except that an executing thread may be used as the executing thread by multiple agents, provided none of the agents sharing the thread have an Agent Record whose [[CanBlock]] field is *true*.</p>

or maybe "executes algorithms".


The intention is to make this section more generic than about jobs.

I'm okay with the idea that an agent can execute a slightly broader class of thing than just jobs, but I'd like it to be clear that a job is one of the things it can execute. Currently, the only explicit connection between 'agent' and 'job' is in this para and the next, and this PR is dissolving that connection. Maybe somewhere near the definition of job, it could say that a job is executed by an agent. (See also issue #2642.)

The history wrt jobs IIRC was that we had a very generic job queue scheduler system that no embedder actually used. This was dead code that no host used, so it was very misleading, and we removed it.

Yup, that was PR #1597.

So when agents talk about "jobs" I'm pretty sure they're talking about this old notion of a job that has been ripped out.

Well, except #1597 also changed these paragraphs, so presumably they were modified to talk about the new notion of a job.

Since then we've redefined a "Job" to be something more specific: an Abstract Closure that's given to the host to schedule and run.

That was also done in #1597.

So it seems odd to me to say that "surrounding agent" is only well-defined when an agent is executing a Job. The initial script evaluation, for instance, is not part of a Job in the Abstract Closure sense.

I agree it's odd. (#1597 dropped ScriptEvaluationJob without saying that an agent could execute non-jobs like ScriptEvaluation.)

But if an agent can execute non-jobs, are there any restrictions on when/how it executes them? (E.g., can an agent start executing a ScriptEvaluation while another is in progress?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added a bit to the Job section about being executed by agents.

spec.html Show resolved Hide resolved
spec.html Show resolved Hide resolved
spec.html Show resolved Hide resolved
spec.html Show resolved Hide resolved
spec.html Show resolved Hide resolved
spec.html Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
@syg syg removed the editor call to be discussed in the next editor call label May 3, 2023
spec.html Show resolved Hide resolved
jmdyck
jmdyck previously requested changes May 5, 2023
spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
@jmdyck
Copy link
Collaborator

jmdyck commented May 5, 2023

Also, side note: I like how HostEnqueueCrossAgentJob and HostEnqueueTimeoutJob are now just stubs; all the pseudocode has been hoisted out to the caller. Could we do something similar for HostEnqueueFinalizationRegistryCleanupJob (not in this PR)?

@syg
Copy link
Contributor Author

syg commented May 8, 2023

Also, side note: I like how HostEnqueueCrossAgentJob and HostEnqueueTimeoutJob are now just stubs; all the pseudocode has been hoisted out to the caller. Could we do something similar for HostEnqueueFinalizationRegistryCleanupJob (not in this PR)?

Good idea, we should do something similar for FinalizationRegistryCleanupJob.

jmdyck added a commit to jmdyck/ecma262 that referenced this pull request May 9, 2023
… a Job

... by hoisting the job-internal steps out to the caller.

This brings it in line with the statement
(in "9.5 Jobs and Host Operations to Enqueue Jobs")
that job-scheduling host hooks
"accept a Job Abstract Closure as the parameter".

See tc39#3049 (comment) and following.
@jmdyck
Copy link
Collaborator

jmdyck commented May 9, 2023

Good idea, we should do something similar for FinalizationRegistryCleanupJob.

On second thought, I don't think HTML's HostEnqueueFinalizationRegistryCleanupJob would like that change: it appears to need access to _finalizationRegistry_, which it wouldn't have if that was hidden within a Job Abstract Closure.

@syg
Copy link
Contributor Author

syg commented May 10, 2023

I've rebased this on top of #3058.

Following HTML spec review and with #3058, I've renamed HostEnqueueCrossAgentJob(job, agentSignifier, realm) to HostEnqueueGenericJob(job, realm), simplifying things slightly.

spec.html Show resolved Hide resolved
Copy link
Member

@michaelficarra michaelficarra left a comment

Choose a reason for hiding this comment

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

LGTM other than some pretty minor comments.

@syg syg added editor call to be discussed in the next editor call has stage 4 This PR represents a proposal that has achieved stage 4, and is ready to merge. and removed pending stage 4 This proposal has not yet achieved stage 4, but may otherwise be ready to merge. editor call to be discussed in the next editor call labels May 24, 2023
Copy link
Contributor

@bakkot bakkot left a comment

Choose a reason for hiding this comment

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

Some comments. Otherwise LGTM.

spec.html Show resolved Hide resolved
spec.html Show resolved Hide resolved
spec.html Show resolved Hide resolved
spec.html Show resolved Hide resolved
@bakkot bakkot added the ready to merge Editors believe this PR needs no further reviews, and is ready to land. label Jun 13, 2023
@ljharb ljharb dismissed jmdyck’s stale review June 14, 2023 22:53

changes addressed

@ljharb ljharb merged commit 3330e23 into tc39:main Jun 14, 2023
7 checks passed
zhangenming pushed a commit to zhangenming/ecma262 that referenced this pull request Dec 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
has stage 4 This PR represents a proposal that has achieved stage 4, and is ready to merge. normative change Affects behavior required to correctly evaluate some ECMAScript source text proposal This is related to a specific proposal, and will be closed/merged when the proposal reaches stage 4. ready to merge Editors believe this PR needs no further reviews, and is ready to land.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants