This repository has been archived by the owner. It is now read-only.

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

Closed
wants to merge 15 commits into
base: latest
from

Conversation

Projects
None yet
10 participants
@addaleax
Contributor

addaleax commented Sep 15, 2017

(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
  • maybe turn the message passing mechanisms into standard MessageChannels
  • implement ArrayBuffer transferring
  • implement SharedArrayBuffer, uh, Sharing
  • implement MessagePort transferring
  • remove the need to call .start() manually for MessagePorts
  • add tests for MessageChannels
  • improve uncaught exception serialization/deserialization
  • fix garbage collection tracking through performance (currently pending the V8 6.1 update in upstream Node.js)
  • add async hooks tests for Workers
  • figure out how to best run as many parallel/ tests as possible from Workers (did that by basically taking @petkaantonov’s approach directly from petkaantonov/io.js@ea143f7)
  • figure out whether and how this can be integrated with the inspector
  • figure out a native addon story
  • make deserialization context for MessagePorts configurable
  • look out for memory leaks created by node internals not cleaning up on environment destruction
  • integrate v8::Platform implementation with multi-isolate support

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

@addaleax addaleax referenced this pull request Sep 15, 2017

Closed

worker: initial implementation #40

14 of 19 tasks complete
@YafimK

This comment has been minimized.

Show comment
Hide comment
@YafimK

YafimK Sep 17, 2017

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.

YafimK commented Sep 17, 2017

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

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax Sep 17, 2017

Contributor

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…)

Contributor

addaleax commented Sep 17, 2017

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

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax Sep 17, 2017

Contributor

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

Contributor

addaleax commented Sep 17, 2017

@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!

Show outdated Hide outdated doc/api/vm.md Outdated
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,

This comment has been minimized.

@TimothyGu
@TimothyGu

TimothyGu Sep 18, 2017

Member

This comment has been minimized.

@addaleax

addaleax Sep 18, 2017

Contributor

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

addaleax Sep 18, 2017

Contributor

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)

This comment has been minimized.

@TimothyGu

TimothyGu Sep 19, 2017

Member

@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?

@TimothyGu

TimothyGu Sep 19, 2017

Member

@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?

This comment has been minimized.

@addaleax

addaleax Sep 19, 2017

Contributor

(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].
@addaleax

addaleax Sep 19, 2017

Contributor

(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].
Show outdated Hide outdated doc/api/worker.md Outdated
Show outdated Hide outdated doc/api/worker.md Outdated
<!-- YAML
added: REPLACEME
-->

This comment has been minimized.

@TimothyGu

TimothyGu Sep 18, 2017

Member

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

@TimothyGu

TimothyGu Sep 18, 2017

Member

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

This comment has been minimized.

@addaleax

addaleax Sep 19, 2017

Contributor

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.

@addaleax

addaleax Sep 19, 2017

Contributor

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.

This comment has been minimized.

@TimothyGu

TimothyGu Sep 19, 2017

Member

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 ...
@TimothyGu

TimothyGu Sep 19, 2017

Member

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 ...

This comment has been minimized.

@addaleax

addaleax Sep 19, 2017

Contributor

Ah, thanks – done!

@addaleax

addaleax Sep 19, 2017

Contributor

Ah, thanks – done!

Show outdated Hide outdated src/node_messaging.cc Outdated
Show outdated Hide outdated doc/api/worker.md Outdated
Show outdated Hide outdated doc/api/vm.md Outdated
Show outdated Hide outdated src/node_messaging.cc Outdated
Show outdated Hide outdated src/node_messaging.cc Outdated
Show outdated Hide outdated src/node_messaging.h Outdated
@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax Sep 19, 2017

Contributor

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

Contributor

addaleax commented Sep 19, 2017

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

@TimothyGu

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

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

This comment has been minimized.

@TimothyGu

TimothyGu Sep 19, 2017

Member
* `value` {any}
* `transferList` {Object[]}
@TimothyGu

TimothyGu Sep 19, 2017

Member
* `value` {any}
* `transferList` {Object[]}
<!-- YAML
added: REPLACEME
-->

This comment has been minimized.

@TimothyGu

TimothyGu Sep 19, 2017

Member

Ditto.

@TimothyGu

TimothyGu Sep 19, 2017

Member

Ditto.

This comment has been minimized.

@addaleax

addaleax Sep 19, 2017

Contributor

You mean, refer to cluster here?

@addaleax

addaleax Sep 19, 2017

Contributor

You mean, refer to cluster here?

This comment has been minimized.

@TimothyGu

TimothyGu Sep 20, 2017

Member

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

@TimothyGu

TimothyGu Sep 20, 2017

Member

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

This comment has been minimized.

@addaleax

addaleax Sep 20, 2017

Contributor

Ah – in that case, done :)

@addaleax

addaleax Sep 20, 2017

Contributor

Ah – in that case, done :)

Show outdated Hide outdated src/node_messaging.cc Outdated
Show outdated Hide outdated doc/api/worker.md Outdated
-->
The `Worker` class represents an independent JavaScript execution thread.
Most Ayo APIs are available inside of it.

This comment has been minimized.

@TimothyGu

TimothyGu Sep 19, 2017

Member

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

TimothyGu Sep 19, 2017

Member

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.

Show outdated Hide outdated doc/api/worker.md Outdated
Show outdated Hide outdated doc/api/vm.md Outdated
Show outdated Hide outdated doc/api/worker.md Outdated
Show outdated Hide outdated doc/api/worker.md Outdated
Show outdated Hide outdated doc/api/worker.md Outdated
@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax Sep 19, 2017

Contributor

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

Contributor

addaleax commented Sep 19, 2017

@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...

Show outdated Hide outdated src/node_messaging.cc Outdated
Show outdated Hide outdated src/node_messaging.cc Outdated
@TimothyGu

This comment has been minimized.

Show comment
Hide comment
@TimothyGu

TimothyGu Sep 20, 2017

Member

@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.

Member

TimothyGu commented Sep 20, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax Sep 20, 2017

Contributor

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

Contributor

addaleax commented Sep 20, 2017

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

@benjamingr

This comment has been minimized.

Show comment
Hide comment
@benjamingr

benjamingr Sep 20, 2017

Contributor

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

Contributor

benjamingr commented Sep 20, 2017

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

addaleax added a commit to addaleax/ayo that referenced this pull request Sep 20, 2017

src: fix `fs_stats_field_array` memory leak
Previously, the `Environment` destructor did not release its
`fs_stats_field_array` memory.

PR-URL: ayojs#58

addaleax added a commit to addaleax/ayo that referenced this pull request Sep 20, 2017

addaleax added a commit to addaleax/ayo that referenced this pull request Sep 20, 2017

addaleax added a commit to addaleax/ayo that referenced this pull request Sep 20, 2017

addaleax added a commit to addaleax/ayo that referenced this pull request Sep 20, 2017

addaleax added a commit to addaleax/ayo that referenced this pull request Sep 20, 2017

addaleax added a commit to addaleax/ayo that referenced this pull request Sep 20, 2017

addaleax added a commit to addaleax/ayo that referenced this pull request Sep 20, 2017

addaleax added a commit to addaleax/ayo that referenced this pull request Sep 20, 2017

addaleax added a commit to addaleax/ayo that referenced this pull request Sep 20, 2017

addaleax added a commit to addaleax/ayo that referenced this pull request Sep 20, 2017

addaleax added a commit to addaleax/ayo that referenced this pull request Sep 20, 2017

addaleax added a commit to addaleax/ayo that referenced this pull request Sep 20, 2017

addaleax added a commit to addaleax/ayo that referenced this pull request Sep 20, 2017

Show outdated Hide outdated lib/internal/process/stdio.js Outdated
Show outdated Hide outdated lib/internal/worker.js Outdated
@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax Oct 15, 2017

Contributor

I’ll rebase this later, there shouldn’t be any actually significant conflicts except maybe with nodejs/node@e5ad545

Contributor

addaleax commented Oct 15, 2017

I’ll rebase this later, there shouldn’t be any actually significant conflicts except maybe with nodejs/node@e5ad545

@Qard

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

@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax Oct 16, 2017

Contributor

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

Contributor

addaleax commented Oct 16, 2017

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

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

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

addaleax and others added some commits Sep 2, 2017

test: add more tests for workers
Taken from petkaantonov/io.js@ea143f7
and modified to fit current linter rules and coding style.
addons: integrate workers with native addons
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()
This should help a lot with actual sandboxing of JS code.
events: move EventEmitter to V8 Extras
Reviewed-By: Anna Henningsen <anna@addaleax.net>
worker: make MessagePort an EventEmitter always
Reviewed-By: Anna Henningsen <anna@addaleax.net>
worker: restrict supported extensions
Only allow `.js` and `.mjs` extensions to provide future-proofing
for file type detection.
benchmark: port cluster/echo to worker
$ ./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

@scotttrinh scotttrinh referenced this pull request Oct 27, 2017

Closed

what is ayo? #125

@TimothyGu TimothyGu referenced this pull request Oct 31, 2017

Closed

inspector: implement sub process inspection. #16627

0 of 4 tasks complete

@addaleax addaleax closed this Jun 11, 2018

@addaleax addaleax deleted the addaleax:workers-impl branch Jun 11, 2018

@p3x-robot

This comment has been minimized.

Show comment
Hide comment
@p3x-robot

p3x-robot Jun 26, 2018

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

p3x-robot commented Jun 26, 2018

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

@TimothyGu

This comment has been minimized.

Show comment
Hide comment
@TimothyGu

TimothyGu Jun 26, 2018

Member

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

Member

TimothyGu commented Jun 26, 2018

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.