Skip to content

Commit

Permalink
Resolver perf: Use directory existence to cut down candidate list (3/…
Browse files Browse the repository at this point in the history
…n) (#1333)

Summary:
Pull Request resolved: #1333

Further optimise resolution with some work avoidance by using the fact that, when resolving `require('foo')`, we can eliminate any candidates of the form `<prefix>/node_modules/foo<suffix>` unless `<prefix>/node_modules` exists and is a directory. This saves a lot of lookups from deeply-nested origin modules.

Further, when resolving `require('foo/bar')`, we can eliminate candidates of the form `<prefix>/node_modules/foo/<suffix>` unless `<prefix>/node_modules/foo` exists as a directory.

This is a bit more subtle than it might seem, and can't be extended beyond these cases, due to redirection. For `require('foo/bar/baz')`, `foo/bar` need not exist anywhere, because some `foo/package.json` could redirect it. However, when looking up a `package.json` for possible redirections, we stop at `node_modules` (we do not check `node_modules/package.json`), so  it's safe to treat anything up to and including `node_modules` (or `node_modules/foo` in the case `node_modules/foo.js` may not be a candidate) as a real file path that must be an extant directory.

This fix reduces the existence checks we need to perform in RNTester such that total resolution speed is improved ~3x, or ~12x relative to the start of this stack / the previous Metro release.

```
- **[Performance]**: Use directory existence to cut down resolution candidates
```

Reviewed By: huntie

Differential Revision: D58594993

fbshipit-source-id: 7be8c213a08fa651870ced588e2020ed8c3495d0
  • Loading branch information
robhogan authored and facebook-github-bot committed Aug 27, 2024
1 parent 60de111 commit 6f3af82
Show file tree
Hide file tree
Showing 5 changed files with 149 additions and 50 deletions.
18 changes: 13 additions & 5 deletions packages/metro-resolver/src/__tests__/index-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ const fileMap = {
name: 'invalid',
main: 'main',
}),
'/root/node_modules/flat-file-in-node-modules.js': '',
'/node_modules/root-module/main.js': '',
'/node_modules/root-module/package.json': JSON.stringify({
name: 'root-module',
Expand Down Expand Up @@ -110,8 +111,8 @@ it('does not resolve a relative path ending in a slash as a file', () => {
file: null,
dir: {
type: 'sourceFile',
filePathPrefix: '/root/project/bar/index',
candidateExts: ['', '.js', '.jsx', '.json', '.ts', '.tsx'],
filePathPrefix: '/root/project/bar/',
candidateExts: [],
},
}),
);
Expand All @@ -124,6 +125,13 @@ it('resolves a package in `node_modules`', () => {
});
});

it('resolves a standalone file in `node_modules`', () => {
expect(Resolver.resolve(CONTEXT, 'flat-file-in-node-modules', null)).toEqual({
type: 'sourceFile',
filePath: '/root/node_modules/flat-file-in-node-modules.js',
});
});

it('fails to resolve a relative path', () => {
try {
Resolver.resolve(CONTEXT, './apple', null);
Expand All @@ -134,8 +142,8 @@ it('fails to resolve a relative path', () => {
}
expect(error.candidates).toEqual({
dir: {
candidateExts: ['', '.js', '.jsx', '.json', '.ts', '.tsx'],
filePathPrefix: '/root/project/apple/index',
candidateExts: [],
filePathPrefix: '/root/project/apple',
type: 'sourceFile',
},
file: {
Expand Down Expand Up @@ -326,7 +334,7 @@ it('throws a descriptive error when a file inside a Haste package cannot be reso
"While resolving module \`some-package/subdir/does-not-exist\`, the Haste package \`some-package\` was found. However the module \`subdir/does-not-exist\` could not be found within the package. Indeed, none of these files exist:
* \`/haste/some-package/subdir/does-not-exist(.js|.jsx|.json|.ts|.tsx)\`
* \`/haste/some-package/subdir/does-not-exist/index(.js|.jsx|.json|.ts|.tsx)\`"
* \`/haste/some-package/subdir/does-not-exist\`"
`);
});

Expand Down
23 changes: 21 additions & 2 deletions packages/metro-resolver/src/__tests__/utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,17 @@ type MockFileMap = $ReadOnly<{
export function createResolutionContext(
fileMap: MockFileMap = {},
): $Diff<ResolutionContext, {originModulePath: string}> {
const directorySet = new Set<string>();
for (const filePath of Object.keys(fileMap)) {
let currentDir = filePath;
let prevDir;
do {
prevDir = currentDir;
currentDir = path.dirname(currentDir);
directorySet.add(currentDir);
} while (currentDir !== prevDir);
}

return {
dev: true,
allowHaste: true,
Expand All @@ -56,12 +67,20 @@ export function createResolutionContext(
web: ['browser'],
},
unstable_enablePackageExports: false,
unstable_fileSystemLookup: filePath => {
unstable_fileSystemLookup: inputPath => {
// Normalise and remove any trailing slash.
const filePath = path.resolve(inputPath);
const candidate = fileMap[filePath];
if (typeof candidate === 'string') {
return {exists: true, type: 'f', realPath: filePath};
}
if (candidate == null || candidate.realPath == null) {
if (candidate == null) {
if (directorySet.has(filePath)) {
return {exists: true, type: 'd', realPath: filePath};
}
return {exists: false};
}
if (candidate.realPath == null) {
return {exists: false};
}
return {
Expand Down
106 changes: 89 additions & 17 deletions packages/metro-resolver/src/resolve.js
Original file line number Diff line number Diff line change
Expand Up @@ -85,14 +85,23 @@ function resolve(
return result.resolution;
}

if (context.allowHaste && !isDirectImport) {
/**
* At this point, realModuleName is not a "direct" (absolute or relative)
* import, so it's either Haste name or a package specifier.
*/

if (context.allowHaste) {
const normalizedName = normalizePath(realModuleName);
const result = resolveHasteName(context, normalizedName, platform);
if (result.type === 'resolved') {
return result.resolution;
}
}

/**
* realModuleName is now a package specifier.
*/

const {disableHierarchicalLookup} = context;

const nodeModulesPaths = [];
Expand All @@ -102,7 +111,10 @@ function resolve(
let candidate;
do {
candidate = next;
nodeModulesPaths.push(path.join(candidate, 'node_modules'));
const nodeModulesPath = candidate.endsWith(path.sep)
? candidate + 'node_modules'
: candidate + path.sep + 'node_modules';
nodeModulesPaths.push(nodeModulesPath);
next = path.dirname(candidate);
} while (candidate !== next);
}
Expand All @@ -111,25 +123,38 @@ function resolve(
nodeModulesPaths.push(...context.nodeModulesPaths);

const extraPaths = [];

const parsedSpecifier = parsePackageSpecifier(realModuleName);

const {extraNodeModules} = context;
if (extraNodeModules) {
let bits = path.normalize(moduleName).split(path.sep);
let packageName;
// Normalize packageName and bits for scoped modules
if (bits.length >= 2 && bits[0].startsWith('@')) {
packageName = bits.slice(0, 2).join('/');
bits = bits.slice(1);
} else {
packageName = bits[0];
}
if (extraNodeModules[packageName]) {
bits[0] = extraNodeModules[packageName];
extraPaths.push(path.join.apply(path, bits));
}
if (extraNodeModules && extraNodeModules[parsedSpecifier.packageName]) {
const newPackageName = extraNodeModules[parsedSpecifier.packageName];
extraPaths.push(path.join(newPackageName, parsedSpecifier.posixSubpath));
}

const allDirPaths = nodeModulesPaths
.map(nodeModulePath => path.join(nodeModulePath, realModuleName))
.map(nodeModulePath => {
let lookupResult = null;
if (context.unstable_fileSystemLookup) {
// Insight: The module can only exist if there is a `node_modules` at
// this path. Redirections cannot succeed, because we will never look
// beyond a node_modules path segment for finding the closest
// package.json. Moreover, if the specifier contains a '/' separator,
// the first part *must* be a real directory, because it is the
// shallowest path that can possibly contain a redirecting package.json.
const mustBeDirectory =
parsedSpecifier.posixSubpath !== '.' ||
parsedSpecifier.packageName.length > parsedSpecifier.firstPart.length
? nodeModulePath + path.sep + parsedSpecifier.firstPart
: nodeModulePath;
lookupResult = context.unstable_fileSystemLookup(mustBeDirectory);
if (!lookupResult.exists || lookupResult.type !== 'd') {
return null;
}
}
return path.join(nodeModulePath, realModuleName);
})
.filter(Boolean)
.concat(extraPaths);
for (let i = 0; i < allDirPaths.length; ++i) {
const candidate = context.redirectModulePath(allDirPaths[i]);
Expand All @@ -149,6 +174,40 @@ function resolve(
throw new FailedToResolveNameError(nodeModulesPaths, extraPaths);
}

function parsePackageSpecifier(specifier: string) {
const normalized =
path.sep === '/' ? specifier : specifier.replaceAll('\\', '/');
const firstSepIdx = normalized.indexOf('/');
if (normalized.startsWith('@') && firstSepIdx !== -1) {
const secondSepIdx = normalized.indexOf('/', firstSepIdx + 1);
if (secondSepIdx === -1) {
return {
firstPart: normalized.slice(0, firstSepIdx),
packageName: normalized,
posixSubpath: '.',
};
}
return {
firstPart: normalized.slice(0, firstSepIdx),
packageName: normalized.slice(0, secondSepIdx),
posixSubpath: '.' + normalized.slice(secondSepIdx),
};
}
if (firstSepIdx === -1) {
return {
firstPart: normalized,
packageName: normalized,
posixSubpath: '.',
};
}
const packageName = normalized.slice(0, firstSepIdx);
return {
firstPart: packageName,
packageName,
posixSubpath: '.' + normalized.slice(firstSepIdx),
};
}

/**
* Resolve any kind of module path, whether it's a file or a directory.
* For example we may want to resolve './foobar'. The closest
Expand Down Expand Up @@ -336,6 +395,19 @@ function resolvePackageEntryPoint(
packagePath: string,
platform: string | null,
): Result<Resolution, FileCandidates> {
const dirLookup = context.unstable_fileSystemLookup?.(packagePath);

if (
dirLookup != null &&
(dirLookup.exists === false || dirLookup.type !== 'd')
) {
return failedFor({
type: 'sourceFile',
filePathPrefix: packagePath,
candidateExts: [],
});
}

const packageJsonPath = path.join(packagePath, 'package.json');

if (!context.doesFileExist(packageJsonPath)) {
Expand Down
Loading

0 comments on commit 6f3af82

Please sign in to comment.