Closed Bug 1260910 Opened 8 years ago Closed 8 years ago

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

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: lth, Assigned: lth)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete)

Attachments

(1 file, 1 obsolete file)

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)
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.)
Attached patch introduce 'wait' and 'wake' (obsolete) — Splinter Review
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: 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
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.
Comment on attachment 8736923 [details] [diff] [review]
introduce 'wait' and 'wake'

Canceling r?, will also attach changed test cases.
Attachment #8736923 - Flags: review?(bbouvier)
Now with adapted test cases
Attachment #8736968 - Flags: review?(bbouvier)
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+
Clearing needinfos as this was discussed, thanks for the heads up.
Flags: needinfo?(jujjyl)
Flags: needinfo?(azakai)
(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.
https://hg.mozilla.org/mozilla-central/rev/63bdfecc99f4
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
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?).
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.