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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P3)

Description

[Rick Waldron [:rwaldron]]

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

Comment 1

[Lars T Hansen [:lth]]

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

Comment 2

[Lars T Hansen [:lth]]

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.

Comment 3

[Lars T Hansen [:lth]]

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

Comment 4

[Lars T Hansen [:lth]]

Before we re-enable SAB we should be sure to correct the naming.

Comment 5

[Lars T Hansen [:lth]]

Attachment: bug1470490-notify-is-the-new-wake.patch

Comment 6

[André Bargull [:anba]]

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

Comment 7

[Lars T Hansen [:lth]]

(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.)

Comment 8

[Lars T Hansen [:lth]]

Attachment: bug1470490-notify-is-the-new-wake-v2.patch

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.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d9d4bf8a92b3
Introduce Atomics.notify, make Atomics.wake an alias.  r=anba

Comment 10

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/d9d4bf8a92b3

Comment 11

[Florian Scholz (Open Web Docs)]

Updated
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Atomics
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Atomics/notify