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

Editorial: Describe WaiterList using Records #2240

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

ExE-Boss
Copy link
Contributor

@ExE-Boss ExE-Boss commented Dec 2, 2020

This makes WaiterLists use existing constructs, rather than the not‑used‑elsewhere “semantic object”.

This also makes the Atomics.waitAsync(…) proposal diff more cleanly.

See also:

@ExE-Boss ExE-Boss force-pushed the editorial/describle-waiterlist-using-records branch from 50c8e7f to d49c434 Compare December 2, 2020 19:58
@ljharb ljharb requested a review from syg December 3, 2020 05:56
Copy link
Contributor

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

I'm on the fence for this PR, given the comments below, but it could be made to work.

spec.html Show resolved Hide resolved
spec.html Show resolved Hide resolved
spec.html Show resolved Hide resolved
spec.html Show resolved Hide resolved
@@ -37244,76 +37244,96 @@ <h1>ValidateAtomicAccess ( _typedArray_, _requestIndex_ )</h1>

<emu-clause id="sec-getwaiterlist" aoid="GetWaiterList">
<h1>GetWaiterList ( _block_, _i_ )</h1>
<p>A <dfn>WaiterList</dfn> is a semantic object that contains an ordered list of those agents that are waiting on a location (_block_, _i_) in shared memory; _block_ is a Shared Data Block and _i_ a byte offset into the memory of _block_. A WaiterList object also optionally contains a Synchronize event denoting the previous leaving of its critical section.</p>
<p>Initially a WaiterList object has an empty list and no Synchronize event.</p>
Copy link
Contributor

Choose a reason for hiding this comment

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

This paragraph is important and need to be captured, see comment below.

<p>The abstract operation GetWaiterList takes arguments _block_ (a Shared Data Block) and _i_ (a non-negative integer). It performs the following steps when called:</p>
<emu-alg>
1. Assert: _block_ is a Shared Data Block.
1. Assert: _i_ and _i_ + 3 are valid byte offsets within the memory of _block_.
1. Assert: _i_ is divisible by 4.
1. Return the WaiterList that is referenced by the pair (_block_, _i_).
1. Return the WaiterList Record that is referenced by the pair (_block_, _i_).
Copy link
Contributor

Choose a reason for hiding this comment

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

This part is a tricky. The "nice" thing about WaiterList being a handwavy magical "semantic object" was that we sidestepped making synchronization around this agent cluster-wide WaiterList store explicit. We can kind of pretend that they magically appear, or that they always exist for any possible (block, i).

Now, since for Records, the creation of new Records is an explicit thing, we now need to also explicitly capture the creation of these WaiterList records in the agent-wide store. Importantly, we need to synchronize the creation. If two agents are concurrently trying to get a WL corresponding to (b, i), we need to ensure both agents get the same WL.

To capture this, we need something like the following:

  • The agent cluster-wide WL store itself needs a critical section that GetWaiterList enters.
  • If the store doesn't have a record for (b, i), one needs to be created and explicitly set [[Waiters]] to the empty List and [[MostRecentLeaveEvent]] to empty.

That's more machinery for not a lot of extra clarity of understanding, so I'm on the fence for this PR.

@ExE-Boss ExE-Boss force-pushed the editorial/describle-waiterlist-using-records branch from 13c7cf8 to 963c098 Compare December 10, 2020 00:07
@ExE-Boss ExE-Boss marked this pull request as draft February 19, 2021 13:49
@ljharb ljharb force-pushed the master branch 3 times, most recently from 3d0c24c to 7a79833 Compare June 29, 2021 02:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants