Closed Bug 1470490 Opened 2 years ago Closed 2 years ago

Rename Atomics.wake to Atomics.notify per ES standard change

Categories

(Core :: JavaScript Engine, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: rwaldron, Assigned: lth)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete)

Attachments

(1 file, 1 obsolete file)

At the May 2018 TC39 meeting, consensus was reached to rename Atomics.wake to Atomics.notify (while the API is temporarily disabled). 

The spec change can be found here: https://github.com/tc39/ecma262/pull/1220
Flags: needinfo?(jwalden+bmo)
Summary: Rename (or alias) Atomics.wake => Atomics.notify → Rename Atomics.wake to Atomics.notify per ES standard change
I was going to flag this as good-first-bug, but as it also affects asm.js I'm not actually sure if that's true...
Let's nuke asm.js's atomics first, then we don't have to worry so much.

Also it may behoove us to keep an alias for Atomics.wake for a while?  Not sure.
Depends on: 1423577
Assignee: nobody → lhansen
Status: NEW → ASSIGNED
asm.js atomics are gone; only straight JS needs to be updated.  (Wasm atomics are unaffected by JS naming changes, plus Wasm is a binary format.)
Assignee: lhansen → nobody
Status: ASSIGNED → NEW
Before we re-enable SAB we should be sure to correct the naming.
Blocks: resab
Blocks: 1349863
Assignee: nobody → lhansen
Status: NEW → ASSIGNED
Attachment #9002194 - Flags: review?(andrebargull)
Priority: -- → P3
Comment on attachment 9002194 [details] [diff] [review]
bug1470490-notify-is-the-new-wake.patch

Review of attachment 9002194 [details] [diff] [review]:
-----------------------------------------------------------------

> Do we never run shell tests marked "slow"?

No, I don't think so.

r+ with the Atomics changes from <https://hg.mozilla.org/mozilla-central/rev/317099325f4c> reverted. (You may want to wait for bug 1484728, because it fixes a typo in a test262 Atomics test.)

::: js/src/builtin/AtomicsObject.cpp
@@ +982,5 @@
>      JS_INLINABLE_FN("xor",                atomics_xor,                3,0, AtomicsXor),
>      JS_INLINABLE_FN("isLockFree",         atomics_isLockFree,         1,0, AtomicsIsLockFree),
>      JS_FN("wait",                         atomics_wait,               4,0),
> +    JS_FN("notify",                       atomics_notify,             3,0),
> +    JS_FN("wake",                         atomics_notify,             3,0), // Legacy name

Creating two separate functions is easier, but I wonder if it makes sense to have only one function installed as two separate properties, similar to how |String.prototype.trimLeft| is the same function as |String.prototype.trimStart| [1] or how |Date.prototype.toGMTString| is the same function as |Date.prototype.toUTCString| [2].


[1] https://searchfox.org/mozilla-central/rev/71ef4447db179639be9eff4471f32a95423962d7/js/src/builtin/String.cpp#3649-3657
[2] https://searchfox.org/mozilla-central/rev/71ef4447db179639be9eff4471f32a95423962d7/js/src/jsdate.cpp#3389-3398

::: js/src/builtin/AtomicsObject.h
@@ +78,4 @@
>      // times allowed; specify mozilla::Nothing() for an indefinite
>      // wait.
>      //
> +    // notify() will not wake up spuriously.  It will return true and

Shouldn't this still be "wait()"?

Pre-existing: |return true| no longer correct since bug 1377576.

@@ +82,1 @@
>      // set *result to a return code appropriate for

Pre-existing: |*result| no longer exists.

@@ +82,2 @@
>      // set *result to a return code appropriate for
>      // Atomics.wait() on success, and return false on error.

Ditto: |return false|.
Attachment #9002194 - Flags: review?(andrebargull) → review+
Depends on: 1484728
(In reply to André Bargull [:anba] from comment #6)
> Comment on attachment 9002194 [details] [diff] [review]
> bug1470490-notify-is-the-new-wake.patch
> 
> Review of attachment 9002194 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/builtin/AtomicsObject.cpp
> @@ +982,5 @@
> >      JS_INLINABLE_FN("xor",                atomics_xor,                3,0, AtomicsXor),
> >      JS_INLINABLE_FN("isLockFree",         atomics_isLockFree,         1,0, AtomicsIsLockFree),
> >      JS_FN("wait",                         atomics_wait,               4,0),
> > +    JS_FN("notify",                       atomics_notify,             3,0),
> > +    JS_FN("wake",                         atomics_notify,             3,0), // Legacy name
> 
> Creating two separate functions is easier, but I wonder if it makes sense to
> have only one function installed as two separate properties, similar to how
> |String.prototype.trimLeft| is the same function as
> |String.prototype.trimStart| [1] or how |Date.prototype.toGMTString| is the
> same function as |Date.prototype.toUTCString| [2].

In this case I'll leave it the way it is since there will be a followup bug to get rid of the wake() alias once we're satisfied that we're not breaking anything in our test suite.

(Nice observation re pre-existing bogus prose.)
Includes the fix for jstests.list and fixes the comment nits.  Leaves wake and notify as two distinct functions.  Ready to land but we need to wait for the blockers.
Attachment #9002194 - Attachment is obsolete: true
Attachment #9002760 - Flags: review+
Flags: needinfo?(jwalden+bmo)
Pushed by lhansen@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d9d4bf8a92b3
Introduce Atomics.notify, make Atomics.wake an alias.  r=anba
https://hg.mozilla.org/mozilla-central/rev/d9d4bf8a92b3
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in before you can comment on or make changes to this bug.