Performance optimizations on separate manager preview split, improved cold start, restart & rebuild #4834
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ndelangen
requested review from
alterx,
Hypnosphi,
igor-dv,
thomasbertet and
tmeasday
as code owners
ndelangen
force-pushed
the
separate-manager-preview-p5
branch
2 times, most recently
from
e79d295
to
84962d9
Compare
Codecov Report
@@ Coverage Diff @@
## next #4834 +/- ##
==========================================
- Coverage 35.21% 34.84% -0.38%
==========================================
Files 564 566 +2
Lines 6931 7005 +74
Branches 931 937 +6
==========================================
Hits 2441 2441
- Misses 3996 4064 +68
- Partials 494 500 +6
Continue to review full report at Codecov.
|
Changelog formatting Bump @storybook/addon-actions from 4.0.0 to 4.0.8 in /docs Bumps [@storybook/addon-actions](https://github.com/storybooks/storybook) from 4.0.0 to 4.0.8. - [Release notes](https://github.com/storybooks/storybook/releases) - [Changelog](https://github.com/storybooks/storybook/blob/next/CHANGELOG.md) - [Commits](v4.0.0...v4.0.8) Signed-off-by: dependabot[bot] <support@dependabot.com> update `npm install` to match current requirements Add TypeScript support for react-scripts 4.1.0-alpha.5 changelog v4.1.0-alpha.5 4.1.0-alpha.6 changelog v4.1.0-alpha.6 Reduce jest image size 4.1.0-alpha.7 changelog v4.1.0-alpha.7 ADD webpack stats generation for debugging Revert "Update LICENSE" This reverts commit 69fc051. Don't change rootElement when received node is the same Merge pull request #4830 from aliceyoung9/patch-1 little documentation fix FIX linting
ndelangen
force-pushed
the
separate-manager-preview-p5
branch
from
84962d9
to
83a1800
Compare
# Conflicts: # addons/backgrounds/package.json # lib/ui/package.json
ndelangen
requested review from
dangreenisrael,
danielduan,
theinterned,
usulpro and
z4o4z
as code owners
ndelangen
changed the title
lib/ui/scripts/config.js
Outdated
| loader: 'babel-loader', | ||
| options: { | ||
| presets: [ | ||
| ['@babel/preset-env', { shippedProposals: true, modules: false }], |
There was a problem hiding this comment.
I feel like i've seen this above. Maybe the babel rcs could be moved to a common location?
There was a problem hiding this comment.
Sounds like an improvement/refactor we could do a bit later. Good suggestion!
|
@norbert - re the Chromatic failures -- looking at a saved snapshot in liveview, e.g. https://www.chromaticqa.com/component?appId=5a375b97f4b14f0020b0cda3&name=App%7Cacceptance&specNumber=1&buildNumber=5357 It looks to me like the project hasn't build properly, specifically this file: https://xxzvtulteo.tunnel.chromaticqa.com/sb_dll/storybook_ui_dll.js Which serves: (those directories [ |
|
Who can give this a final check, so we can move ahead and merge. That way we can get this in a "next" release? |
tmeasday
approved these changes
Nov 29, 2018
There was a problem hiding this comment.
Code LGTM.
I compared it to next on my iMac:
On next:
╭──────────────────────────────────────────────────╮
│ │
│ Storybook 4.1.0-alpha.8 started │
│ 26 s for manager and 12 s for preview │
│ │
│ Local: http://localhost:9011/ │
│ On your network: http://192.168.0.22:9011/ │
│ │
╰──────────────────────────────────────────────────╯
On this branch:
╭──────────────────────────────────────────────────╮
│ │
│ Storybook 4.1.0-alpha.8 started │
│ 7.46 s for manager and 9.1 s for preview │
│ │
│ Local: http://localhost:9011/ │
│ On your network: http://192.168.0.22:9011/ │
│ │
╰──────────────────────────────────────────────────╯
Seems positive.
It did output this:
Error: ENOENT: no such file or directory, stat '/Users/tom/OSS/storybook/lib/core/dist/public/runtime~main.3aaa912fd37e258bb4b5.bundle.js'
Error: ENOENT: no such file or directory, stat '/Users/tom/OSS/storybook/lib/core/dist/public/main.ef442cdddb07ad8080c3.bundle.js'
Error: ENOENT: no such file or directory, stat '/Users/tom/OSS/storybook/lib/core/dist/public/vendors~main.9171936130daf921c71d.bundle.js'
Error: ENOENT: no such file or directory, stat '/Users/tom/OSS/storybook/lib/core/dist/public/runtime~main.0f4ed394e749803fae17.bundle.js'
Error: ENOENT: no such file or directory, stat '/Users/tom/OSS/storybook/lib/core/dist/public/main.0f4ed394e749803fae17.bundle.js'
Error: ENOENT: no such file or directory, stat '/Users/tom/OSS/storybook/lib/core/dist/public/vendors~main.0f4ed394e749803fae17.bundle.js'
It didn't seem to cause any issues though, and didn't appear on a second run.
igor-dv
previously requested changes
Nov 29, 2018
There was a problem hiding this comment.
Shouldn't we "watch" on dlls?
One of the techniques is a shared cache between the 2 instances of webpack
If manager will be moved to its own process, this option probably can't be used. Also, can't this cache become problematic with manager and preview having a different react versions?
| @@ -1,6 +1,6 @@ | |||
| import React from 'react'; | |||
| import { storiesOf } from '@storybook/react'; | |||
| import { action } from '@storybook/addon-actions'; | |||
| import { storiesOf } from '@storybook/react'; // eslint-disable-line import/no-extraneous-dependencies | |||
There was a problem hiding this comment.
what happened in all these files?
There was a problem hiding this comment.
I wanted to remove the cyclical dependencies warnings from lerna
# Conflicts: # addons/backgrounds/package.json # lib/core/src/server/build-static.js # lib/ui/package.json
Co-Authored-By: ndelangen <ndelangen@me.com>
…torybook into separate-manager-preview-p5
…FACTOR writing stats to file
igor-dv
reviewed
Nov 29, 2018
|
Some debugging to do: |
ndelangen
dismissed
igor-dv’s stale review
Got confirmation on discord that this is approved
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR adds optimisation to the preview-manager split.
It uses a couple of techniques to make the cold start and hot restart of webpack faster.
I've done some testing and the performance of the manager seems heavily dependent on how busy the preview is...
I think this means it WOULD be logical to move the manager back out into it's own process.
Overall I'm seeing a 41s start-up on the next branch, this one has a bit more reasonable time of 11s.
When I stop and re-start the dev server I'm seeing a boot up time of 9 seconds.
I also added a bunch of logs and profile data from webpack to be able to somewhat debug what is happening.
The total time for manager and preview are now printed in the terminal for end-users. They'll be more incentivised to report issues I hope, or at the very least they'll get feedback if performance is improving or deteriorating.
The debug info is written to the cache dir when you perform a smoke-test.
The debug information includes all the stats for both manager and preview. You can dump them into something like:
https://chrisbateman.github.io/webpack-visualizer/ or http://webpack.github.io/analyse/#modules
The lib/ui will be missing because it's essentially pre-bundled.
Another piece of debugging data is the full event log of webpack.
Maybe it's a bug, maybe I'm doing something wrong, but it seems to be the combined log of both webpack instances. either way it can be insightful what is taking webpack so long.