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

RESOLVED FIXED in Firefox 63

Status

()

defect
P3
normal
RESOLVED FIXED
Last year
10 months ago

People

(Reporter: rwaldron, Assigned: lth)

Tracking

(Blocks 2 bugs, {dev-doc-complete})

unspecified
mozilla63
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox63 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Reporter

Description

Last year
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
Reporter

Updated

Last year
Flags: needinfo?(jwalden+bmo)
Reporter

Updated

Last year
Summary: Rename (or alias) Atomics.wake => Atomics.notify → Rename Atomics.wake to Atomics.notify per ES standard change
Assignee

Comment 1

11 months ago
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...
Assignee

Comment 2

11 months ago
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

Updated

11 months ago
Assignee: nobody → lhansen
Status: NEW → ASSIGNED
Assignee

Comment 3

11 months ago
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
Assignee

Comment 4

11 months ago
Before we re-enable SAB we should be sure to correct the naming.
Blocks: 1477743
Assignee

Updated

10 months ago
Blocks: 1349863
Assignee

Updated

10 months ago
Assignee: nobody → lhansen
Status: NEW → ASSIGNED
Assignee

Comment 5

10 months ago
Attachment #9002194 - Flags: review?(andrebargull)
Assignee

Updated

10 months ago
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+
Assignee

Updated

10 months ago
Depends on: 1484728
Assignee

Comment 7

10 months ago
(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.)
Assignee

Comment 8

10 months ago
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+
Assignee

Updated

10 months ago
Flags: needinfo?(jwalden+bmo)

Comment 9

10 months ago
Pushed by lhansen@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d9d4bf8a92b3
Introduce Atomics.notify, make Atomics.wake an alias.  r=anba

Comment 10

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d9d4bf8a92b3
Status: ASSIGNED → RESOLVED
Closed: 10 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in before you can comment on or make changes to this bug.