-
Notifications
You must be signed in to change notification settings - Fork 17
perf: fetch the Genesis world manifest concurrently with realm /about #9540
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -27,6 +27,13 @@ public class WorldManifestProvider | |||||||||||||||||||
| private const string dclWorldName = "dcl.eth"; | ||||||||||||||||||||
|
|
||||||||||||||||||||
| private WorldManifest? cachedMainManifest; | ||||||||||||||||||||
| private UniTask<WorldManifest>? inFlightMainManifest; | ||||||||||||||||||||
|
|
||||||||||||||||||||
| // Genesis manifest URLs are compile-time constants, so this runs concurrently with | ||||||||||||||||||||
| // the realm /about request instead of waiting on realmName. Idempotent; Preserve() | ||||||||||||||||||||
| // lets the Genesis branch await this same task rather than issue a second request. | ||||||||||||||||||||
| public void PrefetchGenesisManifest(DecentralandEnvironment environment, CancellationToken ct) => | ||||||||||||||||||||
| inFlightMainManifest ??= FetchGenesisManifestAsync(environment, ct).Preserve(); | ||||||||||||||||||||
|
Comment on lines
+35
to
+36
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [P1] Stale task after cancellation poisons genesis manifest for the rest of the session.
This is especially insidious because Fix: Guard on cache hit and pending status instead of
Suggested change
|
||||||||||||||||||||
|
|
||||||||||||||||||||
| public WorldManifestProvider(IWebRequestController webRequestController) | ||||||||||||||||||||
| { | ||||||||||||||||||||
|
|
@@ -38,7 +45,10 @@ public async UniTask<WorldManifest> FetchWorldManifestAsync(URLDomain assetBundl | |||||||||||||||||||
| try | ||||||||||||||||||||
| { | ||||||||||||||||||||
| if(MAIN_REALM_NAMES.Contains(realmName)) | ||||||||||||||||||||
| return await FetchGenesisManifestAsync(environment, ct); | ||||||||||||||||||||
| { | ||||||||||||||||||||
| PrefetchGenesisManifest(environment, ct); | ||||||||||||||||||||
| return await inFlightMainManifest!.Value; | ||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [P2] Null-forgiving operator
Suggested change
|
||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| if(realmName.EndsWith(dclWorldName)) | ||||||||||||||||||||
| return await FetchNonGenesisManifestAsync(assetBundleRegistry, realmName, ct); | ||||||||||||||||||||
|
|
||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[P2] Comment narrates caller/external behavior (CLAUDE.md §11 anti-patterns). "runs concurrently with the realm /about request" and "lets the Genesis branch await this same task" describe what callers do, not what this method guarantees. The caller pattern can change without this code changing, silently making the comment wrong.