FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Try lazy-loading widgets in Lab by bollwyvl · Pull Request #1192 · bqplot/bqplot · GitHub

/ bqplot Public

Try lazy-loading widgets in Lab - #1192

Merged
martinRenou merged 10 commits into
bqplot:masterfrom
bollwyvl:add-lazy-load-lab
May 31, 2022
Merged

Try lazy-loading widgets in Lab#1192
martinRenou merged 10 commits into
bqplot:masterfrom
bollwyvl:add-lazy-load-lab

Conversation

bollwyvl commented Aug 12, 2020
edited
Loading

Copy link
Copy Markdown
Contributor

This uses the approach from pythreejs to defer loading the widgets until one is actually requested, which should shave nearly 30% off of the built lab. Because threejs is so large, it may also be worth deferring that, too, if possible, as it doesn't appear its usage is guaranteed if someone is using one of the simpler plots.

better still would be to arrive at a common, compatible packaging of threejs, which could be shared by all consumers (e.g. pythreejs, bqplot, ipyvolume) but I have no idea what the compatibility windows are like for all of them.

2021-06: sorry i let this slide for so long. I've rejiggered a few things to make working with package.json a bit more up-to-date with typescript conventions (weird as they are) with the composite build. Would love to see this land to get release along with #1379.

It's probably worth taking a hard look at the sharedPackages (e.g. three.js, underscore) as every extra copy of those heavies we don't have to load n times in e.g. jupyterlite makes me 😀

Copy link
Copy Markdown
Member

Jlab automatically deduplicates threejs if the versions overlap. That's part of the big reason for the build step.

Copy link
Copy Markdown
Contributor Author

great, that's progress, but still built into vendor~main is still quite brutal. Working through it locally, will have some results in a tick...

Copy link
Copy Markdown
Contributor Author

Just because i looked:

ipyvolume    "three": "^0.97.0",
bqplot       "three": "^0.91.0",
pythreejs    "three": "^0.97.0",

So, hooray 🎉!

Copy link
Copy Markdown
Contributor Author

Here's the situation as of 8b1935e:

1.2M Aug 12 16:26 vendors~bqplot.a07da31a0075a039c91a.js
4.4M Aug 12 16:26 vendors~main.881efab41eb56f4e4f00.js

I don't actually know where threejs went, as I can't see it in stats, but it's working fine:

bollwyvl changed the title [wip] Try lazy-loading widgets in Lab Try lazy-loading widgets in Lab Aug 12, 2020

Copy link
Copy Markdown
Contributor Author

Of additional concern is the extra copy of @lumino/widgets... but I assume there's a good, known reason for it.

Copy link
Copy Markdown
Member

I don't actually know where threejs went

Perhaps it was split out to a different file now? Those choices are made automatically at build time, so adding an extension may lead to some different division.

The extension system rework in jlab 3 leads to more things being split out (and shared)

Of additional concern is the extra copy of @lumino/widgets... but I assume there's a good, known reason for it.

There shouldn't be two copies of lumino widgets, as far as I know, so I think that is indeed a cause for concern.

bollwyvl commented Aug 12, 2020 via email
edited
Loading

Copy link
Copy Markdown
Contributor Author

Copy link
Copy Markdown
Contributor Author

On the duplicate widgets: that was already appearing in the downstream where i found the issue, but the two copies appeared in main (of course)... having it split out has just isolated the symptom, I'm fairly sure I didn't create it.

For reference, a vanilla build only includes the index.es6.js

bollwyvl commented Aug 13, 2020
edited
Loading

Copy link
Copy Markdown
Contributor Author

I tried a couple alternate import mechanisms to get at the "real" MessageLoop/Widget, and it's always still pulling its own copy in. While I think it would be possible to get at the Widget class from some part of @jupyter-widgets/base or controls or something, I don't see a way to get at MessageLoop, as it's basically only ever used statically. Seems like some of these things could be re-exported/normalized in an upstream... it hardly seems like "just an implementation detail" at this point...

anyhow: approaches tried

  • import the whole module to get closer to the regex spec
    • no change
  • peerDependencies on phosphor
    • no change
  • try to get the Widget/MessageLoop out of base/controls
    • can't figure out how

Copy link
Copy Markdown
Contributor Author

Yeah, fiddled with some other stuff, no change. I don't think it can be solved from this side, short of changing the phosphumino dependency, which i assume is done to avoid having to maintain separate releases. I'm not entirely sure it's worth fixing this in Lab2 if it hasn't actually been creating problems...

Copy link
Copy Markdown
Contributor Author

As i haven't had to fire up the webpack visualizer in ages (thank GOODNESS), here's a simpler table of the load impact:

I've gotta think reducing initial load to 2 requests of ~30k on page setup vs all 11 of ~ 1.3mb is worth the hassle.

exports: bqplot,
name,
version,
exports: async () => import('./index'),

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Is this the key feature of this PR? (thinking about doing the same in other widgets lib)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

yep, this is the key thing.

bollwyvl commented Jun 30, 2021 via email

Copy link
Copy Markdown
Contributor Author

Copy link
Copy Markdown
Contributor Author

Have repushed with various updates: it builds/runs locally on Ubuntu, and the tests pass on chromium 98.

guess i'd like to see what CI says before sinking any more time!

Copy link
Copy Markdown
Member

Thanks @bollwyvl, just approved the CI

Copy link
Copy Markdown
Contributor Author

@martinRenou thanks!

it's pleasing it worked right off the bat. I really don't think anything in there constitutes an even noticeable change to downstreams, but certainly welcome feedback. I'll have a look at some of those other ones!

martinRenou added this to the 0.13.0 milestone Mar 3, 2022
bollwyvl requested a review from martinRenou May 31, 2022 13:26

Copy link
Copy Markdown
Member

Thanks!!

martinRenou merged commit 2e781d7 into bqplot:master May 31, 2022
bollwyvl deleted the add-lazy-load-lab branch December 8, 2022 11:39
This file contains hidden or 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Lazy-loading in lab

3 participants


Back | FazBrowse Home | New Git URL