-
-
Notifications
You must be signed in to change notification settings - Fork 9.2k
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
Performance optimizations on separate manager preview split, improved cold start, restart & rebuild #4834
Conversation
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
84962d9
to
83a1800
Compare
# Conflicts: # addons/backgrounds/package.json # lib/ui/package.json
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what happened in all these files?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Some debugging to do:
|
Got confirmation on discord that this is approved
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.