Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make jest-worker use jest-serializer #5613

Closed
wants to merge 3 commits into from
Closed

Make jest-worker use jest-serializer #5613

wants to merge 3 commits into from

Conversation

mjesun
Copy link
Contributor

@mjesun mjesun commented Feb 19, 2018

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.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done! 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

constructor(stream: Duplex) {
super();

this._size = Buffer.allocUnsafe(4);
Copy link
Collaborator

@thymikee thymikee Feb 21, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

mjesun commented Feb 22, 2018

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
Copy link
Contributor Author

mjesun commented Mar 15, 2018

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
Copy link
Contributor Author

mjesun commented Mar 15, 2018

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 }

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

Codecov Report

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

Impacted file tree graph

@@            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
Copy link
Member

cpojer commented Mar 15, 2018

So what do you wanna do with this PR?

@cpojer
Copy link
Member

cpojer commented Mar 28, 2018

Ping @mjesun shall we close this for now?

@mjesun
Copy link
Contributor Author

mjesun commented Mar 28, 2018

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
Copy link

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants