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

Use a single require-in-the-middle hook #178285

Merged
merged 5 commits into from
Mar 13, 2024

Conversation

pgayvallet
Copy link
Contributor

@pgayvallet pgayvallet commented Mar 8, 2024

Summary

Doing investigations on Kibana startup time, we discovered that the usage of require-in-the-middle has some significant performance drawback for initial start / module load time.

After investigating, we discovered that the perf hit of the library is directly related to the number of hooks that are being defined (as the patching logic is executed once PER hook, surprisingly).

I will be opening an issue upstream to discuss about the problem, but for now, we'll be working around it from there.

This PR brings some perf improvements by changing the way we're using the lib, and using a single hook now delegating to the various patching methods instead of creating one hook per module patch.

Perf gain

Tests were only performed locally (we will confirm in other environments once the PR is merged), but the results are quite encouraging.

Without the PR

  • Process startup: ~2000ms
  • Preboot: ~5000ms
  • Setup: ~6000ms
  • Start: ~1000ms

With the PR

  • Process startup: ~1650ms
  • Preboot: ~4100ms
  • Setup: ~5150ms
  • Start: ~1000ms (no changes, expected given we don't load any module there)

So TLDR: ~18% in term of initial code loading time.

Thanks to @dgieselaar and @rudolf for the group effort on the investigation

@pgayvallet pgayvallet added Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc release_note:skip Skip the PR/issue when compiling release notes v8.14.0 startup-time labels Mar 8, 2024
@pgayvallet
Copy link
Contributor Author

/ci

@pgayvallet
Copy link
Contributor Author

pgayvallet commented Mar 8, 2024

Tests are failing because of elastic/require-in-the-middle#79, I guess we will have to fix that upstream before going forward

@pgayvallet
Copy link
Contributor Author

/ci

@pgayvallet pgayvallet marked this pull request as ready for review March 13, 2024 11:46
@pgayvallet pgayvallet requested a review from a team as a code owner March 13, 2024 11:46
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-core (Team:Core)

@pgayvallet pgayvallet enabled auto-merge (squash) March 13, 2024 14:50
@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

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

@pgayvallet pgayvallet merged commit 4a806b6 into elastic:main Mar 13, 2024
33 checks passed
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Mar 13, 2024
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 release_note:skip Skip the PR/issue when compiling release notes startup-time Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v8.14.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants