WIP: t31135461 / Refine jest-worker API by Kureev · Pull Request #6676 · jestjs/jest

Kureev · 2018-07-11T10:11:03Z

Summary

Node 10 shipped with a "threading API" that uses SharedBuffers to communicate between the main process and its child threads. Being jest-worker a parallelization library, we can take advantage of this API when available. At the same time, we'll decouple our scheduling logic from our communication logic for better re-usability.

Here are some key things:

Decouple the scheduling logic (i.e. everything that's not specifically to send and receive jobs)
Add the option to use "threads" into the library. If they are present, we'll favor them.

Performance

➜ node --experimental-worker --expose-gc test.js empty 10000
---------------------------------------------------------------------------
jest-worker: { globalTime: 272, processingTime: 217 }
worker-farm: { globalTime: 521, processingTime: 462 }
---------------------------------------------------------------------------
jest-worker: { globalTime: 244, processingTime: 210 }
worker-farm: { globalTime: 502, processingTime: 451 }
---------------------------------------------------------------------------
jest-worker: { globalTime: 290, processingTime: 253 }
worker-farm: { globalTime: 455, processingTime: 433 }
---------------------------------------------------------------------------
jest-worker: { globalTime: 262, processingTime: 229 }
worker-farm: { globalTime: 668, processingTime: 634 }
---------------------------------------------------------------------------
jest-worker: { globalTime: 280, processingTime: 222 }
worker-farm: { globalTime: 600, processingTime: 548 }
---------------------------------------------------------------------------
jest-worker: { globalTime: 222, processingTime: 181 }
worker-farm: { globalTime: 490, processingTime: 459 }
---------------------------------------------------------------------------
jest-worker: { globalTime: 225, processingTime: 181 }
worker-farm: { globalTime: 469, processingTime: 428 }
---------------------------------------------------------------------------
jest-worker: { globalTime: 220, processingTime: 180 }
worker-farm: { globalTime: 437, processingTime: 407 }
---------------------------------------------------------------------------
jest-worker: { globalTime: 214, processingTime: 176 }
worker-farm: { globalTime: 489, processingTime: 444 }
---------------------------------------------------------------------------
jest-worker: { globalTime: 241, processingTime: 191 }
worker-farm: { globalTime: 440, processingTime: 412 }
---------------------------------------------------------------------------
total worker-farm: { wFGT: 5071, wFPT: 4678 }
total jest-worker: { jWGT: 2470, jWPT: 2040 }
---------------------------------------------------------------------------
% improvement over 10000 calls (global time): 51.29165845000986
% improvement over 10000 calls (processing time): 56.39162035057717

Test plan

TBA

Clean up types, create missing interfaes and make Flow and Jest happy

Kureev · 2018-07-11T10:12:24Z

Please, take into account that this is a WIP PR. Currently, there are a few parts missing:

Retry in case of a failure
Tests
Usage docs

Kureev · 2018-07-11T10:13:31Z

+    this._locks = [];
+    this._offset = 0;
+
+    // If we exceeded the amount of retries, we will emulate an error reply


This part will be rewritten and removed from here. This a part of missing functionality I've mentioned in the first comment to the PR

mjesun · 2018-07-11T10:21:46Z

+      const numOfWorkers = this._workerPool.getWorkers().length;
+      for (let i = 0; i < numOfWorkers; i++) {
+        const workerIdx = (this._offset + i) % numOfWorkers;
+        this.enqueue(task, workerIdx);


Separate the initial part of the function into another one, to avoid recursivity.

I can use enqueue(QueueChildMessage, number) for scheduling to a specific worker and push(QueueChildMessage) to schedule a job in all workers. wdyt?

What I mean is to move the for loop into the parent implementation, where you will call enqueue always with a workerId. This way workerId won't be optional anymore. This will also allow you to remove getWorkers, because you won't need it to get the length anymore.

From the design perspective I'd say Farm shouldn't be concerned how WorkerPool spread tasks among workers. It is fine that Farm can pin job to the worker if it has a reference to it, however it isn't cool if Farm contains queue distribution logic in it 🤔

👍🏻 Let's separate it into a different method as discussed offline.

mjesun · 2018-07-11T10:25:21Z

+  _options: FarmOptions;
+  _stderr: Readable;
+  _stdout: Readable;
+  _workers: Array<WorkerInterface>;


The interface shouldn't have any of the private variables.

mjesun · 2018-07-11T10:26:16Z

+  send(request: ChildMessage, onProcessStart: OnStart, onProcessEnd: OnEnd) {
+    onProcessStart(this);
+    this._onProcessEnd = onProcessEnd;
+    // $FlowFixMe


¿
this._child.send(request) doesn't expect to receive a request type

mjesun · 2018-07-11T10:28:00Z

+  ]);
+}
+
+function execMethod(method: string, args: $ReadOnlyArray<any>): void {


Can you create a common file within the workers/ folder that abstract this functionality? It looks like child and threadChild are 90% the same.

mjesun · 2018-07-11T10:28:42Z

 * non-zero exit code.
 */
-process.on('message', (request: any /* Should be ChildMessage */) => {
+process.on('message', (request: any) => {


Let's rename this file to processChild as opposed to threadChild.

mjesun · 2018-07-11T10:32:19Z

+      const workerOptions: WorkerOptions = {
+        forkOptions: options.forkOptions || {},
+        maxRetries: options.maxRetries || 3,
+        useNodeWorkersIfPossible: options.useNodeWorkersIfPossible,


Let's remove this and make it true always.

It makes sense as an option, doesn't it? Seems like an opt-in thing (beyond using a version of node where it's unflagged)

There is no real reason why you'd like to use child_process instead of worker_threads when the second one is available. According to our benchmarks, implementation using workers works ~ twice as fast

API stability, there might be behavioural differences. Upgrading my app from node 10 to 11 (or whenever they remove the flag) shouldn't change the underlying implementation IMO. The docs for the module says it uses multiple processes, and just silently not doing that, depending on node version, seems risky to me

FWIW I could get behind setting it to true by default so it's possible to opt-out.

Upgrading my app from node 10 to 11 (or whenever they remove the flag) shouldn't change the underlying implementation IMO

I think that underlying implementation is not something that should concern an average library user. Public interface stays completely the same (which is crucial for all library users), while "engine" is getting faster. Same happens with the language and other libraries like React (Fiber rewrite) etc.

The docs for the module says it uses multiple processes, and just silently not doing that, depending on node version, seems risky to me

I'm completely on the same page, but we're going to cover all that by specs so there should be no issues with unexpected regression bugs.

I think @mjesun can give you more insights.

Let's re-write the docs as "we'll choose the most-performant implementation that is possible". People can enforce one or another by explicitly passing the corresponding worker pool.

Sounds good to me!

mjesun · 2018-07-11T10:33:51Z

-        return;
-      }
+      const task = {onEnd, onStart, request};
+      const workerId = worker ? worker.getWorkerId() : undefined;


Let's unify the interface. Either we work with worker instances or with worker ids, but I see a mix in the API.

Done. We use workerIds everywhere up to the pool

mjesun · 2018-07-11T10:36:29Z

+
+    if (job.owner) {
+      this._queue[workerId] = job.next ? job.next : null;
+      return this.run(workerId);


Remove recursivity here and use a while loop to find the first job available. Don't lock by worker, lock by job.

SimenB · 2018-07-11T11:46:19Z

Ooooooh, nice!

SimenB

This is also missing docs, in addition to the WIP points highlighted in a comment 🙂

Excited about this!

SimenB · 2018-07-11T11:47:21Z

@@ -0,0 +1,33 @@
+/**
+ * Copyright (c) 2017-present, Facebook, Inc. All rights reserved.


18? I have to admit I don't know what these should say

Kureev · 2018-07-11T11:51:50Z

@SimenB you haven't seen the performance part yet 😉

P.S. Added "Usage docs" as a missing part. I think it should come along with this PR to avoid inconsistency

mjesun · 2018-07-11T10:42:58Z

+    if (workerId != null) {
+      if (this._queue[workerId]) {
+        this._last[workerId].next = task;
+        this._last[workerId] = task;


This line is common.

mjesun · 2018-07-11T10:51:08Z

+
+    this._workerPool.send(worker, job.request, job.onStart, onEnd);
+
+    job.owner = worker;


Convert owner to a boolean.

mjesun · 2018-07-11T10:51:29Z

+    const job = this._queue[workerId];
+    const worker = this._workerPool.getWorkers()[workerId];
+
+    if (!job) {


Convert job into an Array, as in https://github.com/facebook/jest/blob/master/packages/jest-worker/src/worker.js#L151.

mjesun · 2018-07-11T10:54:06Z

+
+    this.lock(workerId);
+
+    this._workerPool.send(worker, job.request, job.onStart, onEnd);


That's an example of API unification: let's use workerId.

mjesun · 2018-07-11T10:58:18Z

-      // $FlowFixMe: This has to be a dynamic require.
-      const child = require(workerPath);
+    this._cacheKeys = Object.create(null);
+    this._options = Object.assign({}, defaultFarmOptions, options);


Make sure to make all configuration non-optional from here down the line (e.g. your input says numWorkers: ?number, but the _options object passed here should not be optional (i.e. numWorkers: number).

mjesun · 2018-07-11T11:01:07Z

 import {
-  CHILD_MESSAGE_INITIALIZE,
  PARENT_MESSAGE_ERROR,
+  CHILD_MESSAGE_INITIALIZE,


Alphabetical order

mjesun · 2018-07-11T15:33:07Z

+
+    const onEnd = (error: ?Error, result: mixed, worker: WorkerInterface) => {
+      this.unlock(workerId);
+      job.onEnd(error, result, worker);


Can we swap this with the previous line? Conceptually sounds better to keep the worker locked while finishing, just in case onEnd decides to go wild.

mjesun · 2018-07-11T15:34:01Z

+  }
+
+  getNextJob(workerId: number): ?QueueChildMessage {
+    if (!this._queue[workerId]) {


Cache the entire queue in const queue = this._queue[workerId].

mjesun · 2018-07-11T15:35:07Z

+        break;
+      }
+      this._queue[workerId] = this._queue[workerId].next;
+    }


Can we rewrite this as https://github.com/facebook/jest/blob/master/packages/jest-worker/src/worker.js#L138-L144?

mjesun · 2018-07-11T15:35:59Z

+      const numOfWorkers = this._workerPool.getWorkers().length;
+      for (let i = 0; i < numOfWorkers; i++) {
+        const workerIdx = (this._offset + i) % numOfWorkers;
+        this.enqueue(task, workerIdx);


What I mean is to move the for loop into the parent implementation, where you will call enqueue always with a workerId. This way workerId won't be optional anymore. This will also allow you to remove getWorkers, because you won't need it to get the length anymore.

mjesun · 2018-07-11T15:37:38Z

+
+      this._workers[i] = worker;
+
+      // $FlowFixMe


Please add explanation to all FlowFixMes.

You can get rid of this ones by redefining at the top the result, as a variable (class methods are covariant in Flow). For instance, putting at the top: getStderr: () => BaseWorkerPool; would do the trick.

Anyway, I'm not sure why do we need to bind them all.

We actually don't need it anymore. Will remove them accordingly

SimenB · 2018-08-15T13:40:53Z

Any news on this one? I'm excited!

SimenB · 2018-09-15T14:53:39Z

@mjesun @Kureev ping 🙂 Would be awesome to land this. Currently landing some breaking changes, so this should be fine to land as part of that if needed

Kureev · 2018-09-24T09:51:31Z

Hi @SimenB! :) Sorry for being silent. I'm working in a different team now, might be someone would like to take it over? IIRC, it's only about tests now. cc @mjesun

SimenB · 2018-10-17T16:17:38Z

@mjesun @rubennorte anyone at FB working on this? If not, anyone able to say what works and what doesn't with this PR so we can land it?

rubennorte · 2018-10-17T16:39:26Z

@SimenB AFAIK no one is working on this right now. I might take a look in a few weeks but I don't think it's going to be ready for the next major. Feel free to take over from here if you want to work on this (or anyone else).

SimenB · 2018-10-17T17:42:03Z

Might look into it if you could say some about what's missing. Just getting CI green? More tests?

rickhanlonii · 2018-11-25T23:00:17Z

I merged in master and wrote tests for this on #7408

Separated the PRs to get a clean review 👍

SimenB · 2018-12-05T23:55:14Z

Landed

github-actions · 2021-05-12T05:14:17Z

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

Alexey Kureev added 7 commits July 9, 2018 14:27

Restructure workers (create a base class and inherit from it)

418c6f9

Clean up types, create missing interfaes and make Flow and Jest happy

Move child.js to workers folder

100c49f

Restructure base classes and make a working POC

f786cb6

Remove BaseWorker

2a1ac0e

Remove MessageChannel and cleanup super() calls

59ebd73

Use worker threads implementation if possible

9b1f19b

Restructure queues

bea945f

Kureev self-assigned this Jul 11, 2018

Kureev requested review from mjesun and rafeca July 11, 2018 10:11

facebook-github-bot added the cla signed label Jul 11, 2018

Kureev commented Jul 11, 2018

View reviewed changes

mjesun suggested changes Jul 11, 2018

View reviewed changes

SimenB reviewed Jul 11, 2018

View reviewed changes

Support experimental modules in jest-resolver

3e9ce02

mjesun reviewed Jul 11, 2018

View reviewed changes

Alexey Kureev added 6 commits July 11, 2018 13:48

Rename child.js to processChild.js

02dedef

Remove private properties from WorkerPoolInterface

5e8347b

Move common line outside of if-else

e60bc1f

Unify interface (use workerId) and remove recursion

b7470d6

Remove opt-out option for worker_threads in node 10.5+

092d700

Alphabetical import sorting

3947fcb

mjesun suggested changes Jul 11, 2018

View reviewed changes

Alexey Kureev added 4 commits July 11, 2018 17:11

Unlock worker after onEnd

c715f00

Cache queue head in the getNextJob loop

4614e54

Elegant while loop

e0bfb83

Remove redundand .binds

9958587

Clean up interfaces and responsibilites

29e04d2

Kureev force-pushed the feature/T31135461 branch from 1ad9aa5 to 29e04d2 Compare July 12, 2018 11:01

SimenB added this to the Jest 24 milestone Oct 16, 2018

rickhanlonii mentioned this pull request Nov 25, 2018

Add node worker-thread support to jest-worker #7408

Merged

SimenB closed this Dec 5, 2018

SimenB mentioned this pull request Jan 22, 2019

Add Jest 24 blog post #7670

Merged

github-actions Bot locked as resolved and limited conversation to collaborators May 12, 2021

		@@ -0,0 +1,33 @@
		/**
		* Copyright (c) 2017-present, Facebook, Inc. All rights reserved.


		this._workerPool.send(worker, job.request, job.onStart, onEnd);

		job.owner = worker;


		this.lock(workerId);

		this._workerPool.send(worker, job.request, job.onStart, onEnd);

Uh oh!

Conversation

Kureev commented Jul 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Performance

Test plan

Uh oh!

Kureev commented Jul 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Kureev Jul 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SimenB commented Jul 11, 2018

Uh oh!

SimenB left a comment

Choose a reason for hiding this comment

Uh oh!

SimenB Jul 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Kureev commented Jul 11, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Kureev commented Jul 11, 2018 •

edited

Loading

Kureev commented Jul 11, 2018 •

edited

Loading

Kureev Jul 11, 2018 •

edited

Loading

SimenB Jul 11, 2018 •

edited

Loading