Strengthen Atomics critical section to imply synchronizes-with #1692

syg · 2019-09-07T00:39:15Z

@conrad-watt @lars-t-hansen

Fixes #1680.

ljharb · 2019-09-07T07:33:22Z

@syg do you think this is something that can have test262 test coverage?

syg · 2019-09-07T18:28:48Z

@ljharb Unfortunately not. :(

conrad-watt · 2019-09-08T11:28:33Z

spec.html

+ 1. Let _leaverEventList_ be the [[EventList]] field of the element in _execution_.[[EventRecords]] whose [[AgentSignifier]] is _L_.
+ 1. Let _enterEvent_ and _leaveEvent_ be new Synchronize events.
+ 1. Append _enterEvent_ to _entererEventList_.
+ 1. Append _leaveEvent_ to _leaverEventList_.


Morally, this will create a synchronization in the wrong place.

The leaver's (unpaired) synchronization event should be inserted into the event list at the moment of leaving the critical section, with the subsequent entry into the critical section creating the paired synchronization event. Otherwise, you allow executions where the leaver's synchronization event shows up in the agent-order after other events which it is programmatically before (i.e. the notifying thread could have continued execution before the notified waiter gets a chance to re-enter the critical section). This is potentially un-observable, but it's un-intuitive and might cause inconsistencies later on if other features which expose operational interleavings are added.

I'm also a little iffy about the prose identification of the "immediately preceding" agent. This seems like the sort of thing that should be explicitly tracked as state in a semantic object.

The minimum patch would be to make the WaiterList keep a record of the last exiting synchronization event, updated with every LeaveCriticalSection, and synchronized on with every EnterCriticalSection.

Thank you as always, @conrad-watt. These are all good suggestions, and I will try to incorporate them.

conrad-watt · 2019-09-08T11:43:48Z

spec.html

@@ -36144,15 +36154,6 @@ <h1>NotifyWaiter ( _WL_, _W_ )</h1>
 <emu-alg>
 1. Assert: The calling agent is in the critical section for _WL_.
 1. Assert: _W_ is on the list of waiters in _WL_.
- 1. Let _execution_ be the [[CandidateExecution]] field of the surrounding agent's Agent Record.


Deleting these lines has an interesting interaction with the current Atomics.waitAsync proposal. Not something that should interfere overly with this PR though.

Currently, Atomics.wait will re-enter the critical section after being notified. This provides synchronization from the critical section exit of the previous Atomics.notify, and makes these lines redundant. However, if I'm reading the proposed spec correctly, Atomics.waitAsync does not enter the critical section after being notified, so deleting this section loses a synchronization edge that should probably be present, and will need to be added somewhere else in the proposal.

Yes, I realized this when writing this. Since it'll most likely fall on me to fix that spec text, I figured it's easiest to do the minimum delta for the current version of the spec so as to not raise "why is this here?" kind of questions, and fix it up in the waitAsync proposal.

lars-t-hansen · 2019-09-09T11:55:10Z

Awaiting more code before reviewing, but really inclined to simply accept the judgement of @conrad-watt on this patch.

syg · 2019-09-09T20:02:51Z

@conrad-watt Is this more or less what you had in mind?

conrad-watt

LGTM!

lars-t-hansen

Rubber stamp, for all practical purposes.

ljharb · 2019-09-11T17:21:00Z

spec.html

@@ -36057,9 +36057,10 @@ <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_.</p>
+ <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>


I see the term "critical section" already in use, but there seems to be no definition of it in the spec. That doesn't have to block this PR, but it'd probably be good to define it in a separate PR, if not this one :-)

I'll do a separate editorial PR.

ljharb · 2019-09-11T17:28:47Z

spec.html

+ 1. Let _leaverEventList_ be _eventsRecord_.[[EventList]].
+ 1. Let _leaveEvent_ be a new Synchronize event.
+ 1. Append _leaveEvent_ to _leaverEventList_.
+ 1. Set the Synchronize event in _WL_ to _leaveEvent_.


Should this be an internal slot, instead of a vague concept of being "in" a WaiterList?

I don't recall exactly the history of why WaiterList is a "semantic object" instead of being a Record with fields. Perhaps it was to distinguish it as a special spec object that can be touched by different agents and needs to be locked by the critical section to be used? That way we can keep thinking of Records as objects that are intra-agent and don't need any extra care for use.

This also affects how I write the Atomics.waitAsync spec, since that extends the WaiterList to include more information that are accessed from multiple agents.

My preference is to keep it as a semantic object for this PR. I'm open to having some specialization of Record to distinguish WaiterList from other Records -- any thoughts there?

I'm more confused by the whole concept of "semantic object" - the spec doesn't have anything else like that afaik.

It's fine to leave as-is, for the reasons you stated, but it seems like it'd be clearer to me in the future to have it be a proper Record; or to define some kind of more explicit syntax for interacting with "semantic objects" besides prose.

ljharb

LGTM.

Would be great to pursue additional PRs to define "critical section", and to more explicitly specify "semantic objects" and things being "in" them.

syg · 2019-10-02T14:45:00Z

Rebased.

…c39#1680)

PR tc39#1692 removed the element-id 'sec-synchronizeeventset'. Reinstate it as an 'oldid'.

(From PR tc39#1692)

PR tc39#1692 removed the element-id 'sec-synchronizeeventset'. Reinstate it as an 'oldid'.

- Add a subscript R: Since _intValue_ is to be treated as a mathematical value, we should be comparing it to a mathematical zero. - Reinstate 'sec-static-semantics-elisionwidth' as an oldid; tc39#1124 removed the element-id 'sec-static-semantics-elisionwidth' - Reinstate 'sec-synchronizeeventset' as an oldid; tc39#1692 removed the element-id 'sec-synchronizeeventset'. - Reference 'ExportFromClause' from Annex A; tc39#1174 introduced the nonterminal 'ExportFromClause', but didn't reference it from Annex A. - Put asterisks around 'true' (from tc39#1716) - Fix typo "_eventRecords_" -> "_eventsRecord_" (from tc39#1692)

syg added the needs consensus This needs committee consensus before it can be eligible to be merged. label Sep 7, 2019

ljharb added the normative change Affects behavior required to correctly evaluate some ECMAScript source text label Sep 7, 2019

ljharb requested a review from lars-t-hansen September 7, 2019 20:37

conrad-watt reviewed Sep 8, 2019

View reviewed changes

syg force-pushed the strengthen-wait-notify branch from 8ff6cb7 to 92a1604 Compare September 9, 2019 20:01

conrad-watt approved these changes Sep 10, 2019

View reviewed changes

lars-t-hansen approved these changes Sep 11, 2019

View reviewed changes

ljharb requested a review from a team September 11, 2019 17:18

ljharb reviewed Sep 11, 2019

View reviewed changes

ljharb approved these changes Sep 11, 2019

View reviewed changes

ljharb self-assigned this Sep 11, 2019

syg mentioned this pull request Sep 28, 2019

Stage 3 reviews tc39/proposal-atomics-wait-async#16

Open

7 tasks

ljharb added has consensus This has committee consensus. spec bug and removed needs consensus This needs committee consensus before it can be eligible to be merged. labels Oct 1, 2019

syg force-pushed the strengthen-wait-notify branch from 92a1604 to 9d4e9d0 Compare October 2, 2019 14:44

syg added 2 commits October 2, 2019 11:01

Normative: Make the Atomics critical section imply synchronizes-with (t…

a8a22db

…c39#1680)

Editorial: Remove unused AO SynchronizeEventSet (tc39#1692)

d764223

ljharb force-pushed the strengthen-wait-notify branch from 9d4e9d0 to d764223 Compare October 2, 2019 15:01

ljharb merged commit d764223 into tc39:master Oct 2, 2019

jmdyck mentioned this pull request Oct 5, 2019

Editorial: "the calling surrounding" #1721

Closed

jmdyck added a commit to jmdyck/ecma262 that referenced this pull request Oct 5, 2019

Editorial: Reinstate 'sec-synchronizeeventset'

04d3d84

PR tc39#1692 removed the element-id 'sec-synchronizeeventset'. Reinstate it as an 'oldid'.

jmdyck added a commit to jmdyck/ecma262 that referenced this pull request Oct 5, 2019

Editorial: Fix typo "_eventRecords_" -> "_eventsRecord_"

94c8255

(From PR tc39#1692)

jmdyck added a commit to jmdyck/ecma262 that referenced this pull request Oct 5, 2019

Editorial: Reinstate 'sec-synchronizeeventset'

b9ee595

PR tc39#1692 removed the element-id 'sec-synchronizeeventset'. Reinstate it as an 'oldid'.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Strengthen Atomics critical section to imply synchronizes-with #1692

Strengthen Atomics critical section to imply synchronizes-with #1692

syg commented Sep 7, 2019 •

edited by ljharb

Loading

ljharb commented Sep 7, 2019

syg commented Sep 7, 2019

conrad-watt Sep 8, 2019 •

edited

Loading

syg Sep 9, 2019

conrad-watt Sep 8, 2019

syg Sep 9, 2019

lars-t-hansen commented Sep 9, 2019

syg commented Sep 9, 2019

conrad-watt left a comment

lars-t-hansen left a comment

ljharb Sep 11, 2019

syg Sep 11, 2019

ljharb Sep 11, 2019

syg Sep 11, 2019

ljharb Sep 11, 2019

ljharb left a comment

syg commented Oct 2, 2019

Strengthen Atomics critical section to imply synchronizes-with #1692

Strengthen Atomics critical section to imply synchronizes-with #1692

Conversation

syg commented Sep 7, 2019 • edited by ljharb Loading

ljharb commented Sep 7, 2019

syg commented Sep 7, 2019

conrad-watt Sep 8, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lars-t-hansen commented Sep 9, 2019

syg commented Sep 9, 2019

conrad-watt left a comment

Choose a reason for hiding this comment

lars-t-hansen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ljharb left a comment

Choose a reason for hiding this comment

syg commented Oct 2, 2019

syg commented Sep 7, 2019 •

edited by ljharb

Loading

conrad-watt Sep 8, 2019 •

edited

Loading