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

Use v8.serialize when available. #5565

Merged
merged 1 commit into from
Feb 14, 2018
Merged

Use v8.serialize when available. #5565

merged 1 commit into from
Feb 14, 2018

Conversation

cpojer
Copy link
Member

@cpojer cpojer commented Feb 14, 2018

Summary

v8 (in Node 8+) has a new function called v8.serialize which can be used to encode/decode JavaScript data structures faster than JSON. This diff changes jest-haste-map to use the new function in newer versions of Node and falls back to JSON if the serializer/deserializer aren't available. It also does a v8 version check to ensure we aren't trying to read a possible outdated binary blob. Making the cache work across node versions is a non goal and will be broken by this feature. If a user upgrades node, the function will throw and recreate the haste map.

I benchmarked this on a 73mb haste map and got the following results averaged over 10 runs:

  • v8.deserialize: 1621.34
  • JSON.parse: 1810.15
  • v8.serialize: 608.28
  • JSON.stringify: 1177.84

Reading is ~200ms faster and writing is 500ms faster. I suspect this is because of less validation work and less GC (edit: it's not GC). Since during startup we read the haste map once, write updates and then read it once per worker, this means that on a gigantic repo, this saves 500+200+(200*workers) time, or close to a second of actual time. On most repos out there in the world, there is likely no visible performance difference.

Next steps (up for grabs):

  • Create a jest-serialize package that uses either v8 serialize or JSON.
  • Use jest-serialize across Jest (jest-worker, jest-haste-map) and Metro.
  • Use more efficient data structures for Jest's haste map to save space and time (this is a large project).

Test plan

I updated some of the tests to pass on either node 8+ or node versions below that.

@orta
Copy link
Member

orta commented Feb 14, 2018

very cool

@codecov-io
Copy link

codecov-io commented Feb 14, 2018

Codecov Report

Merging #5565 into master will increase coverage by 0.08%.
The diff coverage is 97.14%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5565      +/-   ##
==========================================
+ Coverage   61.67%   61.75%   +0.08%     
==========================================
  Files         213      213              
  Lines        7160     7165       +5     
  Branches        4        4              
==========================================
+ Hits         4416     4425       +9     
+ Misses       2743     2739       -4     
  Partials        1        1
Impacted Files Coverage Δ
packages/jest-haste-map/src/index.js 96.3% <97.14%> (+0.11%) ⬆️
packages/jest-cli/src/watch.js 73.52% <0%> (+1.06%) ⬆️
packages/jest-cli/src/plugins/quit.js 33.33% <0%> (+13.33%) ⬆️

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 4ca959b...a706549. Read the comment docs.

if (version !== process.versions.v8) {
throw new Error('jest-haste-map: v8 versions do not match.');
}
return hasteMap;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Setting null prototype is not necessary with v8.deserialize?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, that doesn't meet my expectation:

v8.deserialize(v8.serialize(Object.create(null))).__proto__ === Object.create(null).__proto__  // false

fix incoming

@thymikee
Copy link
Collaborator

thymikee commented Feb 14, 2018

Anyway, would be nice to extract this to a helper/module one day :)
EDIT: haha, I didn't see it's already in the description 😅

@cpojer cpojer requested a review from mjesun February 14, 2018 14:49
fs.readFileSync(this._cachePath),
);
if (version !== process.versions.v8) {
throw new Error('jest-haste-map: v8 versions do not match.');
Copy link
Contributor

Choose a reason for hiding this comment

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

We should be smarter here, like purging the cache and keep it going, but for the moment it's OK.

Copy link
Member Author

Choose a reason for hiding this comment

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

It does already do that. Exceptions from this function are caught in the parent and a new map is created which will overwrite this one. For all intents and purposes, this error message is not user visible at this time.

Copy link
Contributor

Choose a reason for hiding this comment

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

❤️

@mjesun
Copy link
Contributor

mjesun commented Feb 14, 2018

@thymikee Yes, we've got to work on that soon; there are other parts where we could hugely benefit from this, like jest-worker, or even metro. Not only because serialization is faster, but also because it allows to simplify the way data is sent, and it supports Date, RegExp, circular references, etc.

Exciting times ahead!

@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