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

[jeysal]

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]

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

[jeysal]

And I know I know, changelog :D

[ahnpnl]

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

[ludofischer]

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

[jeysal]

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]

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

[jeysal]

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

[SimenB]

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]

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]

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]

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]

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]

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]

Anyway, pushed the createRequire change

[jeysal]

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]

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.