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

Improve process object creation #4955

Merged
merged 1 commit into from
Nov 25, 2017
Merged

Improve process object creation #4955

merged 1 commit into from
Nov 25, 2017

Conversation

mjesun
Copy link
Contributor

@mjesun mjesun commented Nov 25, 2017

This PR improves the way the process object is created in two ways:

  1. It aims to rewrite the way deep cyclic cloning is done:
    1. It uses a specialized method for Arrays.
    2. It copies properties by using getOwnPropertyDescriptors and defineProperties.
    3. It gives a blacklist argument for the root level object.
  2. Using the blacklist, it avoids copying mainModule, which contains a reference to the main module, plus all of the other modules loaded so far in memory.

The total improvement on a raw Node instance is approximately of 40%; but the improvement grows as the amount of previously loaded modules also grows. In my case, I've been able to observe up to 20x performance by artificially loading a bunch of modules before calling createProcessObject.

@mjesun
Copy link
Contributor Author

mjesun commented Nov 25, 2017

Infinite amount of thanks to @thymikee who identified the offending commit by performing a manual bisection!

@codecov-io
Copy link

Codecov Report

Merging #4955 into master will decrease coverage by 0.05%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4955      +/-   ##
==========================================
- Coverage   60.34%   60.29%   -0.06%     
==========================================
  Files         198      198              
  Lines        6604     6598       -6     
  Branches        4        3       -1     
==========================================
- Hits         3985     3978       -7     
- Misses       2619     2620       +1
Impacted Files Coverage Δ
packages/jest-util/src/create_process_object.js 100% <100%> (ø) ⬆️
packages/jest-util/src/deep_cyclic_copy.js 100% <100%> (ø) ⬆️
packages/jest-runtime/src/script_transformer.js 85.35% <0%> (-1.16%) ⬇️
packages/jest-util/src/install_common_globals.js 100% <0%> (ø) ⬆️

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 397acea...fad2df8. Read the comment docs.

@thymikee
Copy link
Collaborator

Can confirm it's back to the nominal speed on my testing suite 👍

@cpojer cpojer merged commit 4883838 into jestjs:master Nov 25, 2017
@cpojer
Copy link
Member

cpojer commented Nov 25, 2017

Thanks for fixing this issue. I would prefer having a whitelist rather than a blacklist. If the node folks decide to add process.allModules = require.cache we have to be pro-active and fix it, otherwise people on old versions of Jest and new versions of node get a dramatic slowdown :(

@mjesun mjesun deleted the improve-process-creation branch December 6, 2017 11:46
vzvu3k6k added a commit to vzvu3k6k/gitify that referenced this pull request Oct 21, 2018
vzvu3k6k added a commit to vzvu3k6k/gitify that referenced this pull request Oct 21, 2018
mainModule is blacklisted by jestjs/jest#4955 and require.main
is provided by jestjs/jest#5618, but require.main can't be
accessed on electron.
@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 13, 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.

5 participants