Repository navigation
fix(runtime-core): performance - prevent re-registrations when not using share scopes - #5062
MatissJanis wants to merge 5 commits into
Conversation
…are consumptions loadShare and loadShareSync called initializeSharing without an initScope, so the init-token guard operated on a fresh array on every call and never short-circuited. Each eager shared consumption re-ran registration over the entire host share table and emitted afterRegisterShare for every entry, costing hundreds of milliseconds of main-thread time per page load on hosts with large share tables. - keep a per-instance shareInitScope so sharing initialization runs only once per share scope across loadShare/loadShareSync calls - skip beforeRegisterShare/afterRegisterShare emissions when no plugin listens
🦋 Changeset detectedLatest commit: 3ee03be The changes in this PR will be included in the next version bump. This PR includes changesets to release 48 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 05c4d4dac6
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
…esettable Follow-up to the share re-registration guard: - cache per-scope remote-initialization promises so concurrent loadShare calls hitting the init-token guard await remote share registration instead of resolving against the share map too early - forget a failed initialization so the next loadShare retries instead of replaying the cached rejection forever - resetShareInit() when registerRemotes registers remotes, so a later version-first loadShare re-initializes the scope and the new remote's shares participate in version selection - no-op resetShareInit on DisabledSharedHandler for remotes-enabled + shared-disabled runtimes
|
Codex recommended doing some changes. I have applied them, however full disclosure - we have not deployed and tested them in prod. I do think they make sense though and should be safe to apply. Let me know if further changes are wanted! |
|
Hey @ScriptedAlchemy any chance you could review this one? We're seeing a MASSIVE performance improvement in DataDog on slower machines with this change. It would be a big win for the MF platform too I believe. |
|
There is a mixed-strategy regression in the persistent scope guard that needs to be addressed before merging. When two shared dependencies use different strategies within the same share scope, the first consumer now determines whether remote initialization runs for subsequent consumers. For example, with host-level // shared.react.strategy = 'loaded-first'
// shared.lodash.strategy = 'version-first'
await mf.loadShare('react');
await mf.loadShare('lodash');The first call pushes the scope token without initializing remotes. The second call hits the early return in As a result, shares from remotes that have not already initialized are absent from version selection. For example, if the host provides lodash 4.17.20 and an uninitialized remote provides a compatible 4.17.21, the consumer can silently use the local version instead of letting the remote version participate. I reproduced this against the current PR head ( Could you preserve the one-time host share registration optimization while ensuring that a later |
…rs hitting the share init guard When a loaded-first consumer first initializes a share scope, the persistent init-token guard early-returns the cached remote-init promises to later consumers. A later version-first consumer therefore never initialized the scope's remotes, so remote-provided shares never joined version selection. The persistent-scope-owning run now captures its remote-initialization procedure, and a guard hit that needs version selection (empty cached promises + version-first) invokes it, combining the new promises with the cached ones. Failed guard-path initialization is forgotten so the next loadShare retries.
|
Thanks @2heal1! I have pushed a patch for this, but please give me a few days to test this in prod. Will report back with the perf data on my side once I have this running in prod. (feel free to review in the meantime if you prefer that) |
…d remote-init promises Recursive/nested container initialization passes its own initScope down. When such a foreign scope already contains the init token, the caching branch returned the owning run's remote-init promises; in a circular remote graph those promises resolve only when the in-flight remote finishes, deadlocking shared-module loading. Guard the cache path with initScope === this.shareInitScope so foreign scopes fall through to the old safe behavior of returning their local promises array. Flagged by agentic review on the web-ui port (ddoghq/web-ui#17022).
|
Deploying this to prod was a success. All performance metrics are stable at the previous levels (which means up to 50% better than the tagged latest release of module federation). Feel free to review! :) |
This branch has not been deployed
2 participants
Description
When share scope is not used - the default empty array is used. This caused the init-guard to be invoked on every call and never short circuited. For large repos with many shared dependencies - this adds a substantial amount of CPU time.
In DataDog (>17m LOC) we observed FCP and LCP going down by as much as 50% when applying this patch.
Related Issue
n/a
I understand that you only accept PRs that are linked to issues, but I hope an exception could be made here. 🥺
Types of changes
Checklist
I have updated the documentation.- not applicable IMO