Atomics Futex API

Atomics Futex API

BUG=chromium:497295
R=jarin@chromium.org
LOG=n

Committed: https://crrev.com/35b2114874bc37fb5fc839244f9794567d5d31d4
Cr-Commit-Position: refs/heads/master@{#29736}

[binji, 2015-07-08 17:37:42]

PTAL, the bots are failing because this CL depends on
https://codereview.chromium.org/1223813008.

[Michael Starzinger, 2015-07-08 17:54:19]

mstarzinger@chromium.org changed reviewers:
+ mstarzinger@chromium.org

[Michael Starzinger, 2015-07-08 17:54:20]

https://codereview.chromium.org/1208933006/diff/140001/src/flag-definitions.h
File src/flag-definitions.h (right):

https://codereview.chromium.org/1208933006/diff/140001/src/flag-definitions.h...
src/flag-definitions.h:197: V(harmony_futex, "harmony futex")                   
         \
This flag seems to be only used in RUNTIME_ASSERTs, otherwise the implementation
seems to hinge off of harmony_atomics instead. Is this intended? Is this flag
needed?

https://codereview.chromium.org/1208933006/diff/140001/src/harmony-atomics.js
File src/harmony-atomics.js (right):

https://codereview.chromium.org/1208933006/diff/140001/src/harmony-atomics.js...
src/harmony-atomics.js:149: return %_AtomicsFutexWait(ia, index, value,
timeout);
The %_FooBar syntax is used to indicate that at least one compiler inlines this
as an intrinsic. Since there are only runtime function backing this so far, I
would for using plain %FooBar instead, to not confuse people about the expected
overhead. Likewise below.

https://codereview.chromium.org/1208933006/diff/140001/src/runtime/runtime.h
File src/runtime/runtime.h (right):

https://codereview.chromium.org/1208933006/diff/140001/src/runtime/runtime.h#...
src/runtime/runtime.h:67: F(AtomicsFutexWait, 4, 1)           \
Please add a new FOR_EACH_INTRINSIC_FUTEX macro, each .cc file has it's own
macro.

[binji, 2015-07-08 18:05:29]

https://codereview.chromium.org/1208933006/diff/140001/src/flag-definitions.h
File src/flag-definitions.h (right):

https://codereview.chromium.org/1208933006/diff/140001/src/flag-definitions.h...
src/flag-definitions.h:197: V(harmony_futex, "harmony futex")                   
         \
On 2015/07/08 at 17:54:20, Michael Starzinger wrote:
> This flag seems to be only used in RUNTIME_ASSERTs, otherwise the
implementation seems to hinge off of harmony_atomics instead. Is this intended?
Is this flag needed?

Removed.

https://codereview.chromium.org/1208933006/diff/140001/src/harmony-atomics.js
File src/harmony-atomics.js (right):

https://codereview.chromium.org/1208933006/diff/140001/src/harmony-atomics.js...
src/harmony-atomics.js:149: return %_AtomicsFutexWait(ia, index, value,
timeout);
On 2015/07/08 at 17:54:20, Michael Starzinger wrote:
> The %_FooBar syntax is used to indicate that at least one compiler inlines
this as an intrinsic. Since there are only runtime function backing this so far,
I would for using plain %FooBar instead, to not confuse people about the
expected overhead. Likewise below.

Done.

https://codereview.chromium.org/1208933006/diff/140001/src/runtime/runtime.h
File src/runtime/runtime.h (right):

https://codereview.chromium.org/1208933006/diff/140001/src/runtime/runtime.h#...
src/runtime/runtime.h:67: F(AtomicsFutexWait, 4, 1)           \
On 2015/07/08 at 17:54:20, Michael Starzinger wrote:
> Please add a new FOR_EACH_INTRINSIC_FUTEX macro, each .cc file has it's own
macro.

Done.

[Michael Starzinger, 2015-07-08 18:08:32]

mstarzinger@chromium.org changed reviewers:
- mstarzinger@chromium.org

[Michael Starzinger, 2015-07-08 18:08:33]

Just a drive-by, I'll leave the rest to Jaro.

[Jarin, 2015-07-10 12:11:40]

First batch of comments.

https://codereview.chromium.org/1208933006/diff/160001/src/futex-emulation.cc
File src/futex-emulation.cc (right):

https://codereview.chromium.org/1208933006/diff/160001/src/futex-emulation.cc...
src/futex-emulation.cc:18: FutexWaitList FutexEmulation::wait_list_;
Static initializers are not allowed by Chrome. You should use the LazyInstance
template.

https://codereview.chromium.org/1208933006/diff/160001/src/futex-emulation.cc...
src/futex-emulation.cc:39: if (node->prev_)
braces, please.

https://codereview.chromium.org/1208933006/diff/160001/src/futex-emulation.h
File src/futex-emulation.h (right):

https://codereview.chromium.org/1208933006/diff/160001/src/futex-emulation.h#...
src/futex-emulation.h:38: : prev_(NULL),
NULL -> nullptr in new code, please.

https://codereview.chromium.org/1208933006/diff/160001/src/harmony-atomics.js
File src/harmony-atomics.js (right):

https://codereview.chromium.org/1208933006/diff/160001/src/harmony-atomics.js...
src/harmony-atomics.js:136: if (index < 0 || index >= ia.length) {
I believe you should not rely on the length property here because it could be
monkey-patched. You should use %_TypedArrayGetLength(ia) here.

Could you add tests with monkey-patched length and byteLength properties for all
the atomic ops?

[binji, 2015-07-14 19:24:37]

https://codereview.chromium.org/1208933006/diff/160001/src/futex-emulation.cc
File src/futex-emulation.cc (right):

https://codereview.chromium.org/1208933006/diff/160001/src/futex-emulation.cc...
src/futex-emulation.cc:18: FutexWaitList FutexEmulation::wait_list_;
On 2015/07/10 at 12:11:39, jarin wrote:
> Static initializers are not allowed by Chrome. You should use the LazyInstance
template.

Done.

https://codereview.chromium.org/1208933006/diff/160001/src/futex-emulation.cc...
src/futex-emulation.cc:39: if (node->prev_)
On 2015/07/10 at 12:11:39, jarin wrote:
> braces, please.

Done.

https://codereview.chromium.org/1208933006/diff/160001/src/futex-emulation.h
File src/futex-emulation.h (right):

https://codereview.chromium.org/1208933006/diff/160001/src/futex-emulation.h#...
src/futex-emulation.h:38: : prev_(NULL),
On 2015/07/10 at 12:11:39, jarin wrote:
> NULL -> nullptr in new code, please.

Done.

https://codereview.chromium.org/1208933006/diff/160001/src/harmony-atomics.js
File src/harmony-atomics.js (right):

https://codereview.chromium.org/1208933006/diff/160001/src/harmony-atomics.js...
src/harmony-atomics.js:136: if (index < 0 || index >= ia.length) {
On 2015/07/10 at 12:11:39, jarin wrote:
> I believe you should not rely on the length property here because it could be
monkey-patched. You should use %_TypedArrayGetLength(ia) here.

Done.

> 
> Could you add tests with monkey-patched length and byteLength properties for
all the atomic ops?

Landed in separate CL: https://codereview.chromium.org/1227913006

[Jarin, 2015-07-15 07:40:18]

jarin@chromium.org changed reviewers:
+ jochen@chromium.org

[Jarin, 2015-07-15 07:42:21]

Adding jochen to discuss the blocking of main thread.

https://codereview.chromium.org/1208933006/diff/180001/src/futex-emulation.cc
File src/futex-emulation.cc (right):

https://codereview.chromium.org/1208933006/diff/180001/src/futex-emulation.cc...
src/futex-emulation.cc:80: node->cond_.Wait(mutex_.Pointer());
Benedikt pointed out that we cannot block the main thread indefinitely.

Either we block only for small time quanta and allow interruption from the
embedder (i.e., Blink) or we find somehow let the embedder do the waiting.

[jochen (gone - plz use gerrit), 2015-07-15 13:41:35]

On 2015/07/15 at 07:42:21, jarin wrote:
> Adding jochen to discuss the blocking of main thread.
> 
> https://codereview.chromium.org/1208933006/diff/180001/src/futex-emulation.cc
> File src/futex-emulation.cc (right):
> 
>
https://codereview.chromium.org/1208933006/diff/180001/src/futex-emulation.cc...
> src/futex-emulation.cc:80: node->cond_.Wait(mutex_.Pointer());
> Benedikt pointed out that we cannot block the main thread indefinitely.
> 
> Either we block only for small time quanta and allow interruption from the
embedder (i.e., Blink) or we find somehow let the embedder do the waiting.

i don't think there's much we can do but block here.

[binji, 2015-07-15 18:48:42]

On 2015/07/15 at 13:41:35, jochen wrote:
> On 2015/07/15 at 07:42:21, jarin wrote:
> > Adding jochen to discuss the blocking of main thread.
> > 
> >
https://codereview.chromium.org/1208933006/diff/180001/src/futex-emulation.cc
> > File src/futex-emulation.cc (right):
> > 
> >
https://codereview.chromium.org/1208933006/diff/180001/src/futex-emulation.cc...
> > src/futex-emulation.cc:80: node->cond_.Wait(mutex_.Pointer());
> > Benedikt pointed out that we cannot block the main thread indefinitely.
> > 
> > Either we block only for small time quanta and allow interruption from the
embedder (i.e., Blink) or we find somehow let the embedder do the waiting.
> 
> i don't think there's much we can do but block here.

Isn't this fundamentally the same thing the user can already do with while(1){}?

I've been told that there is an extra check on back-edges... perhaps it is worth
setting a smaller timeout and adding a similar check?

[jochen (gone - plz use gerrit), 2015-07-15 18:50:53]

yeah, it's essentially the same as while (1) {} - nothing gets rendered, and
after a while you'll get the "tab unresponsive, kill?" dialog.

[Jarin, 2015-07-16 07:36:13]

On 2015/07/15 18:50:53, jochen wrote:
> yeah, it's essentially the same as while (1) {} - nothing gets rendered, and
> after a while you'll get the "tab unresponsive, kill?" dialog.

With while(1) {} you still get stack checks, so you can interrupt (e.g., from
the debugger or some such). Here, there is no possibility to interrupt.

[binji, 2015-07-16 21:29:17]

On 2015/07/16 at 07:36:13, jarin wrote:
> On 2015/07/15 18:50:53, jochen wrote:
> > yeah, it's essentially the same as while (1) {} - nothing gets rendered, and
> > after a while you'll get the "tab unresponsive, kill?" dialog.
> 
> With while(1) {} you still get stack checks, so you can interrupt (e.g., from
the debugger or some such). Here, there is no possibility to interrupt.

PTAL

I've added interrupt checks to FutexEmulation::Wait and a test.

[Jarin, 2015-07-17 12:55:28]

lgtm. (but we should revisit the busy wait at some point.)

https://codereview.chromium.org/1208933006/diff/240001/src/futex-emulation.cc
File src/futex-emulation.cc (right):

https://codereview.chromium.org/1208933006/diff/240001/src/futex-emulation.cc...
src/futex-emulation.cc:63: // TODO(binji): How long should this be? Should it be
configurable?
No idea. Ideally, we should have some notification from the interrupt mechanism
so that you do not have to busy-wait here. Perhaps you could add a TODO here
that reminds us to fix the busy wait?

https://codereview.chromium.org/1208933006/diff/240001/test/cctest/test-api.cc
File test/cctest/test-api.cc (right):

https://codereview.chromium.org/1208933006/diff/240001/test/cctest/test-api.c...
test/cctest/test-api.cc:21866: };
Thanks for the test, you are my hero!

[binji, 2015-07-17 16:29:22]

https://codereview.chromium.org/1208933006/diff/240001/src/futex-emulation.cc
File src/futex-emulation.cc (right):

https://codereview.chromium.org/1208933006/diff/240001/src/futex-emulation.cc...
src/futex-emulation.cc:63: // TODO(binji): How long should this be? Should it be
configurable?
On 2015/07/17 at 12:55:27, jarin wrote:
> No idea. Ideally, we should have some notification from the interrupt
mechanism so that you do not have to busy-wait here. Perhaps you could add a
TODO here that reminds us to fix the busy wait?

Done.

I was thinking about this: because there is only one waiter per isolate, and the
waiting node is stored in the isolate, it may not be too hard to signal that
waiter from HandleInterrupts.

https://codereview.chromium.org/1208933006/diff/240001/test/cctest/test-api.cc
File test/cctest/test-api.cc (right):

https://codereview.chromium.org/1208933006/diff/240001/test/cctest/test-api.c...
test/cctest/test-api.cc:21866: };
On 2015/07/17 at 12:55:27, jarin wrote:
> Thanks for the test, you are my hero!

Thanks :)

[binji, 2015-07-17 16:29:28]

The CQ bit was checked by binji@chromium.org

[binji, 2015-07-17 16:29:28]

The patchset sent to the CQ was uploaded after l-g-t-m from jarin@chromium.org
Link to the patchset: https://codereview.chromium.org/1208933006/#ps260001
(title: "TODO about busy-waiting")

[commit-bot: I haz the power, 2015-07-17 16:29:40]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1208933006/260001

[commit-bot: I haz the power, 2015-07-17 17:11:41]

Committed patchset #14 (id:260001)

[commit-bot: I haz the power, 2015-07-17 17:11:54]

Patchset 14 (id:??) landed as
https://crrev.com/35b2114874bc37fb5fc839244f9794567d5d31d4
Cr-Commit-Position: refs/heads/master@{#29736}