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

Default metro resolver slower than MetroSymlinksResolver in a monorepo #1208

Open
abejfehr opened this issue Feb 4, 2024 · 3 comments
Open

Comments

@abejfehr
Copy link

abejfehr commented Feb 4, 2024

Do you want to request a feature or report a bug?

I'm not sure if this counts as a bug exactly, but it's a performance issue for monorepos in cases where most of the app's code (including node_modules) is outside the directory that the app is in.

What is the current behavior?

When building an app bundle with metro using the default metro resolver in a monorepo, the build times are a lot longer than if you use @rnx-kit/metro-resolver-symlinks.

If the current behavior is a bug, please provide the steps to reproduce and a minimal repository on GitHub that we can yarn install and yarn test.

To reproduce this somewhat effectively, you'd need a really large monorepo that looks something like this:

apps/
├── mobile # this is the RN app, including metro.config.js
└── web
libs/
├── mobile/
│   ├── screens /
│   │   ├── screen-1
│   │   ├── ...
│   │   └── screen-n
│   └── packages/
│       ├── package-1
│       ├── ...
│       └── package-n
└── shared/
    └── packages/
        ├── package-1
        ├── ...
        └── package-n
node_modules/

and in your metro config, something like this:

const path = require('path');
const { getDefaultConfig, mergeConfig } = require('@react-native/metro-config');

const defaultConfig = getDefaultConfig(__dirname);

const config = {
  watchFolders: [
    path.resolve(workspaceRoot, 'node_modules'),
    path.join(__dirname, '../../')
  ],
  projectRoot: __dirname,
};

module.exports = mergeConfig(defaultConfig, config);

If you bundle the app using a command like:

node --cpu-prof ./node_modules/.bin/react-native bundle --platform ios --dev true --minify false --entry-file index.js --reset-cache --bundle-output /tmp/bundle.js --max-workers 1

and analyze the generated .cpuprofile file in speedscope.net in the "Left Heavy" view, you can see that a non-trivial amount of time is spent in here:

image

It appears to be spending a lot of time (in this case over a minute because I'm using only 1 worker thread) in relative in (ironically) fast_path.js here, called by TreeFS here.

I think that happens because the files being imported exist outside of #rootDir, which in this example is /apps/mobile, whereas most of the code is in /libs/... in this repo.

Even though we have no need for resolving symlinks, we use MetroSymlinksResolver because it's just more performant in this scenario.

Possibly related: if you add a console.log inside the relative function and log the args, you see that it seems to run with every permutation of extension, e.g.

relative /Users/abejfehr/Development/some-monorepo/apps/mobile /Users/abejfehr/Development/some-monorepo/node_modules/react.native/package.json
relative /Users/abejfehr/Development/some-monorepo/apps/mobile /Users/abejfehr/Development/some-monorepo/node_modules/react.native
relative /Users/abejfehr/Development/some-monorepo/apps/mobile /Users/abejfehr/Development/some-monorepo/node_modules/react
relative /Users/abejfehr/Development/some-monorepo/apps/mobile /Users/abejfehr/Development/some-monorepo/node_modules/react.ios.ts/package.json
relative /Users/abejfehr/Development/some-monorepo/apps/mobile /Users/abejfehr/Development/some-monorepo/node_modules/react.ios.ts
relative /Users/abejfehr/Development/some-monorepo/apps/mobile /Users/abejfehr/Development/some-monorepo/node_modules/react.native.ts/package.json
relative /Users/abejfehr/Development/some-monorepo/apps/mobile /Users/abejfehr/Development/some-monorepo/node_modules/react.native.ts
relative /Users/abejfehr/Development/some-monorepo/apps/mobile /Users/abejfehr/Development/some-monorepo/node_modules/react.ts/package.json
relative /Users/abejfehr/Development/some-monorepo/apps/mobile /Users/abejfehr/Development/some-monorepo/node_modules/react.ts
relative /Users/abejfehr/Development/some-monorepo/apps/mobile /Users/abejfehr/Development/some-monorepo/node_modules/react.ios.js/package.json
relative /Users/abejfehr/Development/some-monorepo/apps/mobile /Users/abejfehr/Development/some-monorepo/node_modules/react.ios.js
relative /Users/abejfehr/Development/some-monorepo/apps/mobile /Users/abejfehr/Development/some-monorepo/node_modules/react.native.js/package.json
relative /Users/abejfehr/Development/some-monorepo/apps/mobile /Users/abejfehr/Development/some-monorepo/node_modules/react.native.js
relative /Users/abejfehr/Development/some-monorepo/apps/mobile /Users/abejfehr/Development/some-monorepo/node_modules/react.js/package.json
relative /Users/abejfehr/Development/some-monorepo/apps/mobile /Users/abejfehr/Development/some-monorepo/node_modules/react.js
relative /Users/abejfehr/Development/some-monorepo/apps/mobile /Users/abejfehr/Development/some-monorepo/node_modules/react.ios.tsx/package.json
relative /Users/abejfehr/Development/some-monorepo/apps/mobile /Users/abejfehr/Development/some-monorepo/node_modules/react.ios.tsx
relative /Users/abejfehr/Development/some-monorepo/apps/mobile /Users/abejfehr/Development/some-monorepo/node_modules/react.native.tsx/package.json
relative /Users/abejfehr/Development/some-monorepo/apps/mobile /Users/abejfehr/Development/some-monorepo/node_modules/react.native.tsx
relative /Users/abejfehr/Development/some-monorepo/apps/mobile /Users/abejfehr/Development/some-monorepo/node_modules/react.tsx/package.json
relative /Users/abejfehr/Development/some-monorepo/apps/mobile /Users/abejfehr/Development/some-monorepo/node_modules/react.tsx
relative /Users/abejfehr/Development/some-monorepo/apps/mobile /Users/abejfehr/Development/some-monorepo/node_modules/react.ios.graphql/package.json
relative /Users/abejfehr/Development/some-monorepo/apps/mobile /Users/abejfehr/Development/some-monorepo/node_modules/react.ios.graphql
relative /Users/abejfehr/Development/some-monorepo/apps/mobile /Users/abejfehr/Development/some-monorepo/node_modules/react.native.graphql/package.json
relative /Users/abejfehr/Development/some-monorepo/apps/mobile /Users/abejfehr/Development/some-monorepo/node_modules/react.native.graphql
relative /Users/abejfehr/Development/some-monorepo/apps/mobile /Users/abejfehr/Development/some-monorepo/node_modules/react.graphql/package.json
relative /Users/abejfehr/Development/some-monorepo/apps/mobile /Users/abejfehr/Development/some-monorepo/node_modules/react.graphql
relative /Users/abejfehr/Development/some-monorepo/apps/mobile /Users/abejfehr/Development/some-monorepo/node_modules/react.ios.mjs/package.json
relative /Users/abejfehr/Development/some-monorepo/apps/mobile /Users/abejfehr/Development/some-monorepo/node_modules/react.ios.mjs
relative /Users/abejfehr/Development/some-monorepo/apps/mobile /Users/abejfehr/Development/some-monorepo/node_modules/react.native.mjs/package.json
relative /Users/abejfehr/Development/some-monorepo/apps/mobile /Users/abejfehr/Development/some-monorepo/node_modules/react.native.mjs
relative /Users/abejfehr/Development/some-monorepo/apps/mobile /Users/abejfehr/Development/some-monorepo/node_modules/react.mjs/package.json
relative /Users/abejfehr/Development/some-monorepo/apps/mobile /Users/abejfehr/Development/some-monorepo/node_modules/react.mjs
relative /Users/abejfehr/Development/some-monorepo/apps/mobile /Users/abejfehr/Development/some-monorepo/node_modules/react.ios.json/package.json
relative /Users/abejfehr/Development/some-monorepo/apps/mobile /Users/abejfehr/Development/some-monorepo/node_modules/react.ios.json
relative /Users/abejfehr/Development/some-monorepo/apps/mobile /Users/abejfehr/Development/some-monorepo/node_modules/react.native.json/package.json
relative /Users/abejfehr/Development/some-monorepo/apps/mobile /Users/abejfehr/Development/some-monorepo/node_modules/react.native.json
relative /Users/abejfehr/Development/some-monorepo/apps/mobile /Users/abejfehr/Development/some-monorepo/node_modules/react.json/package.json
relative /Users/abejfehr/Development/some-monorepo/apps/mobile /Users/abejfehr/Development/some-monorepo/node_modules/react.json
relative /Users/abejfehr/Development/some-monorepo/apps/mobile /Users/abejfehr/Development/some-monorepo/node_modules/react/package.json
relative /Users/abejfehr/Development/some-monorepo/apps/mobile /Users/abejfehr/Development/some-monorepo/node_modules/react/index.js.native
relative /Users/abejfehr/Development/some-monorepo/apps/mobile /Users/abejfehr/Development/some-monorepo/node_modules/react/index.js

^ this is an actual portion (with some path redactions) of that log of it resolving react

It doesn't feel very effiicent to me, and it might be nice if there was a way to call relative once on the path and append the path with every permutation of extension on the already-relative'd version of the path, but I don't how involved a change like that might be.

What is the expected behavior?

Ideally, the default resolver would be closer to the same speed as MetroSymlinksResolver in the case of a monorepo where most of the app's code is outside of #rootDir.

Please provide your exact Metro configuration and mention your Metro, node, yarn/npm version and operating system.

I've given an abridged version of the metro configuration above.

OS and package manager doesn't matter much, but I use macOS and yarn 1.

@robhogan
Copy link
Contributor

robhogan commented Feb 5, 2024

Hi @abejfehr - thanks for taking the time to deep-dive into this - it's always good to find out how Metro is performing in setups "in the wild" outside Meta, and particularly in this detail.

A couple of high level points -

  • You've hit on two known perf/efficiency issues with the resolver here

    1. That we repeat a lot of work in typical sequences of existence checks - eg for multiple candidate extensions on the same basename (also finding the nearest package.json). This is possible to address with the addition of some fine-tuned APIs wired from TreeFS through to ResolutionContext. I plan to do this as part of the incremental resolution improvements underway at the moment (Implement incremental resolution: Fast Refresh on new files, changed symlinks, etc. #1161), which will likely land around RN 0.74.
    2. That fast_path.relative is built to optimise for resolutions being under the project root, but is actually slower than path.relative for anything else. This is the target of Improve pathUtils.absoluteToNormal correctness and performance #1210, which should improve the case of path under a parent or grandparent of the project root about 10x. I had this in the works already but you've reminded me to revive it :). Improve fast_path.resolve correctness and performance #1209 should help too (fast_path.resolve may be the next entry down in your CPU profile?). This will be in the next Metro patch release for RN 0.73 - it'd be great to know how it works for you.
  • Secondly, I am surprised @rnx-kit/metro-resolver-symlinks is faster - we don't use it and I'm not very close to its internals, but my understanding was that it augments Metro's existence checks with an IO fallback, so I'd expect it to be doing more work if anything. Since you have a like-for-like comparison with your project, might you be able to check what work Metro isn't doing as a result of using @rnx-kit/metro-resolver-symlinks? Maybe it's not using Metro's existence check for anything under a node_modules package?

@robhogan
Copy link
Contributor

Hi @abejfehr - Metro 0.80.6 was released yesterday with the two path manipulation performance improvements I mentioned.

If you're using RN 0.73 you should get that update by refreshing your lockfile - it'd be interesting to see whether that helps.

@abejfehr
Copy link
Author

Unfortunately we're using 0.72 at this time, but thank you for letting me know!

I'll also see if I can get the information you asked for previously about "what work Metro isn't doing as a result of using @rnx-kit/metro-resolver-symlinks"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants