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

perf(jest-runtime): load chalk only once per worker #10864

Merged
merged 3 commits into from Feb 11, 2021
Merged

Conversation

jeysal
Copy link
Contributor

@jeysal jeysal commented Nov 24, 2020

Summary

chalk is imported so many times, in so many util or whatever dependencies, that it's pratically impossible to trace and maintain which imports need to use the 'require outside' option. This is a simple alternative for such cases. Feels a little sketchy adding such a hardcoded list to jest-runtime, but since it's just for perf and can theoretically be removed at any time without changing behavior, I think it's fine.

Test plan

What it does is tested, but I'm thinking I should probably add some perf tests? At least in the 'manual form' that we have e.g. in jest-worker, so that my results are reproducible.
While #7792 gave ~50% per-test file speedup on Windows, this will give another ~15% on top, bringing it to a total of ~70% per-test file speedup over 26.6.3.

@jeysal
Copy link
Contributor Author

jeysal commented Nov 24, 2020

See also comments like this in one of our perf-related issues.

@jeysal
Copy link
Contributor Author

jeysal commented Nov 25, 2020

And I know I know, changelog :D

@ahnpnl
Copy link
Contributor

ahnpnl commented Nov 25, 2020

I think in the future, benchmark test should be introduced for e2e

@ludofischer
Copy link

Would it make sense to move logging to a dedicated worker, eventually?

@jeysal jeysal force-pushed the perf branch 2 times, most recently from 7f25c06 to 49a5ac9 Compare February 9, 2021 20:58
@jeysal
Copy link
Contributor Author

jeysal commented Feb 9, 2021

Seems like we'll need to drop Node 10 for this then, because of createRequire support? How do we do this elsewhere, do we never createRequire? o.O

@jeysal
Copy link
Contributor Author

jeysal commented Feb 9, 2021

Other than that it should be good to go as is now, wdyt @SimenB

@jeysal
Copy link
Contributor Author

jeysal commented Feb 9, 2021

RIP Windows (and possibly Mac?) users when checking out these 500 new files 😂

@SimenB
Copy link
Member

SimenB commented Feb 10, 2021

Seems like we'll need to drop Node 10 for this then, because of createRequire support? How do we do this elsewhere, do we never createRequire? o.O

Node 10 has https://nodejs.org/docs/latest-v10.x/api/modules.html#modules_module_createrequirefrompath_filename, you can check which exists before using

@SimenB
Copy link
Member

SimenB commented Feb 10, 2021

RIP Windows (and possibly Mac?) users when checking out these 500 new files 😂

Can we generate the files in the test and not check them in? Regardless of diff size, it just seems overkill 😛

@merceyz
Copy link
Contributor

merceyz commented Feb 10, 2021

Seems like we'll need to drop Node 10 for this then, because of createRequire support? How do we do this elsewhere, do we never createRequire? o.O

@jeysal Wouldn't require(require.resolve(to, {paths: [from]}) work?

Node 10 has https://nodejs.org/docs/latest-v10.x/api/modules.html#modules_module_createrequirefrompath_filename, you can check which exists before using

The polyfill is also really simple if you need to go lower than that https://github.com/nuxt-contrib/create-require/blob/420682d526a8cbe08df4d2796f4be627d02b1ffd/create-require.js#L30-L37

@jeysal jeysal added this to the Jest 27 milestone Feb 10, 2021
@jeysal
Copy link
Contributor Author

jeysal commented Feb 10, 2021

Node 10 has nodejs.org/docs/latest-v10.x/api/modules.html#modules_module_createrequirefrompath_filename, you can check which exists before using

Meh, knew about that one but thought that was a shit solution - let's see when 27 actually happens, if it's close to 30 Apr we might as well release on 1 May and stick the 'drop Node 10' change in there as well so we don't have Node 10 around for another year until the next major 😅

@jeysal Wouldn't require(require.resolve(to, {paths: [from]}) work?

I'd say likely yes. It'll definitely always resolve the path properly, and I'd expect given an absolute require path PnP (or any other resolution algo) wouldn't do anything special.

Can we generate the files in the test and not check them in? Regardless of diff size, it just seems overkill

I'll add a script that generates them and gitignore them. Although TBH this thing will be barely worth more than a readme at this point. The benchmark already doesn't include a runner script (because others like hyperfine are just way better at this already), and creating a package.json plus test file somewhere, then for i in {1..499}; do cp 0.test.js $i.test.js; done, takes 1 minute 😅

@SimenB
Copy link
Member

SimenB commented Feb 10, 2021

Meh, knew about that one but thought that was a shit solution

const Module = require('module')
const createRequire = Module.createRequire || Module.createRequireFromPath;

Doesn't seem too 💩

I deffo want 27 to support node 10, lots of stuff in this release it'd be nice to make available to people stuck on slightly older versions

@jeysal
Copy link
Contributor Author

jeysal commented Feb 10, 2021

Yeah it's not much code, was more about introducing code that we don't need anymore in three months 😅
Meh, not excited about continuing Node 10 support with all workaround branches in versions released after its LTS ends 🤷

@jeysal
Copy link
Contributor Author

jeysal commented Feb 10, 2021

Anyway, pushed the createRequire change

@SimenB SimenB merged commit 42f78d4 into jestjs:master Feb 11, 2021
@jeysal
Copy link
Contributor Author

jeysal commented Feb 16, 2021

Yeah so I ran a benchmark for 26 vs 27 with #7792 and this on the Babel repo for a more realistic example, and as expected the impact of this on most real projects with real tests (and usually not just one small one per file) will be quite small, on Babel e.g. it's 1.04 ± 0.00 times faster. At least I got consistent test run times though (that's why it's ± 0.00), so it's not just random deviation, but measurable.

@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 10, 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.

None yet

6 participants