Add d8 API for spawning function on a new thread.

Add d8 API for spawning function on a new thread.

This API closely matches the Worker API. The differences:

1) The argument to the Worker constructor is a function to run, not a script.
2) Receiving a message from a worker is a synchronous API (as there is no event
loop).

The serialization done here is not robust as the real DOM implementation. For
example, recursive data structures or otherwise duplicated objects are not
allowed.

BUG=none
R=jochen@chromium.org
LOG=n

Committed: https://crrev.com/3d98b956b56fa283b40913788ff760022d478812
Cr-Commit-Position: refs/heads/master@{#29126}

[binji, 2015-06-15 19:11:48]

PTAL

https://codereview.chromium.org/1185643004/diff/20001/src/d8.cc
File src/d8.cc (right):

https://codereview.chromium.org/1185643004/diff/20001/src/d8.cc#newcode1431
src/d8.cc:1431: static const char format[] = "JSON.stringify((%s).call(this,
sab));";
Better way to do this?

https://codereview.chromium.org/1185643004/diff/20001/src/d8.cc#newcode1465
src/d8.cc:1465: void Worker::ExecuteInThread() {
A lot of this function is copy/paste from other places in the file. Is it worth
trying to refactor?

https://codereview.chromium.org/1185643004/diff/20001/test/mjsunit/d8-worker.js
File test/mjsunit/d8-worker.js (right):

https://codereview.chromium.org/1185643004/diff/20001/test/mjsunit/d8-worker....
test/mjsunit/d8-worker.js:44: var result = Worker.join();
This API is a bit strange, but was the easiest to implement.

[jochen (gone - plz use gerrit), 2015-06-16 13:29:18]

thx for the patch.

I think it would be nice if the API was closer to the Web API, i.e. something
like

var w = new Worker('path/to/script.js');
w.postMessage({thisParam: shouldBeTransferredAsJson}, [ab1, ab2]);
w.terminate();

[Jakob Kummerow, 2015-06-16 14:03:36]

DBC:

> var w = new Worker('path/to/script.js');

This looks like it would require separate .js files, which would make it hard to
use this feature in mjsunit tests. (Yes we could support it, but we'd have to
add special-case hackery to blacklist those files from being treated like
standalone test cases.)
How about passing a source string (representing the contents of the assumed
file) instead of a file name? Other than that, staying close to the Web API
sounds good.

> w.postMessage({thisParam: shouldBeTransferredAsJson}, [ab1, ab2]);
> w.terminate();

[jochen (gone - plz use gerrit), 2015-06-16 14:04:21]

On 2015/06/16 at 14:03:36, jkummerow wrote:
> DBC:
> 
> > var w = new Worker('path/to/script.js');
> 
> This looks like it would require separate .js files, which would make it hard
to use this feature in mjsunit tests. (Yes we could support it, but we'd have to
add special-case hackery to blacklist those files from being treated like
standalone test cases.)
> How about passing a source string (representing the contents of the assumed
file) instead of a file name? Other than that, staying close to the Web API
sounds good.
> 
> > w.postMessage({thisParam: shouldBeTransferredAsJson}, [ab1, ab2]);
> > w.terminate();

fine by me

[binji, 2015-06-17 17:20:24]

On 2015/06/16 at 14:04:21, jochen wrote:
> On 2015/06/16 at 14:03:36, jkummerow wrote:
> > DBC:
> > 
> > > var w = new Worker('path/to/script.js');
> > 
> > This looks like it would require separate .js files, which would make it
hard to use this feature in mjsunit tests. (Yes we could support it, but we'd
have to add special-case hackery to blacklist those files from being treated
like standalone test cases.)
> > How about passing a source string (representing the contents of the assumed
file) instead of a file name? Other than that, staying close to the Web API
sounds good.

I used a function instead, which I think is easier to read/write than a single
string. One drawback is that it is perhaps less clear that the function does not
inherit its lexical context like a function normally would. What do you think?

> > 
> > > w.postMessage({thisParam: shouldBeTransferredAsJson}, [ab1, ab2]);
> > > w.terminate();
> 
> fine by me

Still working on it, but patch 3 is closer to matching the Worker API. One thing
is sending messages back from the worker to the main thread. Since the main
thread isn't running an event loop, it will have to be a synchronous API. Maybe
just:

    w = new Worker(f);
    ...
    msg = w.getMessage();  // blocks
    w.terminate();

[binji, 2015-06-17 23:01:59]

On 2015/06/17 at 17:20:24, binji wrote:
> On 2015/06/16 at 14:04:21, jochen wrote:
> > On 2015/06/16 at 14:03:36, jkummerow wrote:
> > > DBC:
> > > 
> > > > var w = new Worker('path/to/script.js');
> > > 
> > > This looks like it would require separate .js files, which would make it
hard to use this feature in mjsunit tests. (Yes we could support it, but we'd
have to add special-case hackery to blacklist those files from being treated
like standalone test cases.)
> > > How about passing a source string (representing the contents of the
assumed file) instead of a file name? Other than that, staying close to the Web
API sounds good.
> 
> I used a function instead, which I think is easier to read/write than a single
string. One drawback is that it is perhaps less clear that the function does not
inherit its lexical context like a function normally would. What do you think?
> 
> > > 
> > > > w.postMessage({thisParam: shouldBeTransferredAsJson}, [ab1, ab2]);
> > > > w.terminate();
> > 
> > fine by me
> 
> Still working on it, but patch 3 is closer to matching the Worker API. One
thing is sending messages back from the worker to the main thread. Since the
main thread isn't running an event loop, it will have to be a synchronous API.
Maybe just:
> 
>     w = new Worker(f);
>     ...
>     msg = w.getMessage();  // blocks
>     w.terminate();

Added this method, PTAL

[jochen (gone - plz use gerrit), 2015-06-18 13:01:30]

what about just adding v8::JSON::Stringify() and use parse/stringify for
serialization?

[binji, 2015-06-18 15:28:11]

On 2015/06/18 at 13:01:30, jochen wrote:
> what about just adding v8::JSON::Stringify() and use parse/stringify for
serialization?

JSON.stringify doesn't support serialization of cloned or transferred
ArrayBuffers and SharedArrayBuffers.
JSON doesn't really have any extension points in the format for describing
Objects either -- I'd have to
use some key-value pair as a convention for a tag (e.g. {$type: "ArrayBuffer"}).
Doable, but kind of
ugly.

I suppose I could add a flag or something, but it seems strange to do so if it
is only for testing.

[jochen (gone - plz use gerrit), 2015-06-18 15:32:54]

fair enough

lgtm

[binji, 2015-06-18 19:27:33]

The CQ bit was checked by binji@chromium.org

[binji, 2015-06-18 19:27:34]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org
Link to the patchset: https://codereview.chromium.org/1185643004/#ps100001
(title: "fix gcc build")

[commit-bot: I haz the power, 2015-06-18 19:27:54]

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

[commit-bot: I haz the power, 2015-06-18 19:46:19]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-06-18 19:46:33]

Patchset 6 (id:??) landed as
https://crrev.com/3d98b956b56fa283b40913788ff760022d478812
Cr-Commit-Position: refs/heads/master@{#29126}

[binji, 2015-06-18 20:44:38]

A revert of this CL (patchset #6 id:100001) has been created in
https://codereview.chromium.org/1192193002/ by binji@chromium.org.

The reason for reverting is: Fails on V8 Win32 - nosnap - shared
(http://build.chromium.org/p/client.v8/builders/V8%20Win32%20-%20nosnap%20-%20...)

Fails on V8 Arm - debug builder
(http://build.chromium.org/p/client.v8/builders/V8%20Arm%20-%20debug%20builder...)

Fails on V8 Linux - shared
(http://build.chromium.org/p/client.v8/builders/V8%20Linux%20-%20shared/builds...).

[Benedikt Meurer, 2015-06-19 05:00:11]

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

[Benedikt Meurer, 2015-06-19 05:00:12]

https://codereview.chromium.org/1185643004/diff/100001/src/d8.h
File src/d8.h (right):

https://codereview.chromium.org/1185643004/diff/100001/src/d8.h#newcode13
src/d8.h:13: #include "src/unbound-queue.h"
Don't use the UnboundedQueue. It's only purpose is to fullfil the needs of the
CPU profiler, which cannot use locking due to SuspendThread()/SIGPROF. Use a
normal mutex/semaphore guarded queue instead.

[binji, 2015-06-19 06:02:04]

https://codereview.chromium.org/1185643004/diff/100001/src/d8.h
File src/d8.h (right):

https://codereview.chromium.org/1185643004/diff/100001/src/d8.h#newcode13
src/d8.h:13: #include "src/unbound-queue.h"
On 2015/06/19 at 05:00:12, Benedikt Meurer wrote:
> Don't use the UnboundedQueue. It's only purpose is to fullfil the needs of the
CPU profiler, which cannot use locking due to SuspendThread()/SIGPROF. Use a
normal mutex/semaphore guarded queue instead.

Done. See fix in patch #3 here https://codereview.chromium.org/1195613003.

[Yang, 2015-06-19 06:13:00]

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

[Yang, 2015-06-19 06:13:01]

Not specifically about this CL, but at some point we should clean up d8 again.
Maybe spread it into several files. Rethink whether shared library build even
makes sense anymore.

[jochen (gone - plz use gerrit), 2015-06-19 06:32:16]

On 2015/06/19 at 06:13:01, yangguo wrote:
> Not specifically about this CL, but at some point we should clean up d8 again.
Maybe spread it into several files. Rethink whether shared library build even
makes sense anymore.

+1 to cleaning it up - but we need the shared library build for Chrome.

In fact, we should work towards not having a difference between the shared and
non-shared build. d8 should be just another embedder

[jochen (gone - plz use gerrit), 2015-06-19 06:32:55]

binji@ would you mind uploading the reland patch to a fresh code review?

[binji, 2015-06-19 06:33:55]

On 2015/06/19 at 06:32:55, jochen wrote:
> binji@ would you mind uploading the reland patch to a fresh code review?

Uploaded it here: https://codereview.chromium.org/1195613003