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

speed up incremental builds by not doing excessive stats.toJSON work #1362

Merged
merged 2 commits into from Apr 7, 2018

Conversation

@kenotron
Copy link
Contributor

kenotron commented Apr 4, 2018

  • This is a bugfix
  • This is a code refactor
  • This is a test update
  • This is a typo fix
  • This is a metadata update

For Bugs and Features; did you add new tests?

There's no need to add tests, as it is just a configuration change to toJSON

Motivation / Use-Case

The stats.toJSON call is done every time an incremental build finishes. Given a sufficiently large codebase, this is very SLOW (my repo went from ~30s to ~10s just by getting rid of this)

Breaking Changes

None

Additional Info

@codecov

This comment has been minimized.

Copy link

codecov bot commented Apr 4, 2018

Codecov Report

Merging #1362 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1362   +/-   ##
=======================================
  Coverage   79.14%   79.14%           
=======================================
  Files           6        6           
  Lines         494      494           
  Branches      160      160           
=======================================
  Hits          391      391           
  Misses        103      103
Impacted Files Coverage Δ
lib/Server.js 82.26% <100%> (ø) ⬆️

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 3a7f7d5...40000a9. Read the comment docs.

@@ -25,7 +25,7 @@ const createLog = require('./createLog');
const OptionsValidationError = require('./OptionsValidationError');
const optionsSchema = require('./optionsSchema.json');

const clientStats = { errorDetails: false };
const clientStats = { errorDetails: false, chunkSortMode: 'none', chunks: false };

This comment has been minimized.

Copy link
@sokra

sokra Apr 4, 2018

Member

I think you can even go with

{ all: false, assets: true, warnings: true, errors: true, errorDetails: false }

since these are the only fields used.

This comment has been minimized.

Copy link
@kenotron

kenotron Apr 4, 2018

Author Contributor

will do!

This comment has been minimized.

Copy link
@kenotron

kenotron Apr 5, 2018

Author Contributor

Updated

@kenotron

This comment has been minimized.

Copy link
Contributor Author

kenotron commented Apr 5, 2018

@sokra - can you please merge after my update?

@evilebottnawi

This comment has been minimized.

Copy link
Member

evilebottnawi commented Apr 5, 2018

@kenotron just wait @shellscape review

@shellscape

This comment has been minimized.

Copy link
Contributor

shellscape commented Apr 5, 2018

@evilebottnawi @kenotron I no longer maintain this repository. My efforts and recommendation are directed to webpack-serve as a modern alternative to WDS.

@evilebottnawi

This comment has been minimized.

Copy link
Member

evilebottnawi commented Apr 5, 2018

@shellscape I think we should maintain package only for bug fix, many people not yet moved to webpack@4 and webpack-serve. I agree what we don't should add new feature. But this PR fix perf bugs and i think we should merge and do patch release.

@shellscape

This comment has been minimized.

Copy link
Contributor

shellscape commented Apr 5, 2018

@evilebottnawi from an objective standpoint I would agree that a performance improvement is not a new feature and would fall into the category of "maintenance."

@evilebottnawi

This comment has been minimized.

Copy link
Member

evilebottnawi commented Apr 5, 2018

@shellscape let's do this 👍

@kenotron

This comment has been minimized.

Copy link
Contributor Author

kenotron commented Apr 6, 2018

@shellscape would you mind taking this change? Not everyone in the world can afford to move to the next new thing. I think this little bit of maintenance mode change would benefit quite a LOT of people who still must stick with patch / minor changes.

@shellscape

This comment has been minimized.

Copy link
Contributor

shellscape commented Apr 6, 2018

@kenotron I don't think you understand - I am not a maintainer of this repo, hence I cannot make these changes. I was only asked for my opinion on the change. I'll be unsubscribing from this thread now, as I don't care for it when users levy unfair accusations at me.

@kenotron

This comment has been minimized.

Copy link
Contributor Author

kenotron commented Apr 6, 2018

@sokra - can you merge this?

@SpaceK33z SpaceK33z merged commit da33d2b into webpack:master Apr 7, 2018
6 checks passed
6 checks passed
Codacy/PR Quality Review Good work! A positive pull request.
Details
codecov/patch 100% of diff hit (target 79.14%)
Details
codecov/project 79.14% (+0%) compared to 3a7f7d5
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
licence/cla Contributor License Agreement is signed.
Details
security/snyk - package.json No dependency changes
Details
@SpaceK33z

This comment has been minimized.

Copy link
Member

SpaceK33z commented Apr 7, 2018

Fix released in webpack-dev-server@3.1.2. Thanks @kenotron!

@bes bes mentioned this pull request Apr 7, 2018
1 of 2 tasks complete
@SpaceK33z

This comment has been minimized.

Copy link
Member

SpaceK33z commented Apr 8, 2018

Note that this broke HMR, which was fixed in e1b263a.

@SpaceK33z

This comment has been minimized.

Copy link
Member

SpaceK33z commented Apr 8, 2018

@kenotron I am curious if the above fix has any negative performance implications? If so we could perhaps only enable the hash option when HMR is used.

@kenotron

This comment has been minimized.

Copy link
Contributor Author

kenotron commented Apr 9, 2018

@SpaceK33z the hash option didn't significantly do anything to our build times, so I think it's probably okay to leave it be. I'd report another bug if we noticed any anomalies.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.