worker: initial implementation (large/base PR) #58

addaleax · 2017-09-15T18:40:52Z

(status: currently ready for review, by anyone, including you: #40 (comment))

'use strict';
const { Worker } = require('worker');

if (process.isMainThread) {
  module.exports = async function parseJSAsync(script) {
    return new Promise((resolve, reject) => {
      const worker = new Worker(__filename, {
        workerData: script
      });
      worker.on('message', resolve);
      worker.on('error', reject);
      worker.on('exit', (code) => {
        if (code !== 0)
          reject(new Error(`Worker stopped with exit code ${code}`));
      });
    });
  };
} else {
  const { parse } = require('some-js-parsing-library');
  const script = process.workerData;
  process.postMessage(parse(script));
}

(If that gives the wrong impression: No, this does not conform the browser WebWorker API, but that should be rather easily implementable on top of this, and I’m okay with that.)

Preliminary benchmarks: https://gist.github.com/TimothyGu/7fd2fbe15537a84963c36a3e0a03bcce
Fixes: #31

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

TODO

(@petkaantonov please feel free to indicate whether attributing you for code that comes from your original PR is not enough/too much/just right :) )

YafimK · 2017-09-17T10:55:55Z

Hi,
I am trying to compile the branch on my local windows machine and the compilation fails -

c:...\node_perf.h(44): error C2589: '(': illegal token on right side of '::' (compiling
source file src\node_worker.cc) [c:...\node.vcxproj]
c:...\node_perf.h(44): error C2062: type 'unknown-type' unexpected (compiling source fil
e src\node_worker.cc) [c:...\node.vcxproj]
c:...\node_perf.h(44):` error C2059: syntax error: ')' (compiling source file src\node_wo
rker.cc) [c:...node.vcxproj]

I have been able to compile clean version of Node.js without any issues on same machine before
Is compiling this branch on Windows is something that should already work or is it a future goal?

on a side note, not as a requirement or anything but as a mere suggestion for help :)
if you'd like, I'd be happy to help to setup another CI basing on AppVeyor so we'd be able to have continuously windows build ready.

addaleax · 2017-09-17T14:59:54Z

I am trying to compile the branch on my local windows machine and the compilation fails -

@YafimK Hm – I can’t quite figure out what the problem here is :/ Does it help if you just remove line 44 in src/node_perf.h? It doesn’t seem to be used anywhere…

Is compiling this branch on Windows is something that should already work or is it a future goal?

I think this isn’t really about this branch, but about Ayo in general – we don’t have much CI support yet, so talking about support for certain platforms is somewhat hard (but also see #25)

if you'd like, I'd be happy to help to setup another CI basing on AppVeyor so we'd be able to have continuously windows build ready.

If you can make that happen, that would be absolutely awesome 💙

(Also, just fyi, I rebased this against the current latest branch – I don’t think that’s going to make a huge difference, but maybe try rebuilding now…)

addaleax · 2017-09-17T22:50:39Z

@benjamingr @Qard I’ve moved everything new off the process object, PTAL

TimothyGu

Will need to look at the SharedArrayBuffer part more, but I really like the direction this is taking. Hats off to @addaleax!

TimothyGu · 2017-09-18T22:22:42Z

doc/api/vm.md

@@ -310,6 +310,27 @@ console.log(util.inspect(sandbox));
 // { globalVar: 1024 }
 ```

+## vm.moveMessagePortToContext(port, context)


s/context/\0ifiedSandbox/

TimothyGu · 2017-09-18T22:25:41Z

doc/api/worker.md

+
+Sends a JavaScript value to the receiving side of this channel.
+`value` will be transferred in a way
+that is compatible with the [HTML structured clone algorithm][]. In particular,


structured cloning isn't a thing any more (https://html.spec.whatwg.org/multipage/structured-data.html#structuredclone).

Heh. Right. I think this is still what it’s known as, though? (That it’s implemented as a serialize + deserialize step should be more or less transparent to users)

@addaleax That article is pretty outdated though (it has no mention of MessagePort or SharedArrayBuffer)... HTML structured serialization/deserialization? maybe a link to v8.Serializer/Deserializer?

(it has no mention of MessagePort or SharedArrayBuffer)

That’s why these are mentioned explicitly here ;)

maybe a link to v8.Serializer/Deserializer?

Yeah, that’s a good idea … that should give people an idea of how to play around with the algorithm without spinning up message channels every time. I’ve added:

+For more information on the serialization and deserialization mechanisms +behind this API, see the [serialization API of the `v8` module][v8.serdes].

TimothyGu · 2017-09-18T22:28:44Z

doc/api/worker.md

+-->
+
+The `Worker` class represents an independent JavaScript execution thread.
+Most Node APIs are available inside of it.


Ayo or Ayo.js.

Is Worker available in a worker?

Is Worker available in a worker?

Yes. That’s also tested and the code is generally laid out to support such a structure, in part because there’s not really anything standing in the way of it

TimothyGu · 2017-09-18T22:29:46Z

doc/api/worker.md

+* Returns: {undefined}
+
+Starts receiving messages on this `MessagePort`. When using this port
+as an event emitter, this will be called automatically once `message` listeners


nit: 'message'

TimothyGu · 2017-09-18T22:30:40Z

doc/api/worker.md

+<!-- YAML
+added: REPLACEME
+-->
+


What arguments does this event get? (value, transferList)?

Only value. We could try to add the transferList too, if that seems useful, but all of the relevant objects should be reachable through value anyway.

I don’t know if we have some standard format for saying “this events has one parameter that can be any JS value”, so I’m adding prose for it.

I think something like the following is used. Basically the same format as function parameters.

### Event: 'event' * `value` {any} The value This event is fired when ...

Ah, thanks – done!

TimothyGu · 2017-09-18T22:38:55Z

src/node_messaging.cc

+  MessagePort* port;
+  ASSIGN_OR_RETURN_UNWRAP(&port, args.This());
+  if (!port->data_) {
+    env->ThrowError("Can not send data on closed MessagePort");


nit: Cannot

TimothyGu · 2017-09-18T22:44:56Z

doc/api/worker.md

+
+The `MessageChannel` has no methods of its own. `new MessageChannel()`
+yields an object with `port1` and `port2` properties, which refer to linked
+[`MessagePort`][] instances.


Something I've been thinking is that the messaging API could well be applicable to VM contexts too. Is that a correct understanding?

Yes, that is a correct understanding, because the ports in a channel can be moved to different contexts

TimothyGu · 2017-09-18T22:46:10Z

doc/api/vm.md

@@ -310,6 +310,27 @@ console.log(util.inspect(sandbox));
 // { globalVar: 1024 }
 ```

+## vm.moveMessagePortToContext(port, context)


Maybe just vm.transferMessagePort()? The "to context" part is already conveyed by the fact that this method is in vm module, and "transfer" is a more Proper(R) name IMO than "move".

What I like about move rather than transfer is that it conveys better that the original message port object is now rendered unusable … I don’t feel strongly about it, though

TimothyGu · 2017-09-18T22:46:31Z

src/node_messaging.cc

+
+ExternalSABReference ReferenceCountedSAB::ForIncomingSharedArrayBuffer(
+    Environment* env, Local<Context> context, Local<SharedArrayBuffer> source) {
+  Local<Value> lifetime_partner;


TimothyGu · 2017-09-18T22:48:04Z

src/node_messaging.cc

+  Local<FunctionTemplate> templ;
+  templ = env->message_port_constructor_template();
+  if (!templ.IsEmpty())
+    return templ->GetFunction(context);


Is GetFunction() cached by context?

Yup – I have to admit I never got the point of {Function,Object}Templates before making this PR. However, in retrospect it’s a lot clearer: It’s a Template in the sense that it can be used to create functionally equivalent functions/objects in different contexts.

TimothyGu · 2017-09-18T22:52:04Z

src/node_messaging.h

+  void AddSharedArrayBuffer(ExternalSABReference ref);
+  // Internal method of Message that is called once serialization finishes
+  // and that transfers ownership of `data` to this message.
+  void AddMessagePort(std::unique_ptr<MessagePortData>&& data);


Make these private?

I think that would require giving the SerializerDelegate that accesses these a publicly visibly identifier, right?

addaleax · 2017-09-19T11:30:48Z

@TimothyGu I think I got everything you commented on so far? :)

TimothyGu

Some more feedback. Still haven't read much code yet.

TimothyGu · 2017-09-19T18:11:37Z

doc/api/worker.md

+added: REPLACEME
+-->
+
+* Returns: {undefined}


* `value` {any} * `transferList` {Object[]}

TimothyGu · 2017-09-19T19:46:32Z

doc/api/worker.md

+<!-- YAML
+added: REPLACEME
+-->
+


You mean, refer to cluster here?

No, I intended to refer to the comment of adding parameter types. Seems like github messed up the order :/

Ah – in that case, done :)

TimothyGu · 2017-09-19T19:54:11Z

src/node_messaging.cc

+}
+
+void MessagePortData::Entangle(MessagePortData* a, MessagePortData* b) {
+  a->sibling_ = b;


I'd add some CHECKs or even mutexes here making sure it's thread-safe, and only works when either port is not entangled.

I think CHECKs for making sure that the ports aren’t entangled are fine. The method isn’t really required or supposed to be thread-safe, I’ve noted that in the documentation. (It’s only called by the thread that created both message ports.)

TimothyGu · 2017-09-19T19:55:28Z

doc/api/worker.md

+use them for I/O, since Ayo’s built-in mechanisms for performing operations
+asynchronously already treat it more efficiently than Worker threads can.
+
+Workers can also, unlike child processes, share memory efficiently by


You can even explicitly call out clusters here.

TimothyGu · 2017-09-19T20:08:05Z

doc/api/worker.md

+-->
+
+The `Worker` class represents an independent JavaScript execution thread.
+Most Ayo APIs are available inside of it.


I would mention message passing as the primary means of communication between the worker thread and the thread that spawned the worker. Maybe something like:

Like [Web Workers] and the [cluster module], two-way communication can be achieved through inter-thread message passing. Internally, a Worker has a built-in pair of [MessagePort]s that are already associated with each other when the Worker is created. While the MessagePort objects are not directly exposed, their functionalities are exposed through [worker.postMessage()] and the ['message' event] on the Worker object for the parent thread, and [require('worker').postMessage()] and the ['workerMessage' event] on require('worker') for the child thread.

To create custom messaging channels (which is strongly encouraged over using the default global channel for more complex tasks), users can create a MessageChannel object on either thread and pass one of the MessagePorts on that MessageChannel to the other thread through a pre-existing channel, such as the global one.

[insert example]

See [port.postMessage()] for more information on how messages are passed, and what kind of JavaScript values can be successfully transported through the thread barrier.

TimothyGu · 2017-09-19T20:13:22Z

doc/api/worker.md

+
+The `MessageChannel` has no methods of its own. `new MessageChannel()`
+yields an object with `port1` and `port2` properties, which refer to linked
+[`MessagePort`][] instances.


I'd add a description of what MessageChannel is before saying what it has. Maybe:

The MessageChannel class represents an asynchronous messaging channel.

TimothyGu · 2017-09-19T20:19:07Z

doc/api/vm.md

+context.
+
+Note that the return instance is *not* an `EventEmitter`; for receiving
+messages, the `.onmessage` property can be used.


It seems rather crude to have some MessagePorts being EventEmitters, and some not. I understand you are still working on this part, and that the EventEmitter is implemented in JS complicates things. But I'd rather have it all one way or the other.

But I'd rather have it all one way or the other.

Me too. :) But like with the Buffers, it’s just a problem that not all contexts have a concept of EventEmitters … this would require making some internal modules multi-context ready, which I would like to consider out of scope for this PR. :) Also, this is already quite an advanced feature on its own.

TimothyGu · 2017-09-19T20:22:49Z

doc/api/worker.md

+* Extends: {EventEmitter}
+
+Instances of the `worker.MessagePort` class represent an asynchronous
+communications channel. It can be used to transfer structured data,


s/an asynchronous communications channel/an end(or port?) of a two-way asynchronous communications channel/

TimothyGu · 2017-09-19T20:34:18Z

doc/api/worker.md

+
+`transferList` may be a list of `ArrayBuffer` and `MessagePort` objects.
+After transferring, they will not be usable on the sending side of the channel
+anymore.


I guess one of the main thing that confused me was how transferList was used. So now I understand that objects present in value (no matter how deep) that are also in transferList are transferred, while ordinarily they are cloned. I wasn't aware of the "ordinarily" part.

However, what if transferList has an object that is not present in value? Will it get neutered as well?

I’m adding clarifications for both points, thanks

TimothyGu · 2017-09-19T20:39:44Z

doc/api/worker.md

+communications channel. It can be used to transfer structured data,
+memory regions and other `MessagePort`s between different [`Worker`][]s
+or [`vm` context][vm]s.
+


Also, please document if/when open() and close() need to be called. An example would be best.

It’s hard to come up with examples for these … I’ve added to the start() documentation that it doesn’t need to be used unless moving-between-VM-contexts is used. (And, as you pointed out below, ideally it wouldn’t be necessary at all).

Regarding close() … I’m not entirely sure whether MessagePorts should be unref()ed by default or not. Right now, any MessagePort keeps an event loop open unless explicitly unref()ed or close()ed …

addaleax · 2017-09-19T22:38:08Z

@TimothyGu I think I got more or less everything … the Buffer thing really annoys me as well, I’ll think about it a bit.

TimothyGu

Making progress...

TimothyGu · 2017-09-20T07:39:00Z

src/node_messaging.cc

+  void Finish() {
+    for (MessagePort* port : ports_) {
+      port->Close();
+      msg_->AddMessagePort(std::move(port->Detach()));


Is the std::move redundant if MessagePort::Detach already returns a std::move'd smart pointer?

Clang complains:

../src/node_messaging.cc:325:28: warning: moving a temporary object prevents copy elision [-Wpessimizing-move] msg_->AddMessagePort(std::move(port->Detach())); ^ ../src/node_messaging.cc:325:28: note: remove std::move call here msg_->AddMessagePort(std::move(port->Detach())); ^~~~~~~~~~ ~

Will fix.

TimothyGu · 2017-09-20T07:39:26Z

src/node_messaging.cc

+  }
+  Context::Scope context_scope(context);
+  MessagePort* target =
+      MessagePort::New(env, context, nullptr, std::move(port->data_));


This std::move(port->data_) should be port->Detach().

TimothyGu · 2017-09-20T08:15:14Z

@addaleax I've completed making MessagePort consistently EventEmitter using the V8 Extras approach I talked about on Discord. See bb85e11 and 62a0263. The same approach could be taken to make Buffer available everywhere as well.

While I agree that it isn't the most in scope for this PR, it shows something pretty promising IMO.

addaleax · 2017-09-20T13:12:02Z

@TimothyGu that is, indeed, very nice. 👏 The commits generally LGTM, I’ll pull them in here

benjamingr · 2017-09-20T13:23:31Z

With the changes pulled this mostly LGTM - hopefully I'll be able to get some interesting benchmarks going.

Qard

b170b46 LGTM other than minor suggestion to have isMainThread getter rather than threadId === 0 in a bunch of places.

addaleax · 2017-10-16T19:39:00Z

Fwiw I “fixed” the V8 extras + events + internal errors issue by manually editing the error messages and tacking on .code properties … not pretty but I think this should be fine for most practical purposes

Taken from petkaantonov/io.js@ea143f7 and modified to fit current linter rules and coding style.

Native addons need to use flags to indicate that they are capable of being loaded by worker threads. Native addons are unloaded if all Environments referring to it have been cleaned up, except if it also loaded by the main Environment.

This should help a lot with actual sandboxing of JS code.

Reviewed-By: Anna Henningsen <[email protected]>

Only allow `.js` and `.mjs` extensions to provide future-proofing for file type detection.

$ ./ayo benchmark/cluster/echo.js cluster/echo.js n=100000 sendsPerBroadcast=1 payload="string" workers=1: 26,709.154114687004 cluster/echo.js n=100000 sendsPerBroadcast=10 payload="string" workers=1: 15,936.350422945854 cluster/echo.js n=100000 sendsPerBroadcast=1 payload="object" workers=1: 20,778.85550744996 cluster/echo.js n=100000 sendsPerBroadcast=10 payload="object" workers=1: 10,912.260027807712 $ ./ayo benchmark/worker/echo.js worker/echo.js n=100000 sendsPerBroadcast=1 payload="string" workers=1: 69,787.63926344117 worker/echo.js n=100000 sendsPerBroadcast=10 payload="string" workers=1: 32,544.630210444844 worker/echo.js n=100000 sendsPerBroadcast=1 payload="object" workers=1: 48,706.90345844702 worker/echo.js n=100000 sendsPerBroadcast=10 payload="object" workers=1: 18,088.639282621873

p3x-robot · 2018-06-26T06:03:29Z

does it mean that we are not use thread in nodejs? or jsut this branch is deleted?

TimothyGu · 2018-06-26T16:25:01Z

@p3x-robot A newer version of this PR was merged into Node.js' master branch.

addaleax mentioned this pull request Sep 15, 2017

worker: initial implementation #40

Closed

19 tasks

addaleax force-pushed the workers-impl branch from 47e8463 to 70921b6 Compare September 17, 2017 14:59

addaleax force-pushed the workers-impl branch from 70921b6 to 1aa0013 Compare September 17, 2017 22:50

addaleax force-pushed the workers-impl branch from 1aa0013 to 00bb418 Compare September 18, 2017 00:15

TimothyGu reviewed Sep 18, 2017

View reviewed changes

brodycj mentioned this pull request Sep 18, 2017

Is ongoing interoperability with Node.js a requirement for Ayo? #68

Closed

addaleax force-pushed the workers-impl branch from 00bb418 to 9d3a05f Compare September 18, 2017 23:12

This was referenced Sep 18, 2017

Define clear goals/motivations of the Ayo.js project #67

Closed

please develop native threads that can load native modules with require and share/lock objects #31

Open

addaleax force-pushed the workers-impl branch 2 times, most recently from 5d36c5d to 5b0aca3 Compare September 19, 2017 11:30

addaleax force-pushed the workers-impl branch 2 times, most recently from 43b4725 to 5d793a4 Compare September 19, 2017 18:13

TimothyGu reviewed Sep 19, 2017

View reviewed changes

addaleax force-pushed the workers-impl branch 2 times, most recently from 8b087d0 to a2095eb Compare September 19, 2017 22:33

addaleax force-pushed the workers-impl branch from a2095eb to 0693217 Compare September 20, 2017 01:36

TimothyGu reviewed Sep 20, 2017

View reviewed changes

TimothyGu force-pushed the workers-impl branch from 0693217 to ef85bd3 Compare September 20, 2017 07:52

addaleax force-pushed the workers-impl branch from ef85bd3 to d76b9ec Compare September 20, 2017 13:01

addaleax force-pushed the workers-impl branch from d76b9ec to f715000 Compare September 20, 2017 13:38

Qard reviewed Oct 15, 2017

View reviewed changes

addaleax force-pushed the workers-impl branch from adf9752 to 56d80a6 Compare October 16, 2017 19:37

addaleax force-pushed the workers-impl branch from 56d80a6 to c2b23bb Compare October 19, 2017 16:50

addaleax changed the title ~~worker: initial implementation~~ worker: initial implementation (largs/base PR) Oct 19, 2017

addaleax changed the title ~~worker: initial implementation (largs/base PR)~~ worker: initial implementation (large/base PR) Oct 19, 2017

addaleax force-pushed the workers-impl branch from c2b23bb to d35f78d Compare October 22, 2017 11:35

addaleax and others added 15 commits October 22, 2017 21:58

console: make global console work in workers

fbba19f

test: add basic tests for MessageChannel

aac5667

test: add basic tests for worker

9f32cb4

test: add more tests for workers

0662bf1

Taken from petkaantonov/io.js@ea143f7 and modified to fit current linter rules and coding style.

addons: integrate workers with native addons

15be1c5

Native addons need to use flags to indicate that they are capable of being loaded by worker threads. Native addons are unloaded if all Environments referring to it have been cleaned up, except if it also loaded by the main Environment.

worker: implement vm.moveMessagePortToContext()

692e369

This should help a lot with actual sandboxing of JS code.

events: move EventEmitter to V8 Extras

84fc9ce

Reviewed-By: Anna Henningsen <[email protected]>

[squash] add error codes

5229174

worker: make MessagePort an EventEmitter always

dc193a5

Reviewed-By: Anna Henningsen <[email protected]>

doc: add documentation for workers module

2379ef3

worker: restrict supported extensions

10d7b54

Only allow `.js` and `.mjs` extensions to provide future-proofing for file type detection.

test: add workers + ES Module test

3ef9e6a

test: add test against unsupported worker features

0046907

worker: improve error (de)serialization

9fb522e

addaleax force-pushed the workers-impl branch from d35f78d to 9fb522e Compare October 22, 2017 20:01

addaleax mentioned this pull request Oct 25, 2017

Promises allow vm.runInContext timeout to be escaped nodejs/node#3020

Closed

scotttrinh mentioned this pull request Oct 27, 2017

what is ayo? #125

Closed

TimothyGu mentioned this pull request Oct 31, 2017

inspector: implement sub process inspection. nodejs/node#16627

Closed

4 tasks

addaleax closed this Jun 11, 2018

addaleax deleted the workers-impl branch June 11, 2018 18:13

worker: initial implementation (large/base PR) #58

worker: initial implementation (large/base PR) #58

Conversation

addaleax commented Sep 15, 2017 • edited Loading

Checklist

TODO

YafimK commented Sep 17, 2017

addaleax commented Sep 17, 2017

addaleax commented Sep 17, 2017

TimothyGu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

addaleax Sep 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

addaleax commented Sep 19, 2017

TimothyGu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

addaleax commented Sep 19, 2017

TimothyGu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TimothyGu commented Sep 20, 2017

addaleax commented Sep 20, 2017

benjamingr commented Sep 20, 2017

Qard left a comment

Choose a reason for hiding this comment

addaleax commented Oct 16, 2017

p3x-robot commented Jun 26, 2018

TimothyGu commented Jun 26, 2018

addaleax commented Sep 15, 2017 •

edited

Loading

addaleax Sep 18, 2017 •

edited

Loading