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

[jest-haste-map] reduce the number of lstat calls in node crawler #9514

Merged
merged 9 commits into from
Feb 5, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@

### Performance

- `[jest-haste-map]` Reduce number of `lstat` calls in node crawler ([#9514](https://github.com/facebook/jest/pull/9514))

## 25.1.0

### Features
Expand Down
132 changes: 124 additions & 8 deletions packages/jest-haste-map/src/crawlers/__tests__/node.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,8 @@ jest.mock('child_process', () => ({
}),
}));

let mockHasReaddirWithFileTypesSupport = false;

jest.mock('fs', () => {
let mtime = 32;
const size = 42;
Expand All @@ -42,7 +44,7 @@ jest.mock('fs', () => {
return path.endsWith('/directory');
},
isSymbolicLink() {
return false;
return path.endsWith('symlink');
},
mtime: {
getTime() {
Expand All @@ -56,13 +58,66 @@ jest.mock('fs', () => {
};
return {
lstat: jest.fn(stat),
readdir: jest.fn((dir, callback) => {
if (dir === '/project/fruits') {
setTimeout(() => callback(null, ['directory', 'tomato.js']), 0);
} else if (dir === '/project/fruits/directory') {
setTimeout(() => callback(null, ['strawberry.js']), 0);
} else if (dir == '/error') {
setTimeout(() => callback({code: 'ENOTDIR'}, undefined), 0);
readdir: jest.fn((dir, options, callback) => {
// readdir has an optional `options` arg that's in the middle of the args list.
// we always provide it in practice, but let's try to handle the case where it's not
// provided too
if (typeof callback === 'undefined') {
if (typeof options === 'function') {
callback = options;
}
throw new Error('readdir: callback is not a function!');
}

if (mockHasReaddirWithFileTypesSupport) {
if (dir === '/project/fruits') {
setTimeout(
() =>
callback(null, [
{
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In case you're wondering - I mirrored the existing mocks to look like a fake fs.Dirent object here. I thought about coming up with something fancy to avoid this duplication (e.g defining the mocks in a map and then reduceing it to get strings/dirents as needed) but it seemed more complicated than it was worth.

Copy link
Member

Choose a reason for hiding this comment

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

yeah, let's try to keep it simple here

isDirectory: () => true,
isSymbolicLink: () => false,
name: 'directory',
},
{
isDirectory: () => false,
isSymbolicLink: () => false,
name: 'tomato.js',
},
{
isDirectory: () => false,
isSymbolicLink: () => true,
name: 'symlink',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also noticed that there weren't any existing tests for symlinks that I could find, so I added one here. It doesn't change the result of any existing tests (since we ignore symlinks) but will affect the lstat call-counting tests I added.

Copy link
Member

Choose a reason for hiding this comment

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

very nice, thank you! we'll hopefully be landing some symlink support soonish, so these tests should come in handy

},
]),
0,
);
} else if (dir === '/project/fruits/directory') {
setTimeout(
() =>
callback(null, [
{
isDirectory: () => false,
isSymbolicLink: () => false,
name: 'strawberry.js',
},
]),
0,
);
} else if (dir == '/error') {
setTimeout(() => callback({code: 'ENOTDIR'}, undefined), 0);
}
} else {
if (dir === '/project/fruits') {
setTimeout(
() => callback(null, ['directory', 'tomato.js', 'symlink']),
0,
);
} else if (dir === '/project/fruits/directory') {
setTimeout(() => callback(null, ['strawberry.js']), 0);
} else if (dir == '/error') {
setTimeout(() => callback({code: 'ENOTDIR'}, undefined), 0);
}
}
}),
stat: jest.fn(stat),
Expand Down Expand Up @@ -296,4 +351,65 @@ describe('node crawler', () => {
expect(removedFiles).toEqual(new Map());
});
});

describe('readdir withFileTypes support', () => {
it('calls lstat for directories and symlinks if readdir withFileTypes is not supported', () => {
nodeCrawl = require('../node');
const fs = require('fs');

const files = new Map();
return nodeCrawl({
data: {files},
extensions: ['js'],
forceNodeFilesystemAPI: true,
ignore: pearMatcher,
rootDir,
roots: ['/project/fruits'],
}).then(({hasteMap, removedFiles}) => {
expect(hasteMap.files).toEqual(
createMap({
'fruits/directory/strawberry.js': ['', 33, 42, 0, '', null],
'fruits/tomato.js': ['', 32, 42, 0, '', null],
}),
);
expect(removedFiles).toEqual(new Map());
// once for /project/fruits, once for /project/fruits/directory
expect(fs.readdir).toHaveBeenCalledTimes(2);
// once for each of:
// 1. /project/fruits/directory
// 2. /project/fruits/directory/strawberry.js
// 3. /project/fruits/tomato.js
// 4. /project/fruits/symlink
// (we never call lstat on the root /project/fruits, since we know it's a directory)
expect(fs.lstat).toHaveBeenCalledTimes(4);
});
});
it('avoids calling lstat for directories and symlinks if readdir withFileTypes is supported', () => {
mockHasReaddirWithFileTypesSupport = true;
nodeCrawl = require('../node');
const fs = require('fs');

const files = new Map();
return nodeCrawl({
data: {files},
extensions: ['js'],
forceNodeFilesystemAPI: true,
ignore: pearMatcher,
rootDir,
roots: ['/project/fruits'],
}).then(({hasteMap, removedFiles}) => {
expect(hasteMap.files).toEqual(
createMap({
'fruits/directory/strawberry.js': ['', 33, 42, 0, '', null],
'fruits/tomato.js': ['', 32, 42, 0, '', null],
}),
);
expect(removedFiles).toEqual(new Map());
// once for /project/fruits, once for /project/fruits/directory
expect(fs.readdir).toHaveBeenCalledTimes(2);
// once for strawberry.js, once for tomato.js
expect(fs.lstat).toHaveBeenCalledTimes(2);
});
});
});
});
27 changes: 24 additions & 3 deletions packages/jest-haste-map/src/crawlers/node.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,22 +32,42 @@ function find(

function search(directory: string): void {
activeCalls++;
fs.readdir(directory, (err, names) => {
fs.readdir(directory, {withFileTypes: true}, (err, entries) => {
activeCalls--;
if (err) {
callback(result);
return;
}
names.forEach(file => {
file = path.join(directory, file);
// node < v10.10 does not support the withFileTypes option, and
// entry will be a string.
entries.forEach((entry: string | fs.Dirent) => {
const file = path.join(
directory,
typeof entry === 'string' ? entry : entry.name,
);

if (ignore(file)) {
return;
}

if (typeof entry !== 'string') {
if (entry.isSymbolicLink()) {
return;
}

if (entry.isDirectory()) {
search(file);
return;
}
}
SimenB marked this conversation as resolved.
Show resolved Hide resolved

activeCalls++;

fs.lstat(file, (err, stat) => {
activeCalls--;

// This logic is unnecessary for node > v10.10, but leaving it in
// since we need it for backwards-compatibility still.
Comment on lines +69 to +70
Copy link
Member

@SimenB SimenB Feb 5, 2022

Choose a reason for hiding this comment

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

@yepitschunked we're about to drop node 10 (#12220), which part of the logic does this refer to? The entire stat or just specific parts of it?

Copy link
Member

Choose a reason for hiding this comment

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

this PR is old, but would love your comment over in that PR 🙂

if (!err && stat && !stat.isSymbolicLink()) {
if (stat.isDirectory()) {
search(file);
Expand All @@ -58,6 +78,7 @@ function find(
}
}
}

if (activeCalls === 0) {
callback(result);
}
Expand Down