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

[ml] Optimize bundles, reduce page load async chunks by 74% #179311

Merged

Conversation

clintandrewhall
Copy link
Contributor

@clintandrewhall clintandrewhall commented Mar 24, 2024

Summary

The AnomalyExplorerChartsService was importing the SWIM_LANE_LABEL_WIDTH numerical constant from swimlane_container.tsx. As a result, the entirety of that file was being included in the async bundle.

AnomalyExplorerChartsService is loaded asynchronously on page load by the embeddable. By relocating the constant to its own file-- as well as other optimizations (see below)-- we reduce the async page load by 74%, (in dev mode).

Before - 351k

Screenshot 2024-03-24 at 10 09 31 AM

After - 93.4k

Screenshot 2024-04-05 at 11 45 45 AM

Unfortunately, this change led to a bloating of async modules, the cause of which is still unclear. The application async chunk weighed in at 2.2 MB compressed! To get this PR to a shippable state, I refactored additional code to bring down duplication and bloat.

The result is an ml experience that fetches small bundles on demand as someone interacts with it:

Apr-05-2024 11-51-18

More work can be done to continue to optimize the plugin, of course, but this set of changes is an excellent start, (and case study on bundle load).

Other optimizations in this PR

  • Registration of some start services are conditional, and contain their own async calls. I've removed these from the register helper, (which itself is a brute-force offload of code from the plugin, but is still loaded every time), and loaded them async if the conditions apply.
  • Routing in ml use factories to create individual routes. In a lot of cases, the pages these routes displayed were not asynchronously loaded, adding tremendous amounts of React code to the root application.
    • I made all pages loaded by routes async.
    • In some cases, the components themselves were colocated with the route factory. I moved those to their own files for async import.
    • Similarly, some state managers were colocated. Those were moved to their own files, as well.
  • Moved flyouts, modals, expanding rows to React.lazy async modules, (using dynamic from @kbn/shared-ux-utility.
  • Refactored export * from directives from public/shared.ts to accurately reflect what is being consumed outside the ml plugin, (and also reduce the size of that bundle.
  • Refactored lodash imports to submodule imports to enable tree-shaking, (if ever enabled in webpack).
  • Moved getMlGlobalServices off of the app.tsx file for import by others, (to avoid depending on the rest of the code there).
  • Moved some types to /common to avoid importing code, (though, admittedly, types are compiled away). But it felt cleaner to move them out.

@clintandrewhall clintandrewhall added review loe:small Small Level of Effort release_note:skip Skip the PR/issue when compiling release notes impact:high Addressing this issue will have a high level of impact on the quality/strength of our product. Team:ML Team label for ML (also use :ml) ci:build-webpack-bundle-analyzer v8.14.0 labels Mar 24, 2024
@clintandrewhall clintandrewhall requested a review from a team as a code owner March 24, 2024 14:30
@clintandrewhall clintandrewhall force-pushed the ml/anomaly_edmbeddable_perf branch from c172f04 to f1f06e6 Compare March 24, 2024 16:45
Copy link
Contributor

@darnautov darnautov left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for spotting and improving this!

@darnautov
Copy link
Contributor

@elasticmachine merge upstream

@clintandrewhall
Copy link
Contributor Author

@darnautov I can't merge this, unfortunately.

For a very strange reason I haven't been able to identify, this change causes several chunks to bloat to 2MB each. It appears code gets copied for each one.

Screenshot 2024-03-25 at 11 48 16 PM Screenshot 2024-03-25 at 11 48 46 PM

I've only been able to fix it by outright removing the explorer routes from the plugin, (by commenting it out).

My only guess is that because a number of the files pertaining to that collection of routes are still in JS, there may be a circular or other dependency. But I haven't been able to hunt it down, after a day of trying.

Perhaps @mistic or @elastic/kibana-operations can assist. But this is a crazy effect from moving a simple constant.

@clintandrewhall
Copy link
Contributor Author

Another update-- I've run all the permutations, and all of the changes in this PR are fine with only one exception:

If swimlane_container does not export SWIM_LANE_LABEL_WIDTH, and AnomalyExplorerChartsService does not import it from that location, the chunks explode.

Unfortunately, that appears to to be the change needed to see the performance gains in the browser.

@clintandrewhall
Copy link
Contributor Author

I believe I've found the cause and the solution to the async module bloat: a utility function was defined in ml/public/application/app.tsx. This module, however, is responsible for rendering the entire application.

Importing the single utility from that file brought all of the other code with it. By moving it to its own module, we decoupled the utility and prevented the rest of the application code from being bundled with the async module.

@clintandrewhall clintandrewhall force-pushed the ml/anomaly_edmbeddable_perf branch 4 times, most recently from 927dfbf to 5b3c7e8 Compare March 26, 2024 20:48
@darnautov darnautov self-requested a review March 27, 2024 09:23
@clintandrewhall clintandrewhall force-pushed the ml/anomaly_edmbeddable_perf branch 2 times, most recently from 6ec6a38 to 022c61c Compare April 5, 2024 04:52
@clintandrewhall clintandrewhall requested review from a team as code owners April 5, 2024 05:26
@clintandrewhall clintandrewhall force-pushed the ml/anomaly_edmbeddable_perf branch from db32220 to ea4267f Compare April 5, 2024 05:28
@clintandrewhall clintandrewhall removed request for a team April 5, 2024 05:29
@clintandrewhall clintandrewhall changed the title [ml] Relocate constant, reduce page load async chunks by 59% [ml] Optimize bundles, reduce page load async chunks by 59% Apr 5, 2024
@clintandrewhall clintandrewhall changed the title [ml] Optimize bundles, reduce page load async chunks by 59% [ml] Optimize bundles, reduce page load async chunks by 74% Apr 5, 2024
@darnautov darnautov self-requested a review April 5, 2024 16:47
Copy link
Contributor

@darnautov darnautov left a comment

Choose a reason for hiding this comment

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

LGTM 🚀 many thanks @clintandrewhall!
Async chunks are significantly smaller now. Reorganising code by getting rid of some deep imports and introducing async imports for routes are the great enhancements 🙏🏻

@clintandrewhall clintandrewhall enabled auto-merge (squash) April 11, 2024 16:04
@darnautov
Copy link
Contributor

@elasticmachine merge upstream

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
aiops 490 487 -3
ml 1980 1983 +3
total -0

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
ml 65 66 +1

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
aiops 412.1KB 411.5KB -595.0B
dataVisualizer 653.0KB 652.9KB -50.0B
ml 3.7MB 4.1MB ⚠️ +399.5KB
transform 392.1KB 392.0KB -47.0B
total +398.8KB

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
ml 78.5KB 76.5KB -1.9KB
Unknown metric groups

API count

id before after diff
ml 151 152 +1

async chunk count

id before after diff
ml 45 99 +54

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@clintandrewhall clintandrewhall merged commit 67ab4f7 into elastic:main Apr 11, 2024
18 checks passed
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Apr 11, 2024
@clintandrewhall clintandrewhall deleted the ml/anomaly_edmbeddable_perf branch April 11, 2024 20:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting ci:build-webpack-bundle-analyzer impact:high Addressing this issue will have a high level of impact on the quality/strength of our product. loe:small Small Level of Effort release_note:skip Skip the PR/issue when compiling release notes review Team:ML Team label for ML (also use :ml) v8.14.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants