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

Cache checked files in fs #8388

Merged
merged 6 commits into from May 1, 2019

Conversation

@Connormiha
Copy link
Contributor

Connormiha commented Apr 28, 2019

Summary

For the same path, the function fs.statSync can call many times. The more test files, the more often it will be called for the same file or directory.
I just added hash table to avoid repeated call for fs.statSync.

Test plan

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Apr 28, 2019

Codecov Report

Merging #8388 into master will increase coverage by 0.02%.
The diff coverage is 88.23%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #8388      +/-   ##
==========================================
+ Coverage   62.34%   62.37%   +0.02%     
==========================================
  Files         266      266              
  Lines       10734    10739       +5     
  Branches     2610     2613       +3     
==========================================
+ Hits         6692     6698       +6     
+ Misses       3460     3459       -1     
  Partials      582      582
Impacted Files Coverage Δ
packages/jest-resolve/src/defaultResolver.ts 68.11% <88.23%> (+4.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 ff9985c...4bf5f36. Read the comment docs.

Copy link
Collaborator

thymikee left a comment

Sounds reasonable. Can you update the changleog?
Btw, do you happen to have any perf measurements on whether it improves stuff?

@jeysal

This comment has been minimized.

Copy link
Collaborator

jeysal commented Apr 28, 2019

I also think some perf numbers would be great. Caches (esp. those that are never invalidated) can cause problems, so I think we should only introduce this extra complexity for a good reason.

@Connormiha

This comment has been minimized.

Copy link
Contributor Author

Connormiha commented Apr 28, 2019

I created some function performer for testing isFile function.

function perfDecorator(fn) {
  const results = [];
  function printResult() {
    console.log('Perf result start:');
    console.log('Total calls:', results.length, ' ');
    console.log('Min time:', Math.min(...results));
    console.log('Max time:', Math.max(...results));
    console.log(
      'Average time:',
      results.reduce((acc, next) => acc + next, 0) / results.length,
    );
    console.log(
      'Sum time:',
      results.reduce((acc, next) => acc + next, 0),
    );
    console.log('Uniq paths:', typeof checkedPaths === 'Object' ? Object.keys(checkedPaths).length) : '';
    console.log('Perf result end');
  }
  return function(path) {
    let time = process.hrtime()[1];
    const fnResult = fn(path);
    time = process.hrtime()[1] - time;
    results.push(time);
    if (results.length === 4500) {
        printResult();
    }
    return fnResult;
  };
}

isFile = perfDecorator(isFile);

I tested it in my some project.
Total calls isFile: 4500
Uniq paths: 3697
About 20% calsl got from cache.

Before

Perf result start:
Total calls: 4500  
Min time: 7804
Max time: 5951437
Average time: 54111.720888888885
Sum time: 243502744
Perf result end

After

Perf result start:
Total calls: 4500  
Min time: 543
Max time: 5578729
Average time: 37700.06844444444
Sum time: 169650308
Perf result end

@thymikee

@StringEpsilon

This comment has been minimized.

Copy link
Contributor

StringEpsilon commented Apr 29, 2019

Not exactly scientific, but I tested this patch against our test suite with some promising results. Thought you might appreciate a real world report. Each run is just npx jest over the full project.

run current patched
1 57.8 s 52.5 s
2 56.1 s 49.0 s
3 55.5 s 53.0 s
4 57.9 s 49.8 s
5 56.9 s 48.8 s
avg 56.8 s 50.6 s

I could not detect any regressions for single-file test suites with the time reported by jest.

@SimenB

This comment has been minimized.

Copy link
Collaborator

SimenB commented Apr 29, 2019

Woah, that's great!

Only thing I'm worried about is if the path starts pointing to a different file or it's a file instead of a directory - do we do any cache invalidation here?

@StringEpsilon

This comment has been minimized.

Copy link
Contributor

StringEpsilon commented Apr 29, 2019

I'd think that cache invalidation is mostly a problem in watch mode. You could simply bypass the caching completely in watch mode for an easy fix.

Apart from watch mode, the only other sources of file changes during runtime I could think of are:

  1. Newly generated snapshot files
  2. Jest internal cache files
  3. If a user changes something while jest is running.

I tested the first scenario by deleting and re-populating all our snapshots (>200) without any problems. The created files match perfectly according to git. With respect to number 3, jest is already running into problems with weird errors, so I don't think this PR matters in that respect. Though it might make future improvements harder.

With respect to number 2, you know better if jest-resolve is even used in those cases and what the mechanism for cache invalidation should look like.

@SimenB

This comment has been minimized.

Copy link
Collaborator

SimenB commented Apr 30, 2019

It's watch mode I was thinking of, yes. Maybe we could just clear the map when starting a new test run?

@jeysal

This comment has been minimized.

Copy link
Collaborator

jeysal commented Apr 30, 2019

Should the cache perhaps be in Resolver itself instead of the default resolver impl? That would probably make resetting a lot cleaner as well.

@jeysal

This comment has been minimized.

Copy link
Collaborator

jeysal commented Apr 30, 2019

BTW testing this on a company React app test suite showed about 8% reduction in total time. Pretty good 👍

@Connormiha

This comment has been minimized.

Copy link
Contributor Author

Connormiha commented Apr 30, 2019

It's watch mode I was thinking of, yes. Maybe we could just clear the map when starting a new test run?

I tried run with --watch mode. This patch doesn't effect it because defaultResolver re-required after change files in project. So map refreshed. You can check it just print console.log(checkedPaths.size) inside module. Well, or I do not understand something ((.

const checkedPaths = new Map<string, number>();
console.log('We are here and we have clear Map', checkedPaths.size);

And after run test with --watch, and after try to change your tests.
@SimenB

Copy link
Contributor

scotthovestadt left a comment

I love this. I'm going to verify myself that the invalidation is working in watch mode. If we can swap out those values to an enum, we can get this in the release this week!

packages/jest-resolve/src/defaultResolver.ts Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@SimenB
SimenB approved these changes May 1, 2019
@Connormiha Connormiha force-pushed the Connormiha:optimize-modules-paths branch from ddea541 to 4bf5f36 May 1, 2019
*/
function isFile(file: Config.Path): boolean {
let result;
enum IPathType {

This comment has been minimized.

Copy link
@scotthovestadt

scotthovestadt May 1, 2019

Contributor

It's not a big deal, but FYI-- not using the "prefix with I" convention in the Jest codebase.

@scotthovestadt scotthovestadt merged commit a9c0e6b into facebook:master May 1, 2019
10 of 11 checks passed
10 of 11 checks passed
facebook.jest #20190501.3 failed
Details
ci/circleci: lint-and-typecheck Your tests passed on CircleCI!
Details
ci/circleci: test-browser Your tests passed on CircleCI!
Details
ci/circleci: test-jest-circus Your tests passed on CircleCI!
Details
ci/circleci: test-node-10 Your tests passed on CircleCI!
Details
ci/circleci: test-node-11 Your tests passed on CircleCI!
Details
ci/circleci: test-node-6 Your tests passed on CircleCI!
Details
ci/circleci: test-node-8 Your tests passed on CircleCI!
Details
ci/circleci: test-or-deploy-website Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
deploy/netlify Deploy preview ready!
Details
@ArtemGovorov

This comment has been minimized.

Copy link
Contributor

ArtemGovorov commented May 14, 2019

@scotthovestadt @Connormiha The change is breaking Jest watch mode. Here is a simple test:

  • start a simple failing test that imports an inexistent module in watch mode,
  • add the module while in watch mode,
  • the test is still failing:

ji

aliaksandr-yermalayeu added a commit to aliaksandr-yermalayeu/jest that referenced this pull request Jul 16, 2019
* Cache checked files in fs

* Cache checked dirs in fs

* Refactoring isfile

* Add to change.md

* Use unem instead of const

* Update changelog
@Connormiha

This comment has been minimized.

Copy link
Contributor Author

Connormiha commented Aug 19, 2019

@ArtemGovorov Patch arrived with 24.9.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
9 participants
You can’t perform that action at this time.