Rename Atomics.{futexWait,futexWake} as Atomics.{wait,wake}

RESOLVED FIXED in Firefox 48

Status

()

Core
JavaScript Engine
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: lth, Assigned: lth)

Tracking

(Blocks: 1 bug, {dev-doc-complete})

unspecified
mozilla48
dev-doc-complete
Points:
---

Firefox Tracking Flags

(firefox48 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

2 years ago
Spec change: https://github.com/tc39/ecmascript_sharedmem/issues/95.  No functional change, the API remains the same (see bug 1260835 for recent changes).

Jukka/Alon, this affects you.  If you like, I can keep the futexWait/futexWake names around for a while, but as there is no functional change it's easy to polyfill (in either direction).
Flags: needinfo?(jujjyl)
Flags: needinfo?(azakai)
Keywords: dev-doc-needed
(Assignee)

Comment 1

2 years ago
Chatted with Jukka about this.  I may opt to introduce wait and wake ASAP (before FF48 departs) but to only remove futexWait and futexWake in the next release cycle, since my need to change the names overlaps in a poor way with Jukka's need for some PTO and he has to adapt emscripten to the new names.

(I could even opt to let futexWait return an integer code for a cycle.)
(Assignee)

Comment 2

2 years ago
Created attachment 8736923 [details] [diff] [review]
introduce 'wait' and 'wake'

This introduces 'wake' and 'wait' as aliases for 'futexWake' and 'futexWait'; the old names are obsolete.  Being a nice guy I am leaving the old names in for one release cycle, ie they will be removed after we branch on 4/18.
Attachment #8736923 - Flags: review?(bbouvier)
(Assignee)

Updated

2 years ago
Assignee: nobody → lhansen
Status: NEW → ASSIGNED
(In reply to Lars T Hansen [:lth] from comment #2)
> This introduces 'wake' and 'wait' as aliases for 'futexWake' and
> 'futexWait'; the old names are obsolete.  Being a nice guy I am leaving the
> old names in for one release cycle, ie they will be removed after we branch
> on 4/18.

I will update the docs to discourage the use of the old names once this lands. No idea how widespread this API is already, but want to let you know that there is also the possibility to add a warning to the console like this one:

JSMSG_DEPRECATED_STRING_CONTAINS Warning: String.prototype.contains() is deprecated and will be removed in a future release; use String.prototype.includes() instead
(Assignee)

Comment 4

2 years ago
Florian, thanks for the tip.  These APIs are bleeding edge and mostly used by our libraries, and the only reason for me letting the old names linger for a cycle is because we're shipping emscripten libraries that can't be updated immediately for logistical reasons.
(Assignee)

Comment 5

2 years ago
Comment on attachment 8736923 [details] [diff] [review]
introduce 'wait' and 'wake'

Canceling r?, will also attach changed test cases.
Attachment #8736923 - Flags: review?(bbouvier)
(Assignee)

Comment 6

2 years ago
Created attachment 8736968 [details] [diff] [review]
introduce 'wait' and 'wake'

Now with adapted test cases
Attachment #8736968 - Flags: review?(bbouvier)
(Assignee)

Updated

2 years ago
Attachment #8736923 - Attachment is obsolete: true
Comment on attachment 8736968 [details] [diff] [review]
introduce 'wait' and 'wake'

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

Looks good.

::: js/src/builtin/AtomicsObject.cpp
@@ +1236,5 @@
>      JS_INLINABLE_FN("and",                atomics_and,                3,0, AtomicsAnd),
>      JS_INLINABLE_FN("or",                 atomics_or,                 3,0, AtomicsOr),
>      JS_INLINABLE_FN("xor",                atomics_xor,                3,0, AtomicsXor),
>      JS_INLINABLE_FN("isLockFree",         atomics_isLockFree,         1,0, AtomicsIsLockFree),
> +    JS_FN("wait",                         atomics_futexWait,          4,0),

More a question than a remark: do you prefer to keep the atomics_futexWait function named as is, or to rename it atomics_wait, now or later? I like that's it's descriptive, but it breaks the 1:1 mapping between the JS names and C++ names. Same question for futexWake.

There are also comments referencing futexWait/futexWake in vm/Runtime.{h,cpp} and builtin/Atomic.{h,cpp} that you could want to update, now or later.
Attachment #8736968 - Flags: review?(bbouvier) → review+

Comment 8

2 years ago
Clearing needinfos as this was discussed, thanks for the heads up.
Flags: needinfo?(jujjyl)
Flags: needinfo?(azakai)
(Assignee)

Comment 9

2 years ago
(In reply to Benjamin Bouvier [:bbouvier] from comment #7)
> Comment on attachment 8736968 [details] [diff] [review]
> introduce 'wait' and 'wake'
> 
> Review of attachment 8736968 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> More a question than a remark: do you prefer to keep the atomics_futexWait
> function named as is, or to rename it atomics_wait, now or later? I like
> that's it's descriptive, but it breaks the 1:1 mapping between the JS names
> and C++ names. Same question for futexWake.

It's a bug and I will fix it.
(Assignee)

Comment 10

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/63bdfecc99f488142d1601f381f6241fd22ddb92
Bug 1260910 - introduce 'wait' and 'wake'. r=bbouvier

Comment 11

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/63bdfecc99f4
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Updated and moved 
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/wait
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Atomics/wake
(redirects in place for the old names)

Not going to add any compatibility note as this is an experimental API anyway and we are shipping the old API names for 2 releases only. (Will this be uplifted to 47 to only have 1 release with the old names?).
Keywords: dev-doc-needed → dev-doc-complete
(Assignee)

Comment 13

2 years ago
I could uplift but I'd also have to uplift bug 1260835 and that's a bigger change, I'm not sure I want to worry about it.  Also I removed Atomics.fence and Atomics.futexWaitOrRequeue at the same time and could incorporate those patches...

I think I'll just let this ride the train with 48.
You need to log in before you can comment on or make changes to this bug.