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

Performance: Cache regular expression instead of creating anew for every file in ScriptTransformer. #8235

Merged

Conversation

scotthovestadt
Copy link
Contributor

Summary

This PR improves performance by caching the regular expression string -> RegExp conversion.

Benchmarks on cached regular expression vs. creating new each time:

  new regex x   596 ops/sec ±0.95% (97 runs sampled)
  cached regex  x 1,668 ops/sec ±0.61% (97 runs sampled)

For large projects, Jest may process a LOT of files. The more workers in use, the higher chance that the same file is processed multiple times. Additionally, each time it's hit multiple regular expressions may be tested against.

The benchmark I used:

suite.add('new regex', function() {
  for (var i = 0; i < 10000; i++) {
    new RegExp('^.+\\.[jt]sx?$').test(
      'jest/packages/jest-haste-map/src/__tests__/haste_impl.js'
    );
  }
});

suite.add('cached regex', function() {
  const regex = new RegExp('^.+\\.[jt]sx?$');
  for (var i = 0; i < 10000; i++) {
    regex.test(
      'jest/packages/jest-haste-map/src/__tests__/haste_impl.js'
    );
  }
});

Test plan

  • All tests pass.
  • Performance benefit verified.

@codecov-io
Copy link

codecov-io commented Mar 29, 2019

Codecov Report

Merging #8235 into master will increase coverage by 0.01%.
The diff coverage is 80.95%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #8235      +/-   ##
==========================================
+ Coverage   62.34%   62.36%   +0.01%     
==========================================
  Files         265      265              
  Lines       10559    10569      +10     
  Branches     2568     2570       +2     
==========================================
+ Hits         6583     6591       +8     
- Misses       3387     3388       +1     
- Partials      589      590       +1
Impacted Files Coverage Δ
packages/jest-transform/src/ScriptTransformer.ts 80.76% <80.95%> (-0.05%) ⬇️

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 384a0d9...6107b50. Read the comment docs.

Copy link
Collaborator

@thymikee thymikee left a comment

Choose a reason for hiding this comment

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

Looks good!

packages/jest-transform/src/ScriptTransformer.ts Outdated Show resolved Hide resolved
if (new RegExp(this._config.transform[i][0]).test(filename)) {
return this._config.transform[i][1];
const transformRegExp = this._cache.transformRegExp;
if (!transformRegExp) {
Copy link
Member

Choose a reason for hiding this comment

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

When can this be falsy?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When the array passed from config with transforms is 0-length.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, transform: {}? Gotcha

Copy link
Contributor Author

Choose a reason for hiding this comment

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

calcTransformRegExp returns undefined if config.transform.length === 0.

I try not to return empty arrays to loop through when the function should actually be telling you "nothing to do here". I personally find it makes the code more readable and sometimes make optimizations in the future possible.

@scotthovestadt scotthovestadt merged commit a233361 into jestjs:master Mar 29, 2019
if (
!config.transformIgnorePatterns ||
config.transformIgnorePatterns.length === 0
) {
return null;
Copy link
Member

Choose a reason for hiding this comment

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

this changed introduced a lint warning

image

@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 11, 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