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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[meta] Kibana loading performances optimizations #62263

Closed
15 of 17 tasks
pgayvallet opened this issue Apr 2, 2020 · 25 comments
Closed
15 of 17 tasks

[meta] Kibana loading performances optimizations #62263

pgayvallet opened this issue Apr 2, 2020 · 25 comments
Labels
Meta performance Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Team:Operations Team label for Operations Team

Comments

@pgayvallet
Copy link
Contributor

pgayvallet commented Apr 2, 2020

Starting with 7.7, there has been a significant increase in the Kibana loading time (#62238 for example)

This issue is here to discuss with @elastic/kibana-platform and @elastic/kibana-operations to list the possible improvements that could be done

We got some work in progress regarding some possible optimizations from the platform team:

Plan for 7.9 release:

Plan for 7.8 release:

@pgayvallet pgayvallet added Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Team:Operations Team label for Operations Team Meta labels Apr 2, 2020
@pgayvallet
Copy link
Contributor Author

pgayvallet commented Apr 2, 2020

To start the discussion, I got a couple questions regarding the NP plugins optimizer: (apologies in advance if any or all of them have already been discussed numerous times)

  • Are the NP plugins also using the packages/kbn-ui-shared-deps common bundle, or is this exclusive to legacy plugins?
  • I see some widely used deps not present in this kbn-ui-shared-deps list (rxjs or lodash i.e) Wouldn't add them reduce the overall plugin bundles size?
  • From what I understand, there is no longer any common (splitChunks) bundle for NP plugins? If that's true, as (at least first party) plugin are using the same version of their dependencies (right?), what was the reason to not at least generate a common chunk restricted to node_modules commons?
  • Should NP plugins be able to use webpack's code splitting (using async/lazy require) to reduce their initial bundle size, or is chunk splitting not supported atm?

@mshustov
Copy link
Contributor

mshustov commented Apr 2, 2020

http://localhost:5601/app/kibana#/home load time (with disabled cache)

  • v7.6
    2020-04-02_10-26-10 2
    v7.6 HAR file

  • v7.7
    2020-04-02_10-31-58
    v7.7 HAR file

  • Is there a way to get webpack stats from '@kbn/optimizer` to diagnose what dependencies bloat artifact size?

  • Do we monitor the size of distributable artifacts? Can we add it at least on CI?

@spalger
Copy link
Contributor

spalger commented Apr 2, 2020

  • Are the NP plugins also using the packages/kbn-ui-shared-deps common bundle, or is this exclusive to legacy plugins?

This is exclusive to legacy plugins as it is created by webpack analyzing the modules in each bundle and requires that each plugin use a separate bundle. This is not how the new platform builds work, in oder to make them plug-and-play they need to be totally independent so they're made by different webpack instances and therefor can't be chunked in the same way.

  • I see some widely used deps not present in this kbn-ui-shared-deps list (rxjs or lodash i.e) Wouldn't add them reduce the overall plugin bundles size?

Yes, probably, but we created @kbn/ui-shared-deps for dependencies that must have only one instance and not as a performance optimization. That said, I also noticed that the ui-shared-deps dist is over 20mb now, so I'm experimenting with splitting that file up and might be able to justify moving a couple but packages into that bundle (as long as we are fine forcing version consistency across the project)

  • From what I understand, there is no longer any common (splitChunks) bundle for NP plugins? If that's true, as (at least first party) plugin are using the same version of their dependencies (right?), what was the reason to not at least generate a common chunk restricted to node_modules commons?

That is true, and the main reason is because we need to isolate the build steps for the different plugins, which means there can't be a webpack instance that is organizing modules into different chunks by detecting repetitive uses. As mentioned, I'm looking into ways we can elevate enormous dependencies like the legacy elasticsearch client and maybe rxjs, since we don't currently have any reason to allow multiple versions of these modules.

  • Should NP plugins be able to use webpack's code splitting (using async/lazy require) to reduce their initial bundle size, or is chunk splitting not supported atm?

Yes, this works and @restrry is working on making sure that it's used in more places. We could probably use help with finding patterns that we can spread around to different plugin teams. Part of #16085 will be raising awareness of how big certain plugins are and how they're impacting Kibana load times.

  • Is there a way to get webpack stats from '@kbn/optimizer` to diagnose what dependencies bloat artifact size?

If you run node scripts/build_kibana_platform_plugins you can run @kbn/optimizer independently and pass flags to it, including the --profile flag which will write stats.json files in the target/public directories of each plugin.

  • Do we monitor the size of distributable artifacts? Can we add it at least on CI?

No, but that is my primary goal for 7.8.

@pgayvallet
Copy link
Contributor Author

That is true, and the main reason is because we need to isolate the build steps for the different plugins, which means there can't be a webpack instance that is organizing modules into different chunks by detecting repetitive uses

My webpack knowledge starts to be rusty, but can't this be resolved with a dll? Wouldn't that still allow to have isolated per-plugin builds (even if it requires a preliminary step before any plugin build to generate the dll)?

Should NP plugins be able to use webpack's code splitting
Yes, this works

This is excellent news. We really need to identify pattern / best practice and push that to the other teams. The first obvious usage would be to use split points into applications' mount functions.

@mshustov
Copy link
Contributor

mshustov commented Apr 3, 2020

This is excellent news. We really need to identify pattern / best practice and push that to the other teams. The first obvious usage would be to use split points into applications' mount functions.

Examples in the migration guide use async imports. What we can improve:

  • to update migration guide to use async imports within management section mount function
  • establish a convention on declaring async mounters (a filename/separate folder for them, passed arguments, etc.) and document it

@mshustov
Copy link
Contributor

mshustov commented Apr 6, 2020

updated migration guide with code splitting examples #62593

@joshdover
Copy link
Contributor

@spalger Is there any reason we can't use Webpack's public path feature instead of the public path replacements we do at bundle serve time?

I don't know if that processing is actually a bottleneck, but I wouldn't be surprised if it consumes a non-trivial amount of CPU and delays the serving of the bundle. Would be worth benchmarking with and without that processing to see.

@spalger
Copy link
Contributor

spalger commented Apr 18, 2020

Yeah, if we use the webpack feature we have to embed the basePath in the bundles and reoptimize when the basePath is changed. This is how we used to do things.

I do think we could run the replacement once and store the result to disk when the basePath changes rather than doing the replacement at request time, at least if we have the ability to write to the file system.

@mshustov
Copy link
Contributor

Can we leverage __webpack_public_path__ to inject basePath on a page load?
https://webpack.js.org/guides/public-path/#on-the-fly

@joshdover
Copy link
Contributor

Yeah I was hoping we could just pull it from a global variable on window instead. It’s not clear whether or not this is supported from the docs.

@pgayvallet
Copy link
Contributor Author

It’s not clear whether or not this is supported from the docs.

Not sure if it's a real supported API, however I've been using the __webpack_public_path__ in most of my webpack bundled apps and it does work properly.

@mshustov
Copy link
Contributor

mshustov commented Apr 20, 2020

How bad the problem is?

We don’t know since we don’t yield any real user metrics. Any development time parameters could be misleading, and we can spend time fixing the wrong problem.

Action points:

  • create a list of metrics to optimize
  • decide what metrics value are considered acceptable
  • automate perf metrics gathering
    • setup infrastructure to run gathering perf metrics manually (puppeteer + lighthouse, ie)
    • run perf tests on CI. Several tests in the same environment to get a reliable result.
    • setup reporting for every PR

Fixing root causes

Number of parallel requests to load plugin bundles

Problem
Each plugin is shipped with a separate js & CSS bundle, which slow down a page loading time due to the number of concurrent request limitation in a browser. The mechanic is described in the sub-task #55241
Action points:

  • concatenate CSS bundles
  • concatenate js bundles
  • load bundles eagerly

Open questions:

  • Do it in runtime or at build time?
  • Should we load all the plugins or enabled only?
  • How to maintain parity for prod & dev builds?
  • What file size limitations can we stumble into?

Alternatives to discuss:

  • use another cache invalidation policy instead of etag
  • use HTTP2

Bloated js artifacts

Problem
Bundles shipped with unused code that a browser has to load and parse.
Action points:

Problem
Kibana uses not the more aggressive compression algorithm.
Action points:

Problem
Plugins tend to ship all the code in a common bundle. This creates unnecessary load on the network and CPU, as the user may never use all the functionality of the loaded plugin.
Action points:

  • audit plugin bundles and creates issues for solution teams to use lazy loading strategy whenever possible: app mount, management section mount (maps, spaces, others merged after 7.7 FF), load data sets via REST API

Problem
Platform loads legacy platform code when bootstraps Kibana platform application.
Action points:

Problem
Bundles contain repetitive dependencies. We extracted some packages into the kbn-shared-deps-ui package, but this work is done manually. Therefore, this cannot guarantee that there will be no duplicate dependencies in the future.
Different bundles could contain different versions of the same package.
Examples:
maps --> @elastic/ems-client --> lodash
maps --> lodash

Packages import the whole library, even only some components are needed. There was a suggestion to use https://www.npmjs.com/package/babel-plugin-transform-imports to change import paths allowing only used components to be imported.

Open questions:

  • Do we want to deal with these duplications?

Risks

  • We may not have time to make all the listed improvements before FF on May 7th. Conclusion: we must focus on low hanging fruits giving us a big win.
  • Significant changes in infrastructure may not be tested if there is little time left before FF. conclusion: start testing before the FF date.
  • Over the next releases, performance will gradually degrade as more plugins migrate to the Kibana platform. Conclusion: we should automate perf metrics checks, can be done after FF.
  • The metrics collected on CI will be synthetic and will not reflect the state of affairs of end-users. Conclusion: we should start monitoring real users to understand the real situation. Can it be done after FF?

@tylersmalley
Copy link
Contributor

Something to note; separate CSS and common/vendor files are only from the legacy platform.

@joshdover
Copy link
Contributor

use another cache invalidation policy instead of etag

This may be one of the best 80/20 solutions right now. The strategy I've seen used before is to include a timestamp (set to the last modified date of the file) or a version number in the URL search parameters and then set cache-control to max-age=864000 instead of must-revalidate.

This great eliminates the number of requests the browser needs to make since it does not need to re-fetch the etag header from the server. The cache busting mechanism just becomes the query parameter which we can ensure changes anytime the file changes.

This does not help the cold start performance, but may have a big impact on every subsequent page load. Could be easily tested without having to write much code.

If we went this route, the easiest option would be to only use this strategy for bundles built by the new optimizer since we know those never change in production.

@mshustov
Copy link
Contributor

mshustov commented Apr 21, 2020

Plan for 7.8 release:

@joshdover
Copy link
Contributor

Looked into using different compression, specifically, the brotli algo.

One thing about this compression is that it is quite slow on the compression side, so it’s only useful if you have static content that you can compress ahead of time.

Due to the “replace public path” processing we do at request time, this is not an option for us. We will need to figure out how to remove this processing by enabling webpack to dynamically look this up at runtime.

@tylersmalley
Copy link
Contributor

As discussed in our sync today - there is an opportunity to also remove the bootstrap.js file as we should be able to do this in the main page template.

@joshdover
Copy link
Contributor

As discussed in our sync today - there is an opportunity to also remove the bootstrap.js file as we should be able to do this in the main page template.

I can take a look at this, though I'm not optimistic it will make a big difference.

I'm also experimenting with making the public path dynamic to unblock using brotli. The complicated part is injecting the __webpack_public_path__ = declaration into each entry point.

@spalger
Copy link
Contributor

spalger commented Apr 22, 2020

As discussed in our sync today - there is an opportunity to also remove the bootstrap.js file as we should be able to do this in the main page template.

I’m fairly confident that we use bootstrap.js for CSP reasons, but I’m not totally sure.

I'm also experimenting with making the public path dynamic to unblock using brotli. The complicated part is injecting the __webpack_public_path__ = declaration into each entry point.

Could we just set window.__webpack_public_path__ in bootstrap.js? I’m happy to help identify how we might be able to get this working if you want to brainstorm

@joshdover
Copy link
Contributor

Could we just set window.webpack_public_path in bootstrap.js? I’m happy to help identify how we might be able to get this working if you want to brainstorm

Unfortunately, I don't think so. This variable isn't a global variable, it's a webpack "free variable" that must be in the compiled code and is set in each webpack runtime.

Another trick thing is that this code variable should be set before any other code executes, and since you cannot execute code before ES imports, we have to add an import to the top of the entry point module that points to module that sets this.

My initial idea is to add a Webpack plugin or loader that appends an import to each entry point to import a synthetic module that just contains __webpack_public_path__ = window.__kbnBasePath__. I haven't quite started on this yet, but @spalger if you have any pointers for modifying modules before they're compiled, that'd be great. From what I've read it's important where in the webpack lifecycle this gets added, because if it's too late in the process it will no be picked up by the Webpack magic.

@joshdover
Copy link
Contributor

joshdover commented Apr 22, 2020

I've put up a PR for setting the public path in the JS runtime: #64226

This unblocks brotli compression which I will work on as a follow up.

@vigneshshanmugam
Copy link
Member

vigneshshanmugam commented Apr 30, 2020

Sorry for jumping in to the issue, Thought I might give a hand/provide some info as I have some prior experience working on this area.

is going to change a cache invalidation mechanism to switch from etag to hash-based filenames with max-age header to avoid a round trip per cached asset.

Max agent would definitely work in most of the cases but not on reload as browser still hits the server and validates the cache of the resource. Could we also add immutable value along in the lines of max-age to the Cache control header to speed up things for reload cases.

cache-control: max-age:<secs>; immutable

You can read more about the immutable caching in this article

collect performance metrics on CI

Happy to help here in any way I can to capture more Metrics. We have a really good setup in running benchmarks with our Real User Monitoring agent and we not only capture the bundle size, but also capture CPU time spent by the bundle, Memory contribution of some of the hot functions on the agent's code. Some details are in this Benchmarks readme. Happy to go over the setup if it's of interest.

The metrics collected on CI will be synthetic and will not reflect the state of affairs of end-users. Conclusion: we should start monitoring real users to understand the real situation. Can it be done after FF?

We have the APM agents running already on dev. Will it make sense to extend it for our users as well?

Love this initiative. Kudos to the team 🎉

@jbudz
Copy link
Member

jbudz commented Apr 30, 2020

great stuff here. is this limited to our targets for 7.8? a few somewhat out of the theme options below. more in the possibly worth investigating brain dump category than confirmed. i can put these somewhere else, don't want to clutter.

ui

  • thread blocking (discover table, bundle parsing, painting, lazy loading)

http

  • blocking ui xhr (waiting for username from api/security/me)
  • blocking server xhr (is license valid -> is user authorized -> query1 -> migrate response -> process -> query2 -> process)
  • xhr size. responses to the browser limited to what the ui needs to draw. possibly w/ scrolling.
  • xhr + http slow start. large requests or small requests with less in between
  • background requests - e.g. less load on es potential for quicker responses. i can provide a wireguard dump if anyone's interested, there's a lot of noise.
  • request pipeline limits, mentioned above. either combining css, adding http2 support, or both
  • generate index.html <scripts> on the server for - 1 round trip. i think this may be addressed already

npm

  • not side effect free
  • partial cleanups (kbn-ui-framework),
  • partially frameworks (not anymore but e.g. import full angular.ui for date picker)
  • version merging (mostly by upgrading to latest on our side)
  • version deduplication(https://github.com/atlassian/yarn-deduplicate))
  • different but same, e.g. react state, fetch

assets

  • caniuse. remove safari7, eventually ie
  • image compression (separate from content-encoding) and resizing
  • extract text. can license notices can be shipped but separate? not sure if that's a legal option
  • mangling. i think we have it disabled due to the memory requirements, but with immutable bundles we could revisit

@mshustov
Copy link
Contributor

mshustov commented May 4, 2020

Could we also add immutable value along in the lines of max-age to the Cache control header to speed up things for reload cases.

@vigneshshanmugam immutable is not supported by Chromium-based browsers and most likely is not going to be supported

We have the APM agents running already on dev. Will it make sense to extend it for our users as well?

Do you collect metrics for the dev environment only? Is there a dashboard with these metrics available?

@pgayvallet
Copy link
Contributor Author

All listed improvements up to 7.9 have been addressed. Closing, for further/additional points, independent issues should be opened.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Meta performance Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Team:Operations Team label for Operations Team
Projects
None yet
Development

No branches or pull requests

7 participants