Skip to content
This repository has been archived by the owner on Oct 27, 2020. It is now read-only.

perf(index): use the compiler's cached fs for stats (this.fs.stat) #42

Merged
merged 1 commit into from
Oct 25, 2018

Conversation

gusvargas
Copy link
Contributor

@gusvargas gusvargas commented Oct 22, 2018

Related to #35

I was experiencing a similar issue to that described in #10 (comment)

In my experience, with cache-loader rebuild time increased dramatically – from 2.5s to 9.5s. While initial build time decreased for ~40s.

After profiling incremental builds it looks like fs.stat is the biggest bottle neck, specifically when validating a cache entry after a hit:

cache-loader/src/index.js

Lines 112 to 114 in 74fccc4

async.each(cacheData.dependencies.concat(cacheData.contextDependencies), (dep, eachCallback) => {
fs.stat(dep.path, (statErr, stats) => {
if (statErr) {

This change decreased my incremental build times back down below 1s.

@jsf-clabot
Copy link

jsf-clabot commented Oct 22, 2018

CLA assistant check
All committers have signed the CLA.

@codecov
Copy link

codecov bot commented Oct 22, 2018

Codecov Report

Merging #42 into master will not change coverage.
The diff coverage is 0%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #42   +/-   ##
=======================================
  Coverage   17.47%   17.47%           
=======================================
  Files           2        2           
  Lines         103      103           
  Branches       13       13           
=======================================
  Hits           18       18           
  Misses         73       73           
  Partials       12       12
Impacted Files Coverage Δ
src/index.js 17.64% <0%> (ø) ⬆️

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 74fccc4...4a7ff10. Read the comment docs.

@gusvargas
Copy link
Contributor Author

If anyone is interested in trying this change out I created a fork of an open source app here: https://github.com/gusvargas/hackernews-react-graphql

Steps to reproduce:

  1. yarn && yarn dev
  2. Navigate to localhost:3000 (I think there is some magic in nextjs that makes it so that only "active" modules are compiled)
  3. Make a trivial change to "Main.js" and observe the terminal output. For me, compile time was floating slightly above and below 100ms.
  4. Uncomment the webpack config change in next.config.js to enable the cache-loader
  5. Restart the dev server and repeat step 3. Now I see compile times around 600ms-700ms.

Linking against the changes in PR brings the compile times back down to what is observed in step 3.

@michael-ciniawsky michael-ciniawsky changed the title perf: use the compiler's cached fs when stating perf(index): use the compiler's cached fs for stats (this.fs.stat) Oct 25, 2018
Copy link
Member

@michael-ciniawsky michael-ciniawsky left a comment

Choose a reason for hiding this comment

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

@michael-ciniawsky michael-ciniawsky merged commit d8c630b into webpack-contrib:master Oct 25, 2018
@michael-ciniawsky michael-ciniawsky added this to the 1.2.3 milestone Oct 25, 2018
@gusvargas gusvargas deleted the use-compiler-fs branch October 26, 2018 15:12
@gusvargas
Copy link
Contributor Author

@michael-ciniawsky Any chance we can get the 1.2.3 release soon? The incremental build performance issue was the only thing holding us back from using the cache-loader – we'd love to start taking advantage of it ASAP.

@michael-ciniawsky
Copy link
Member

Sry for the delay, released in v1.2.3 🎉

@michael-ciniawsky michael-ciniawsky removed this from the 1.2.4 milestone Oct 30, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants