Closed
Bug 1260910
Opened 9 years ago
Closed 9 years ago
Rename Atomics.{futexWait,futexWake} as Atomics.{wait,wake}
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
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)
8.86 KB,
patch
|
bbouvier
:
review+
|
Details | Diff | Splinter Review |
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)
Updated•9 years ago
|
Keywords: dev-doc-needed
Assignee | ||
Comment 1•9 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•9 years ago
|
||
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•9 years ago
|
Assignee: nobody → lhansen
Status: NEW → ASSIGNED
Comment 3•9 years ago
|
||
(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•9 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•9 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•9 years ago
|
||
Now with adapted test cases
Attachment #8736968 -
Flags: review?(bbouvier)
Assignee | ||
Updated•9 years ago
|
Attachment #8736923 -
Attachment is obsolete: true
Comment 7•9 years ago
|
||
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•9 years ago
|
||
Clearing needinfos as this was discussed, thanks for the heads up.
Flags: needinfo?(jujjyl)
Flags: needinfo?(azakai)
Assignee | ||
Comment 9•9 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•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/63bdfecc99f488142d1601f381f6241fd22ddb92
Bug 1260910 - introduce 'wait' and 'wake'. r=bbouvier
Comment 11•9 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Comment 12•9 years ago
|
||
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•9 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.
Description
•