Make jest-worker use jest-serializer by mjesun · Pull Request #5613 · jestjs/jest

mjesun · 2018-02-19T15:38:57Z

This PR implements jest-serializer as a new layer for data transmission between the parent process and all the child ones. Since IPC is a custom Node layer, we re-implemented the data transmission as well to work directly with binary buffers, using an extra file descriptor (number 4).

The Comms class wraps around the fd Socket, and provides an almost 1:1 compatible interface with process, providing a send method and a message event. This way, putting the Comms instance was relatively easy.

Tests cover all Comms class, plus some modification to the original jest-worker tests so that they keep passing.

thymikee · 2018-02-19T18:13:32Z

+ * saved until there is enough information to parse a payload, then a "message"
+ * event is emitted.
+ */
+export default class extends EventEmitter {


How about renaming it Communications? I guess that was the original intent, but shortened.

Comms seems pretty weird, yeah. How about "Channel"?

Yeah, now that I read Channel, I prefer it rather than Communications. Changing it again 😅

thymikee · 2018-02-21T13:11:40Z

+  constructor(stream: Duplex) {
+    super();
+
+    this._size = Buffer.allocUnsafe(4);


@mjesun This is v5+ API, so technically we break Node 4 compat :/

Should probably use https://github.com/LinusU/buffer-alloc-unsafe or something, then. Would be great to keep around node 4 support (unofficial though it is) until it's EOL in a couple of months

mjesun · 2018-02-22T11:13:52Z

I'm still investigating how to merge this (if we are going to merge it at all). The internal IPC implementation seems to bypass a bunch of Node layers (e.g. writes directly into the socket via writeUtf8String), and it's quite hard to make a Buffer implementation as fast as theirs.

Performance results using __performance_tests__/test.js showed a 7% regression compared to worker-farm (so approx. 13-14% compared to the current jest-worker version, since that one is 6-7% faster than worker-farm).

The fact that v8.serialize is slower for simple, small objects compared to JSON.stringify does not help either 😞

mjesun · 2018-03-15T02:15:20Z

Some good news: after #5793, there might still be a chance for this PR to merge. I will rebase and re-measure, see how that goes. This is actually something that would benefit Jest, Metro and potentially other OSS projects.

cc / @rafeca @BYK for the Buffer transport in Metro.

mjesun · 2018-03-15T03:05:27Z

TL;DR: Using v8.serialize on small payloads has a huge performance cost (from 2 to 3x slower), but it can even make jest-worker slower than worker-farm.

Command `node --expose-gc test.js loadTest 10000`:

Before the change:

jest-worker: { globalTime: 758, processingTime: 715 }
jest-worker: { globalTime: 851, processingTime: 826 }
jest-worker: { globalTime: 824, processingTime: 800 }

After the change:

jest-worker: { globalTime: 1285, processingTime: 1233 }
jest-worker: { globalTime: 1025, processingTime: 986 }
jest-worker: { globalTime: 978, processingTime: 951 }

Command `node --expose-gc test.js empty 100000`:

Before the change:

jest-worker: { globalTime: 2501, processingTime: 2463 }
jest-worker: { globalTime: 2442, processingTime: 2413 }
jest-worker: { globalTime: 2502, processingTime: 2469 }

After the change:

jest-worker: { globalTime: 6525, processingTime: 6471 }
jest-worker: { globalTime: 6816, processingTime: 6783 }
jest-worker: { globalTime: 6552, processingTime: 6520 }

codecov-io · 2018-03-15T03:13:34Z

Codecov Report

Merging #5613 into master will increase coverage by 0.16%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #5613      +/-   ##
==========================================
+ Coverage   63.73%   63.89%   +0.16%     
==========================================
  Files         216      217       +1     
  Lines        7938     7969      +31     
  Branches        3        3              
==========================================
+ Hits         5059     5092      +33     
+ Misses       2878     2876       -2     
  Partials        1        1

Impacted Files	Coverage Δ
packages/jest-worker/src/channel.js	`100% <100%> (ø)`
packages/jest-worker/src/worker.js	`98.41% <100%> (+0.13%)`	⬆️
packages/jest-worker/src/child.js	`100% <100%> (ø)`	⬆️
packages/jest-serializer/src/index.js	`100% <0%> (+3.92%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update feeb371...784b002. Read the comment docs.

cpojer · 2018-03-15T15:04:27Z

So what do you wanna do with this PR?

cpojer · 2018-03-28T08:26:36Z

Ping @mjesun shall we close this for now?

mjesun · 2018-03-28T10:01:12Z

Yep, let's close it for the moment. A follow up could be to be able to inject the serializer / deserializer; but I'm not going to do that ATM.

github-actions · 2021-05-12T15:07:09Z

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.

facebook-github-bot added the CLA Signed ✔️ label Feb 19, 2018

mjesun requested review from SimenB, cpojer and thymikee February 19, 2018 15:43

thymikee reviewed Feb 19, 2018

View reviewed changes

thymikee reviewed Feb 21, 2018

View reviewed changes

Miguel Jimenez Esun added 3 commits March 15, 2018 02:47

Make jest-worker use jest-serializer

d4e7021

Move "comms" to "channel"

dd110ef

Use v4 compatible API

784b002

jestjs deleted a comment from codecov-io Mar 15, 2018

mjesun closed this Mar 28, 2018

mjesun deleted the jest-worker-using-serializer branch May 15, 2018 14:16

SimenB mentioned this pull request May 27, 2018

Investigate moving away from Node's default IPC mechanism #4942

Closed

mjesun mentioned this pull request Oct 23, 2018

jest fails to generate coverage when using the testRegex config option #7242

Closed

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

Uh oh!

Uh oh!

Conversation

mjesun commented Feb 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thymikee Feb 19, 2018

Choose a reason for hiding this comment

Uh oh!

cpojer Feb 19, 2018

Choose a reason for hiding this comment

Uh oh!

mjesun Feb 19, 2018

Choose a reason for hiding this comment

Uh oh!

mjesun Feb 20, 2018

Choose a reason for hiding this comment

Uh oh!

thymikee Feb 21, 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!

SimenB Feb 21, 2018

Choose a reason for hiding this comment

Uh oh!

mjesun commented Feb 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mjesun commented Mar 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mjesun commented Mar 15, 2018

Command node --expose-gc test.js loadTest 10000:

Before the change:

After the change:

Command node --expose-gc test.js empty 100000:

Before the change:

After the change:

Uh oh!

codecov-io commented Mar 15, 2018

Codecov Report

Uh oh!

cpojer commented Mar 15, 2018

Uh oh!

cpojer commented Mar 28, 2018

Uh oh!

mjesun commented Mar 28, 2018

Uh oh!

github-actions Bot commented May 12, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

mjesun commented Feb 19, 2018 •

edited

Loading

thymikee Feb 21, 2018 •

edited

Loading

mjesun commented Feb 22, 2018 •

edited

Loading

mjesun commented Mar 15, 2018 •

edited

Loading

Command `node --expose-gc test.js loadTest 10000`:

Command `node --expose-gc test.js empty 100000`: