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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

lazy load vega-embed #4706

Merged
merged 16 commits into from Jul 7, 2018
Merged

lazy load vega-embed #4706

merged 16 commits into from Jul 7, 2018

Conversation

@bollwyvl
Copy link
Contributor

@bollwyvl bollwyvl commented Jun 10, 2018

This moves the load of vega-embed into the renderModel method, which should ensure that the 1MB of vega-related stuff isn't loaded unless a user asks for it. This one was convenient because of the existing Promise stuff in RenderModel.

However, it looks like those bits are still getting loaded into the load-time vendor bundle, rather than a new vega bundle, as i would have expected. Anyhow, opening this to get feedback while i investigate further...

馃敆 #4695 #4702

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Jun 12, 2018

For now, can we use the require.ensure syntax instead of the newer import syntax? I think we are just moving to es2015 modules, not esnext modules, so we still can't use import.

@domoritz
Copy link
Member

@domoritz domoritz commented Jun 15, 2018

To avoid merge conflicts, it would be nice if we can merge #4661 first.

@bollwyvl
Copy link
Contributor Author

@bollwyvl bollwyvl commented Jun 15, 2018

Please excuse the delay on getting back on this!

To avoid merge conflicts, it would be nice if we can merge #4661 first.

Sure, no hurry here, happy to rebase, etc. Heck, if you wanted to just take the (rather minimal) code from here and land it there, that would be even better from my laziness perspective!

can we use the require.ensure syntax instead of the newer import syntax

require.ensure works like a champ, but for the life of me i couldn't get @types/webpack-env to play nice with @types/node. I've pushed a version with a bit of any casting, and am seeing this in my build:
screenshot from 2018-06-14 20-39-17

So I'm thinking it looks like the rule of thumb might be "is it bigger than codemirror?" as to whether a dependency is "big" and should trigger a lazy load. Anyhoo, on to numbers...

master

                                   Asset      Size  Chunks                    Chunk Names
       vega~main.6c6420135a192f937426.js   520 KiB       3  [emitted]  [big]  vega~main
    674f50d287a8c48dc19ba404d20fe713.eot   162 KiB          [emitted]         
  af7ae505a9eed503f8b8e6982036873e.woff2  75.4 KiB          [emitted]         
    912ec66d7572ff821749319396470bde.svg   434 KiB          [emitted]  [big]  
   fee66e712a8a08eef5805a46892932ad.woff  95.7 KiB          [emitted]         
               0.694c1bea111cef5ab380.js   440 KiB       0  [emitted]  [big]  
               1.f07d1dd71ae3c4532523.js  3.59 KiB       1  [emitted]         
    vendors~main.84a7b30667cb9270093a.js  2.25 MiB       2  [emitted]  [big]  vendors~main
    b06871f281fee6b241d60582ae9369b9.ttf   162 KiB          [emitted]         
            main.f61640d735a02f93ebdf.js   756 KiB       4  [emitted]  [big]  main

this

                                   Asset       Size  Chunks                    Chunk Names
    vendors~main.7f72cbc2540c6055c365.js   2.03 MiB       4  [emitted]  [big]  vendors~main
    674f50d287a8c48dc19ba404d20fe713.eot    162 KiB          [emitted]         
  af7ae505a9eed503f8b8e6982036873e.woff2   75.4 KiB          [emitted]         
    912ec66d7572ff821749319396470bde.svg    434 KiB          [emitted]  [big]  
   fee66e712a8a08eef5805a46892932ad.woff   95.7 KiB          [emitted]         
               0.694c1bea111cef5ab380.js    440 KiB       0  [emitted]  [big]  
    vendors~vega.ce6fd61dd7a4d846d03e.js    744 KiB       1  [emitted]  [big]  vendors~vega
            vega.6d375955045d44688db8.js  156 bytes       2  [emitted]         vega
               3.1c3d204ab3ad7bf950b7.js   3.59 KiB       3  [emitted]         
    b06871f281fee6b241d60582ae9369b9.ttf    162 KiB          [emitted]         
            main.3f32f8229b20d22ff040.js    756 KiB       5  [emitted]  [big]  main

The tiny vega.6...8.js is weird, with unfulfilled shims, but that 700k (+200k stolen from the main vendors) isn't hitting the browser until well down the rendering chain, and doesn't block the moonimation from clearing. I'd say clawing back almost 100ms from the load time milestones on localhost is pretty good, even if (apparently) the first vega render on the page takes more time. Unscientific screenshots:

master
screenshot from 2018-06-14 20-49-18

this
screenshot from 2018-06-14 20-55-46

Will revisit when #4661 lands...

@bollwyvl
Copy link
Contributor Author

@bollwyvl bollwyvl commented Jun 15, 2018

Oooh, and the tests worked :)

@jasongrout jasongrout added this to the Beta 3 milestone Jun 27, 2018
@bollwyvl
Copy link
Contributor Author

@bollwyvl bollwyvl commented Jun 29, 2018

now awaiting #4806

@bollwyvl
Copy link
Contributor Author

@bollwyvl bollwyvl commented Jun 30, 2018

Merged in master, everything's looking fine:

screenshot from 2018-06-29 19-59-23

@bollwyvl
Copy link
Contributor Author

@bollwyvl bollwyvl commented Jun 30, 2018

Sizes:

                                   Asset       Size  Chunks                    Chunk Names
    vendors~main.a70de201cd6d40eba5f6.js   2.05 MiB       4  [emitted]  [big]  vendors~main
    674f50d287a8c48dc19ba404d20fe713.eot    162 KiB          [emitted]         
  af7ae505a9eed503f8b8e6982036873e.woff2   75.4 KiB          [emitted]         
    912ec66d7572ff821749319396470bde.svg    434 KiB          [emitted]  [big]  
   fee66e712a8a08eef5805a46892932ad.woff   95.7 KiB          [emitted]         
               0.8e18d3a8a2a11bdb38b3.js    441 KiB       0  [emitted]  [big]  
    vendors~vega.9325ae64f1bd957cb790.js    729 KiB       1  [emitted]  [big]  vendors~vega
            vega.6d375955045d44688db8.js  156 bytes       2  [emitted]         vega
               3.1c3d204ab3ad7bf950b7.js   3.59 KiB       3  [emitted]         
    b06871f281fee6b241d60582ae9369b9.ttf    162 KiB          [emitted]         
            main.aac0597bdd210d38ef01.js    788 KiB       5  [emitted]  [big]  main

@ian-r-rose
Copy link
Member

@ian-r-rose ian-r-rose commented Jun 30, 2018

Nice, thanks @bollwyvl!

@bollwyvl
Copy link
Contributor Author

@bollwyvl bollwyvl commented Jun 30, 2018

Appveyor fail was just flakiness. Rekicked.

@bollwyvl
Copy link
Contributor Author

@bollwyvl bollwyvl commented Jun 30, 2018

Thoughts on unwrapping the callback 馃敟 馃懣 馃嵈

/**
   * Render Vega/Vega-Lite into this widget's node.
   */
  renderModel(model: IRenderMime.IMimeModel): Promise<void> {
    const data = model.data[this._mimeType] as JSONObject;
    const metadata = model.metadata[this._mimeType] as { embed_options?: VegaModuleType.EmbedOptions };
    const embedOptions = metadata && metadata.embed_options ? metadata.embed_options : {};
    const mode: VegaModuleType.Mode = this._mimeType === VEGA_MIME_TYPE ? 'vega' : 'vega-lite';
    const options = {
      actions: true,
      defaultStyle: true,
      ...embedOptions,
      mode,
    };

    let vega: typeof VegaModuleType;

    return this._ensureVega()
      .then(_vega => { vega = _vega; })
      .then(() => this._resolver.resolveUrl(''))
      .then(path => this._resolver.getDownloadUrl(path))
      .then(baseURL => {
        const el = document.createElement('div');
        // clear the output before attaching a chart
        this.node.innerHTML = '';
        this.node.appendChild(el);
        return vega.default(el, data, {
          ...options,
          loader: {
            baseURL,
            http: { credentials: 'same-origin' }
          }
        });
      })
      .then(result => {
        if (!model.data['image/png']) {
          // Add png representation of vega chart to output
          return result.view.toImageURL('png');
        }
      })
      .then(imageData => {
        if (imageData != null) {
          const data = { ...model.data, 'image/png': imageData.split(',')[1] };
          model.setData({ data });
        }
      });
  }

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Jun 30, 2018

Don't forget that you can use async/await syntax, which should make that look a lot neater :).

@bollwyvl
Copy link
Contributor Author

@bollwyvl bollwyvl commented Jun 30, 2018

OH WE CAN????!!!???

@bollwyvl
Copy link
Contributor Author

@bollwyvl bollwyvl commented Jun 30, 2018

Yeah, the async/await is a few lines longer, but the lack of indentation makes things a lot clearer.

/**
* Lazy-load the vega-embed library.
*/
private _ensureVega(): Promise<typeof VegaModuleType> {
Copy link
Contributor Author

@bollwyvl bollwyvl Jun 30, 2018

Choose a reason for hiding this comment

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

I tried to unwrap this with awaits, but webpack is picky about how it should work, so leaving for now.

@saulshanabrook
Copy link
Member

@saulshanabrook saulshanabrook commented Jul 2, 2018

I have had luck using the lighthouse chrome tool before to run some performance tests on different build scenarios. It even lets you simulate different network conditions. I opened an issue to track adding some form of this to the repo, maybe even run on ci (#4825)

Copy link
Member

@blink1073 blink1073 left a comment

Thanks!

@bollwyvl
Copy link
Contributor Author

@bollwyvl bollwyvl commented Jul 3, 2018

@@ -2,14 +2,15 @@
| Copyright (c) Jupyter Development Team.
| Distributed under the terms of the Modified BSD License.
|----------------------------------------------------------------------------*/
/// <reference path="../../../node_modules/@types/webpack-env/index.d.ts"/>
Copy link
Contributor Author

@bollwyvl bollwyvl Jul 4, 2018

Choose a reason for hiding this comment

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

I couldn't find a better way to get this into scope...

/**
* A namespace for private module data.
*/
namespace Private {
Copy link
Contributor Author

@bollwyvl bollwyvl Jul 4, 2018

Choose a reason for hiding this comment

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

This seemed like the most common pattern for weird global stuff... thoughts?

Copy link
Member

@blink1073 blink1073 Jul 6, 2018

Choose a reason for hiding this comment

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

馃憤馃徏

}

vegaReady = new Promise((resolve, reject) => {
require.ensure(
Copy link
Contributor Author

@bollwyvl bollwyvl Jul 4, 2018

Choose a reason for hiding this comment

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

Hooray, the /// directive makes this work... most of it is still untyped though because of how crazy this syntax is...

@bollwyvl
Copy link
Contributor Author

@bollwyvl bollwyvl commented Jul 6, 2018

Hooray back to green!

Would love to see this land: there's a lot of great stuff coming up that would do very well to be lazy loaded!

@@ -111,13 +111,7 @@ module.exports = {
},
optimization: {
splitChunks: {
chunks: 'all',
cacheGroups: {
Copy link
Contributor Author

@bollwyvl bollwyvl Jul 6, 2018

Choose a reason for hiding this comment

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

Figured this was worth doing now.

Copy link
Member

@blink1073 blink1073 left a comment

Looks great, thanks!

@blink1073 blink1073 merged commit afedbf9 into jupyterlab:master Jul 7, 2018
2 checks passed
@bollwyvl bollwyvl mentioned this pull request Jul 10, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Aug 8, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants