Stage 3 reviews - Githubissues

littledan commented 5 years ago

Reviewers:

[x] @littledan
[ ] @kmiller68
[x] @rwaldron

Editors:

[x] @ljharb
[ ] @syg
[ ] @bakkot
[ ] @michaelficarra

littledan commented 5 years ago

I've reviewed this text, and the big issues I see are in #13 and #15, which I'd like to see resolved before Stage 3. I'll do my best to review the fixes, but I'll be on leave starting July 1st through August 30th, and I'll defer to others on reviewing the rest of the fixes.

littledan commented 4 years ago

The issues I reported seem to be fixed, or have fixes along the way, so LGTM for Stage 3 from me.

ljharb commented 4 years ago

https://tc39.es/proposal-atomics-wait-async/#sec-removewaiter seems to have an unmarked “todo” note at the end of it. Confirm?
https://tc39.es/proposal-atomics-wait-async/#sec-notifywaiter step 3 is missing a _ after waiterRecord
https://tc39.es/proposal-atomics-wait-async/#sec-addalarm step 4 d - what does “dead” mean? Is this defined explicitly anywhere?
https://tc39.es/proposal-atomics-wait-async/#sec-atomics.wait is the term “truthy” typically used in the spec, as opposed to ToBoolean(x) is true?
wait and waitAsync seem to have many steps in common. Can that be refactored to share an abstract operation for the common parts somehow?

syg commented 4 years ago

@ljharb Thanks for the catches! All but the last bullet point should be fixed after #18 was merged, PTAL.

As for the refactoring I'll be happy to for the final merge into the main spec. I don't much feel like typing out a bunch of <ins> and <del> tags will help the stage 3 review. :)

ljharb commented 4 years ago

Thanks! Updated review:

https://tc39.es/proposal-atomics-wait-async/#sec-getwaiterlist the phrase "promise capability" (as opposed to "PromiseCapability Record") does not appear in 262. It kind of seems like you're using it instead of something like "[[PromiseCapability]]" - ie, an internal slot that may hold undefined or a PromiseCapability Record. Is there a reason this is referred to in a very prose-like way, instead of something more akin to internal slots? It might be much clearer if a WaiterList is a semantic object with internal slots, instead of one that "contains an ordered List"? (I also note that step 3 in https://tc39.es/proposal-atomics-wait-async/#sec-suspend actually talks about WaiterLists in the way I'm suggesting; can that be made consistent?)
Regarding the refactoring; I'm happy to wait on the Atomics.wait changes until the spec PR, but perhaps Atomics.waitAsync could make use of the abstract operation here first?

syg commented 4 years ago

Yeah, I guess we might as well change WaiterList to be Records holding a List and a Synchronize event. Though currently step 3 in https://tc39.es/proposal-atomics-wait-async/#sec-suspend isn't referring to WaiterLists as a Record with fields; it's referring to Records within the WaiterList's ordered list.
Having Atomics.waitAsync use an AO to be shared later sounds reasonable, I'll do that.

syg commented 4 years ago

I've done the record refactoring and pushed it to master: https://tc39.es/proposal-atomics-wait-async/#sec-getwaiterlist

I have the AO refactoring in a separate branch and will open a PR for you to take a look. I'm personally not a fan of the refactored AO because of the conditionals inside.

Edit: Note that the current master is layered on top of the yet unmerged https://github.com/tc39/ecma262/pull/1692, which I dare hope is uncontroversial and will be merged next week.

syg commented 4 years ago

@kmiller68 Review ping, would like to advance to stage 3 next meeting.

kmiller68 commented 4 years ago

@kmiller68 Review ping, would like to advance to stage 3 next meeting.

Whoops sorry, I wish GitHub had a better way to notify me when someone @s me. Anywho, this is my first review so bear with me.

https://tc39.es/proposal-atomics-wait-async/#sec-addwaiter seems wrong when run from waitAsync. Currently it says:

1. If waiterRecord.[[PromiseCapability]] is undefined, then
    a. Assert: There is no Waiter Record in WL.[[Waiters]] whose [[AgentSignifier]] field is waiterRecord.[[AgentSignifier]].
    b. For each element wr in WL.[[Waiters]], do
        i. If wr.[[AgentSignifier]] is waiterRecord.[[AgentSignifier]], then
            1. Insert waiterRecord to immediately precede wr in WL.[[Waiters]].
            2. Set inserted to true.

Is the intention that you can't have more than one async waiter for an address? I'm guessing there should be an else between a. and b.?

Additionally, why does waiterRecord go before wr? Isn't that a stack system? That's definitely wrong since you could in effect see any ordering of resolution for a given thread. Also, and probably more importantly, why do we insert right after the last record for the same agent? IMO, the waiter list should just be a global FIFO queue for a given "address".

It also looks like https://tc39.es/proposal-atomics-wait-async/#sec-addwaiter lost it's indenting for the then block. Idk, if that's important.

syg commented 4 years ago

Is the intention that you can't have more than one async waiter for an address? I'm guessing there should be an else between a. and b.?

The opposite, that you can't have more than one sync waiter for an address. That whole if block is "if the promise capability is undefined", i.e. from Atomics.wait. The async case is the "if inserted is false" block below this one.

Additionally, why does waiterRecord go before wr?

That's the "sync waits skip the line" semantics we've discussed. Rationale being if a thread is blocked, resolving its asyncWait promises are pretty useless without first unblocking the thread.

Also, and probably more importantly, why do we insert right after the last record for the same agent?

Where is this done? A synchronous waiterRecord for an agent always ends up before any other record for that agent in the waiter queue, while an asynchronous waiterRecord goes after every other record for that agent.

IMO, the waiter list should just be a global FIFO queue for a given "address".

Yeah, for async waiters, it is a per-address FIFO queue. But it also keeps track of the synchronous waits with the line-skipping logic, which is why the logic gets more complicated.

kmiller68 commented 4 years ago

Is the intention that you can't have more than one async waiter for an address? I'm guessing there should be an else between a. and b.?

The opposite, that you can't have more than one sync waiter for an address. That whole if block is "if the promise capability is undefined", i.e. from Atomics.wait. The async case is the "if inserted is false" block below this one.

AHH, I think the assert is wrong then (and threw me off >.>). I think the assert should be: Assert: There is no Waiter Record in WL.[[Waiters]] whose **[[PromiseCapability]] is not undefined and** [[AgentSignifier]] field is waiterRecord.[[AgentSignifier]].

The rest makes sense now. Ignore my other comments, except the indentation one.

syg commented 4 years ago

Oh doh, that assert most def is wrong, thanks for the catch.

Btw which then block lost its indentation? All the then blocks (step 4.a-b, 4.b.i.1-2, 5.a, 6.a-b) look indented to me.

kmiller68 commented 4 years ago

Btw which then block lost its indentation? All the then blocks (step 4.a-b, 4.b.i.1-2, 5.a, 6.a-b) look indented to me.

Ugh, sorry I pasted the wrong link before. I meant to link to: https://tc39.es/proposal-atomics-wait-async/#sec-entercriticalsection

syg commented 4 years ago

Ah, will fix.

syg commented 4 years ago

The two issues @kmiller68 raised have been addressed in defee82967f56f5c92eb05d6b475190258eb80ca and 87210b5c8ac686727063e3582417899df000f094

kmiller68 commented 4 years ago

Cool, LGTM now.

syg commented 4 years ago

Thank you for the reviews.

ljharb commented 4 years ago

The phrase "promise capability" still appears in NotifyWaiter; the first occurrence could probably also be Note: an undefined [[PromiseCapability]] denotes a blocking wait, but the second is probably best as is (and it'd be fine, since the first is in a note, to keep it as-is too).
calls into abstract ops that don't introspect the completion record should have ? or !; eg in TriggerTimeout, AddWaiter, Suspend, etc.
in DoWait, instead of making them strings, should they be enum values? ie ~sync~ instead of *"sync"* etc

Other than these items, consider me signed off.

syg commented 4 years ago

@ljharb Thanks for the review

I prefer to leave "promise capability" as is in both cases.
We discussed at this at the last editor meeting and you know my opinion on this. I think it would be very strange to prefix ? or ! for AOs like Suspend and TriggerTimeout. Until we make the entire spec consistent wrt ? and !, since both uses unfortunately are in concurrent use, I'm going to stick with not prefixing them here.
I've changed spec-internal string constants to enum values.

ljharb commented 4 years ago

Sounds good.

syg commented 4 years ago

I've decided to change the semantics of Atomics.wait to not cut the line and to keep a single per-location FIFO queue for both sync and async waiters. In light of that, I'd like the reviewers to review #20 before the next meeting if possible. It's a small, but major change.

rwaldron commented 4 years ago

During the course of writing tests for Test262, I've completed a first pass review and filed issues from my findings (all of which have been fixed—thanks @syg!). I will continue to file issues and propose fixes as I encounter them.

@littledan I feel comfortable giving a ✔️ for my Stage 3 review.

tc39 / proposal-atomics-wait-async

Stage 3 reviews #16