[webpack] setup lazy loading for all visualizations #4727
Conversation
| [VIZ_TYPES.event_flow]: () => loadVis(import(/* webpackChunkName: "EventFlow" */ './EventFlow.jsx')), | ||
| [VIZ_TYPES.paired_ttest]: () => loadVis(import(/* webpackChunkName: "paired_ttest" */ './paired_ttest.jsx')), | ||
| [VIZ_TYPES.partition]: () => loadVis(import(/* webpackChunkName: "partition" */ './partition.js')), | ||
| [VIZ_TYPES.deck_scatter]: () => loadVis(import(/* webpackChunkName: "deckgl/layers/scatter" */ './deckgl/layers/scatter.jsx')), |
There was a problem hiding this comment.
can we use the same webpackChunkName for all deck_*?
There was a problem hiding this comment.
What's the goal here? for readability or to have all of that code in one chunk?
I think using the same chunk name would only work if the same code was imported which is currently not possible since there are multiple index files for the various deck gl layers, right?
|
Wondering why the We should probably have one |
|
But this is awesome btw! |
|
fyi hitting some issues testing this with prod data, still working on it. should be able to get the chunk files to have names that match the name in the code 👍 |
|
@williaster Did the issue around testing with Prod data resolve? |
|
sorry progress has been slower than expected, have been slammed with other projects including a focused sprint this week. will make this a priority to test early next week. |
williaster
force-pushed
the
chris--lazy-load-visualizations
branch
from
2102987 to
7fe5243
Compare
|
I got chunk names to update (see exact webpack input / output below) and wanted to post some numbers from local dashboard testing which all look pretty good:
webpack output before webpack output after |
|
We should use the same webchunk name for all The "DeckGL Demo" dashboard should be much fast as it will load one 8mb chunk instead of 6 of them. Is it just a matter of sharing/reusing webchunknames? |
|
It's not as trivial as using the same chunk name. The final name consists of chunk name and hash. I'd rather do that optimization in another PR because I think you'd have to create an index file that exports all of deckgl and async import that file. I'm not as familiar with the deckgl code organization so you all might be best suited to do that? |
|
I can give deck.gl a shot if you want tho, I think the d3 bundle is almost over optimizing. That'll be harder to untangle potentially as we refactor vis libraries. |
|
I'm worried about the way things look here without bundling visualizations together. The sum of all visualization is much more than the original A simple dashboard with a I'm guessing the load time improvements have to do with the fact that some of the work can be started upfront while the bundles are downloading. Results may look different on a network-bound scenario. |
|
We need to fix the fact that We probably need some sort of |
Yes, and this is great from a user perspective because it doesn't block page render like it currently does. I think this will have the effect of making users feel like the page is loading much faster.
React is not that large (even smaller if we could actually upgrade to It seems like the proper way to pull out "globally shared" code is using the webpack The sad part is that this new deck.gl chunk doesn't load properly 😢 will keep digging a bit but may have to abandon it, or go talk to one of our web perf folks to see if they can help point me in the right direction. Sounds like there are many improvements for these exact thing in |
|
all right things are going to get crazier from here, I was advised to not waste time getting the Just for fun-sies I tried the changes here on top of the webpack 4 change and we are going going to generate a crazy number of bundles (because it does basically all of the shared chunk-ing you described above). this is a good overview article. |
|
@williaster not sure if you are already familiar with this, but https://github.com/webpack-contrib/webpack-bundle-analyzer may be useful for deeper insights into the items impacting your bundle size here. Beyond the deck.gl stuff, seems like brace, mathjs, and moment (locale) are most impactful. There may be some opportunity to further optimize those, but would be interested in the webpack 4 upgrade stats before diving in on anything there. Happy to help on this effort once you decide on direction to take this with @mistercrunch. This is the output I got from your branch with the above plugin:
|
|
Hi, Just wanted to check if we are planning to merge william's changes into superset? Thanks |
|
Sorry I've been slow getting back to this, was on vacation and now trying to wrap up dashboard v2 which is a monster. I hit an issue with the chunk urls that are generated when I use the webpack v4 |
|
back on it, I think I figured out the webpack 4 issue. will post back today or tomorrow with bundle analysis etc! 🚀 |
williaster
force-pushed
the
chris--lazy-load-visualizations
branch
from
b5d0216 to
5f92319
Compare
|
Bundles are looking good in terms of maximum splitting/re-use. These images don't fully capture the picture for entry points (e.g., explorer, dashboard, sqllab) because every entry point "runtime" now actually consists of multiple scripts (for maximum sharing) [templates were updated accordingly]. Rough empirical #s for DOM loaded, MB transferred, etc. match pretty close to what I posted above, but the number of requests is now higher. I'm not sure this really matters if empirical load time + total MB transferred are still good, though. before
after webpack 4 + splitchunks
|
|
Curious as to what happens if you pick multiple visualizations to have same |
superset/assets/package.json
Outdated
| "webpack-manifest-plugin": "1.3.2", | ||
| "webworkify-webpack": "2.1.0" | ||
| "url-loader": "^1.0.1", | ||
| "webpack": "^4.6.0", |
There was a problem hiding this comment.
Oh noice! I tried to do this in the past and failed :)
There was a problem hiding this comment.
there were several loaders that had to be swapped out to support it 👍 took a while to figure it out!
|
Very excited to see this. Bundle sizes prior to this PR were starting to be quite embarrassing. |
|
Wow, this is super cool. It will improve user experience for lot of superset users. Thanks @williaster for all the hard work 👍 |
williaster
force-pushed
the
chris--lazy-load-visualizations
branch
from
daf5e5d to
7a49d26
Compare
|
@mistercrunch re the same chunk for deckgl, I'm not sure that using the same chunk name is the best way to fix that given the options that the SplitChunks plugin supports. For example you can specify a My inclination is that we should see what happens with this since it's already a big change, and then tweak the optimizations from there? wdyt? Rebased on latest master just now and tried to test a lot myself locally. Will try to test on a staging box but if you could pull it and test more extensively with deck.gl that'd be super helpful! Unless we see any issues I think it's ready to merge! |
Codecov Report
@@ Coverage Diff @@
## master #4727 +/- ##
==========================================
+ Coverage 77.46% 77.47% +<.01%
==========================================
Files 44 44
Lines 8730 8737 +7
==========================================
+ Hits 6763 6769 +6
- Misses 1967 1968 +1
Continue to review full report at Codecov.
|
|
Very nice. This seems to be a huge step forward, we can tweak things later as needed. |
|
These are the final set of 3x empirical tests per example dashboard before and after, on non-minimized code with hard refresh/no caching. They look even better than above!
@mistercrunch shall we merge this? 🚀
|
…hart /> vis promise when vis type changes
williaster
force-pushed
the
chris--lazy-load-visualizations
branch
from
ea5f147 to
6a22d90
Compare
|
all right tests look good, should we merge @mistercrunch @graceguo-supercat @john-bodley ? |
|
Green light on my side |
|
all righty 🤞🤞🤞 |
* [webpack] setup lazy loading for all visualizations * [lazy-load] push renderVis function to <Chart /> state * no mapbox token * [lazy loading] use native webpack import func to fix chunk names, add babel-plugin-syntax-dynamic-import, fix rebase bug. * fix geojson import, undefined t, and fix async css bug * [lazy load] actually add babel-plugin-syntax-dynamic-import * [webpack] working dev version of webpack v4 * [webpack 4] fix url issues, use mini-css-extract-plugin and webpack-assets-manifest plugins * [webpack 4] use splitchunks for all files, update templates to multi-file entrypoints * [webpack 4] multiple theme entry files for markup vis css, don't uglify mapbox * [webpack 4] lint python manifest changes, update yarn lock. * [webpack 4] fix tests with babel-plugin-dynamic-import-node * [webpack 4] only use 'dynamic-import-node' plugin in tests, update <Chart /> vis promise when vis type changes * [webpack 4] clean up package.json and yarn.lock after rebase * [webpack 4] lint? * [webpack 4] lint for real (cherry picked from commit de0aaf4)
This reverts commit de0aaf4.
* [webpack] setup lazy loading for all visualizations * [lazy-load] push renderVis function to <Chart /> state * no mapbox token * [lazy loading] use native webpack import func to fix chunk names, add babel-plugin-syntax-dynamic-import, fix rebase bug. * fix geojson import, undefined t, and fix async css bug * [lazy load] actually add babel-plugin-syntax-dynamic-import * [webpack] working dev version of webpack v4 * [webpack 4] fix url issues, use mini-css-extract-plugin and webpack-assets-manifest plugins * [webpack 4] use splitchunks for all files, update templates to multi-file entrypoints * [webpack 4] multiple theme entry files for markup vis css, don't uglify mapbox * [webpack 4] lint python manifest changes, update yarn lock. * [webpack 4] fix tests with babel-plugin-dynamic-import-node * [webpack 4] only use 'dynamic-import-node' plugin in tests, update <Chart /> vis promise when vis type changes * [webpack 4] clean up package.json and yarn.lock after rebase * [webpack 4] lint? * [webpack 4] lint for real
…5219) * Revert "[explore] fix autocomplete on verbose names (apache#5204)" This reverts commit d5ebc43. * Revert "[webpack] setup lazy loading for all visualizations (apache#4727)" This reverts commit de0aaf4.
* [webpack] setup lazy loading for all visualizations * [lazy-load] push renderVis function to <Chart /> state * no mapbox token * [lazy loading] use native webpack import func to fix chunk names, add babel-plugin-syntax-dynamic-import, fix rebase bug. * fix geojson import, undefined t, and fix async css bug * [lazy load] actually add babel-plugin-syntax-dynamic-import * [webpack] working dev version of webpack v4 * [webpack 4] fix url issues, use mini-css-extract-plugin and webpack-assets-manifest plugins * [webpack 4] use splitchunks for all files, update templates to multi-file entrypoints * [webpack 4] multiple theme entry files for markup vis css, don't uglify mapbox * [webpack 4] lint python manifest changes, update yarn lock. * [webpack 4] fix tests with babel-plugin-dynamic-import-node * [webpack 4] only use 'dynamic-import-node' plugin in tests, update <Chart /> vis promise when vis type changes * [webpack 4] clean up package.json and yarn.lock after rebase * [webpack 4] lint? * [webpack 4] lint for real
…5219) * Revert "[explore] fix autocomplete on verbose names (apache#5204)" This reverts commit d5ebc43. * Revert "[webpack] setup lazy loading for all visualizations (apache#4727)" This reverts commit de0aaf4.
mistercrunch
added
🏷️ bot
This PR adds lazy loading to all visualizations to partially address #4705 to reduce JS bundle size. This reduces the
exploreanddashboardbundle sizes by half, with the idea that the user would then fetch visualization bundles as needed, depending on what's present on a dashboard or as they toggle vis types in explore.We want to do some empirical testing with some of our internal dashboards next week, and will update then. In the meantime feel free to pull the branch or comment on the code.
before
after
@graceguo-supercat @mistercrunch