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.

Sign up for GitHub

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

Jump to bottom

Prevent unnecessary FS calls #16

Open
gaearon opened this issue Jan 17, 2018 · 4 comments
Open

Prevent unnecessary FS calls #16

gaearon opened this issue Jan 17, 2018 · 4 comments

Comments

@gaearon
Copy link

gaearon commented Jan 17, 2018

It seems like the plugin does a bunch of FS calls for every single file. This can cause build performance issues in large projects.

Could you consider caching some results? For example if I put a lot before fs.existsSync check in find-root, I see output like this for a tiny project that just imports react and react-dom:

checking /Users/dan/Projects/create-react-app/node_modules/object-assign/index.js
checking /Users/dan/Projects/create-react-app/node_modules/object-assign
checking /Users/dan/Projects/create-react-app/node_modules/react/index.js
checking /Users/dan/Projects/create-react-app/node_modules/react
checking /Users/dan/Projects/create-react-app/node_modules/fbjs/lib/emptyFunction.js
checking /Users/dan/Projects/create-react-app/node_modules/fbjs/lib
checking /Users/dan/Projects/create-react-app/node_modules/fbjs
checking /Users/dan/Projects/create-react-app/node_modules/promise/lib/core.js
checking /Users/dan/Projects/create-react-app/node_modules/promise/lib
checking /Users/dan/Projects/create-react-app/node_modules/promise
checking /Users/dan/Projects/create-react-app/node_modules/fbjs/lib/emptyObject.js
checking /Users/dan/Projects/create-react-app/node_modules/fbjs/lib
checking /Users/dan/Projects/create-react-app/node_modules/fbjs
checking /Users/dan/Projects/create-react-app/packages/react-scripts/config/polyfills.js
checking /Users/dan/Projects/create-react-app/packages/react-scripts/config
checking /Users/dan/Projects/create-react-app/packages/react-scripts
checking /Users/dan/Projects/create-react-app/node_modules/promise/lib/rejection-tracking.js
checking /Users/dan/Projects/create-react-app/node_modules/promise/lib
checking /Users/dan/Projects/create-react-app/node_modules/promise
checking /Users/dan/Projects/create-react-app/node_modules/asap/browser-raw.js
checking /Users/dan/Projects/create-react-app/node_modules/asap
checking /Users/dan/Projects/create-react-app/node_modules/webpack/buildin/global.js
checking /Users/dan/Projects/create-react-app/node_modules/webpack/buildin
checking /Users/dan/Projects/create-react-app/node_modules/webpack
checking /Users/dan/Projects/create-react-app/node_modules/promise/lib/es6-extensions.js
checking /Users/dan/Projects/create-react-app/node_modules/promise/lib
checking /Users/dan/Projects/create-react-app/node_modules/promise
checking /Users/dan/Projects/create-react-app/node_modules/whatwg-fetch/fetch.js
checking /Users/dan/Projects/create-react-app/node_modules/whatwg-fetch
checking /Users/dan/Projects/create-react-app/packages/react-scripts/template/src/index.js
checking /Users/dan/Projects/create-react-app/packages/react-scripts/template/src
checking /Users/dan/Projects/create-react-app/packages/react-scripts/template
checking /Users/dan/Projects/create-react-app/packages/react-scripts
checking /Users/dan/Projects/create-react-app/node_modules/react/cjs/react.production.min.js
checking /Users/dan/Projects/create-react-app/node_modules/react/cjs
checking /Users/dan/Projects/create-react-app/node_modules/react
checking /Users/dan/Projects/create-react-app/node_modules/react-dom/index.js
checking /Users/dan/Projects/create-react-app/node_modules/react-dom
checking /Users/dan/Projects/create-react-app/node_modules/react-dom/cjs/react-dom.production.min.js
checking /Users/dan/Projects/create-react-app/node_modules/react-dom/cjs
checking /Users/dan/Projects/create-react-app/node_modules/react-dom
checking /Users/dan/Projects/create-react-app/node_modules/fbjs/lib/ExecutionEnvironment.js
checking /Users/dan/Projects/create-react-app/node_modules/fbjs/lib
checking /Users/dan/Projects/create-react-app/node_modules/fbjs
checking /Users/dan/Projects/create-react-app/node_modules/fbjs/lib/EventListener.js
checking /Users/dan/Projects/create-react-app/node_modules/fbjs/lib
checking /Users/dan/Projects/create-react-app/node_modules/fbjs
checking /Users/dan/Projects/create-react-app/node_modules/fbjs/lib/getActiveElement.js
checking /Users/dan/Projects/create-react-app/node_modules/fbjs/lib
checking /Users/dan/Projects/create-react-app/node_modules/fbjs
checking /Users/dan/Projects/create-react-app/node_modules/fbjs/lib/shallowEqual.js
checking /Users/dan/Projects/create-react-app/node_modules/fbjs/lib
checking /Users/dan/Projects/create-react-app/node_modules/fbjs
checking /Users/dan/Projects/create-react-app/node_modules/fbjs/lib/containsNode.js
checking /Users/dan/Projects/create-react-app/node_modules/fbjs/lib
checking /Users/dan/Projects/create-react-app/node_modules/fbjs
checking /Users/dan/Projects/create-react-app/node_modules/fbjs/lib/isTextNode.js
checking /Users/dan/Projects/create-react-app/node_modules/fbjs/lib
checking /Users/dan/Projects/create-react-app/node_modules/fbjs
checking /Users/dan/Projects/create-react-app/node_modules/fbjs/lib/isNode.js
checking /Users/dan/Projects/create-react-app/node_modules/fbjs/lib
checking /Users/dan/Projects/create-react-app/node_modules/fbjs
checking /Users/dan/Projects/create-react-app/node_modules/fbjs/lib/focusNode.js
checking /Users/dan/Projects/create-react-app/node_modules/fbjs/lib
checking /Users/dan/Projects/create-react-app/node_modules/fbjs
checking /Users/dan/Projects/create-react-app/packages/react-scripts/template/src/index.css
checking /Users/dan/Projects/create-react-app/packages/react-scripts/template/src
checking /Users/dan/Projects/create-react-app/packages/react-scripts/template
checking /Users/dan/Projects/create-react-app/packages/react-scripts
checking /Users/dan/Projects/create-react-app/packages/react-scripts/template/src/App.js
checking /Users/dan/Projects/create-react-app/packages/react-scripts/template/src
checking /Users/dan/Projects/create-react-app/packages/react-scripts/template
checking /Users/dan/Projects/create-react-app/packages/react-scripts
checking /Users/dan/Projects/create-react-app/packages/react-scripts/template/src/logo.svg
checking /Users/dan/Projects/create-react-app/packages/react-scripts/template/src
checking /Users/dan/Projects/create-react-app/packages/react-scripts/template
checking /Users/dan/Projects/create-react-app/packages/react-scripts
checking /Users/dan/Projects/create-react-app/packages/react-scripts/template/src/App.css
checking /Users/dan/Projects/create-react-app/packages/react-scripts/template/src
checking /Users/dan/Projects/create-react-app/packages/react-scripts/template
checking /Users/dan/Projects/create-react-app/packages/react-scripts
checking /Users/dan/Projects/create-react-app/packages/react-scripts/template/src/registerServiceWorker.js
checking /Users/dan/Projects/create-react-app/packages/react-scripts/template/src
checking /Users/dan/Projects/create-react-app/packages/react-scripts/template
checking /Users/dan/Projects/create-react-app/packages/react-scripts
checking /Users/dan/Projects/create-react-app/packages/react-scripts/template/src/index.css
checking /Users/dan/Projects/create-react-app/packages/react-scripts/template/src
checking /Users/dan/Projects/create-react-app/packages/react-scripts/template
checking /Users/dan/Projects/create-react-app/packages/react-scripts
checking /Users/dan/Projects/create-react-app/node_modules/css-loader/lib/css-base.js
checking /Users/dan/Projects/create-react-app/node_modules/css-loader/lib
checking /Users/dan/Projects/create-react-app/node_modules/css-loader
checking /Users/dan/Projects/create-react-app/node_modules/style-loader/lib/addStyles.js
checking /Users/dan/Projects/create-react-app/node_modules/style-loader/lib
checking /Users/dan/Projects/create-react-app/node_modules/style-loader
checking /Users/dan/Projects/create-react-app/node_modules/style-loader/lib/urls.js
checking /Users/dan/Projects/create-react-app/node_modules/style-loader/lib
checking /Users/dan/Projects/create-react-app/node_modules/style-loader
checking /Users/dan/Projects/create-react-app/packages/react-scripts/template/src/App.css
checking /Users/dan/Projects/create-react-app/packages/react-scripts/template/src
checking /Users/dan/Projects/create-react-app/packages/react-scripts/template
checking /Users/dan/Projects/create-react-app/packages/react-scripts

There might be some opportunities to improve here.

For example when we learn the root for packages/react-scripts/template/src/App.css by traversing upwards, we can memoize that it's also the root for packages/react-scripts/template/src/ and packages/react-scripts/template/.

Then next time we traverse we can short-circuit instead of checking it again.

I would suggest taking a large project (>1000 files) and profiling the build time with and without this plugin. If there is a noticeable difference it's worth trying to cache more things.
@gaearon gaearon mentioned this issue Jan 17, 2018
Closed
@darrenscerri
Copy link
Owner

darrenscerri commented Jan 17, 2018 •
edited
Loading

Thanks for opening this issue @gaearon, this definitely seems like it could cause performance issues in big projects. I'll look into it.

@gaearon
Copy link
Author

gaearon commented Jan 17, 2018

Note that after some discussion, we might not include the plugin after all. I'm worried it will too often be unactionable especially for tiny deps where this doesn't matter (e.g. is-path-regex or whatever). But I figured it's still worth raising an issue here.

@darrenscerri
Copy link
Owner

darrenscerri commented Jan 19, 2018 •
edited
Loading

That's a good point in the context of CRA, I think it all depends on the type/magnitude of the project and what the developers deem important in the context of that project. Maybe we can make the plugin configurable to make it more appropriate for use with CRA, such as setting a minimum size for deps before warning?

@gaearon
Copy link
Author

gaearon commented Jan 21, 2018

That would make sense, but it's not obvious how the plugin could measure that.

@fengzilong fengzilong mentioned this issue Jan 20, 2020
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@darrenscerri @gaearon