fix: stop GPU Instancer blocking the main thread on startup - #10111
Conversation
🚦 CI StatusWindows and Mac built successfully in Unity Cloud.
Warnings count reduced: 11910 => 11903 Warnings/errors in files changed by this PR (12)Lint run · full InspectCode report · took 23m 51s All Unity tests passed ✅
Tests time sums the test cases; Job time is the job's wall clock including checkout, licensing and asset import. Slowest tests
Full report: run summary · results + editor logs: editmode · playmode 🏁 Bare-metal benchmark finished — run #35020229630. Full reportPR #10111, run #35020229630 Overall: 🟢 GPU 1% worst improved on Intel Core i5 Builds: Windows change, Windows baseline, macOS change, macOS baseline How to read this table
Apple M1
Intel Core i5
On demand — comment |
decentraland-bot
left a comment
decentraland-bot
left a comment
There was a problem hiding this comment.
Summary
What: Pre-loads GPUIShaderBindings asynchronously via the project's assetsProvisioner and assigns it to GPUIShaderBindings.Instance before the GPUI rendering system is created, eliminating a synchronous WaitForCompletion() call that blocks the main thread during startup.
Step 2 — Root-cause check: PASS ✅
The fix addresses the actual root cause. The ANR occurs because GPUIShaderBindings.Instance getter internally calls Addressables.LoadAssetAsync<T>().WaitForCompletion(), which synchronously completes ALL in-flight async loads — not just the requested asset. By pre-loading and assigning the asset before any code reads Instance, the synchronous path never executes. This is a genuine bypass of the blocking load, not a symptom relocation.
Step 3 — Design & integration: PASS ✅
Owner search: GPUICoreAPI.RegisterRenderer — the call that triggers GPUIRenderingSystem.Awake() → GPUIShaderBindings.Instance getter — is called only in LandscapePlugin.InitializeAsync() (line 97). All other GPUICoreAPI usage is in TreeData.cs for buffer/instance-count operations that don't create the rendering system. GPUIRenderingSystem has zero hits outside the vendored GPUI package. GPUI is exclusively Landscape-scoped. LandscapePlugin is the natural and sole owner of this preload.
enableLandscape guard: The fix is placed after if (!enableLandscape) return; (line 79). When landscape is disabled, RegisterRenderer is also never called, so no code path triggers the synchronous load. Correct.
Static singleton write: GPUIShaderBindings.Instance is a third-party library static (GPUInstancerPro), not a project container. CLAUDE.md's "plugins read from containers, don't write" rule targets the project's own StaticContainer/DynamicWorldContainer graph. Setting a vendor singleton that the vendor's own init reads is an acceptable pragmatic workaround — there is no project container to hoist it into.
No new long-lived units introduced. The change adds a field to an existing plugin, a typed asset reference (standard pattern), and a serialized settings field.
Step 4 — Member audit
No new public properties or accessors on existing types. GPUIShaderBindingsRef is a new public type — a trivial AssetReferenceT<GPUIShaderBindings> subclass following the established LandscapeDataRef pattern in the same file. Used only by Unity serialization.
Step 5 — Findings
One inline finding (P2, see inline comment).
Categories checked clean:
- R1 (alloc): No hot-path allocations — runs once during plugin init ✅
- R2 (LINQ): No LINQ ✅
- R3 (struct/class): No struct-class mixing ✅
- R4 (ECS): No ECS queries or systems modified ✅
- R5 (Entity by-ref): N/A ✅
- R6 (teardown):
gpuiShaderBindingsfollows the same undisposedProvidedAsset<T>pattern aslandscapeData— pre-existing, not introduced by this PR ✅ - R7 (nullability): Annotations consistent ✅
- R8 (root cause): Genuine fix, not symptom relocation ✅
- R9 (logging): No new logging ✅
- R10 (catch-scope): No try/catch ✅
- R11 (cancellation):
CancellationTokenproperly threaded throughProvideMainAssetAsync✅ - R12 (abstraction): No new interfaces or wrappers ✅
- R13 (reuse): Uses existing
assetsProvisionerandProvidedAsset<T>patterns ✅ - R14–R15 (contract/dead weight): N/A ✅
- R16–R17 (naming): Names follow conventions (
GPUIShaderBindingsRefmatches the*Reffamily) ✅ - R18 (magic numbers): None ✅
- R19 (file hygiene):
GPUIShaderBindingsRefco-located withLandscapeDataReffollows existing convention ✅ - R20 (.NET idioms): Field can't be
readonly— assigned in async init, not constructor ✅ - R21 (MVC): N/A ✅
- R22 (tests): No regression test. The ANR depends on timing of concurrent Addressables loads — inherently non-deterministic.
LandscapePluginhas no existing test coverage. Manual test plan covers functional verification ✅ - R24 (scope): All files directly related to the stated change ✅
Lint pre-flight: scripts/lint/custom-rules.sh not present on this branch — rules checked by hand.
Step 6 — Complexity: SIMPLE
Touches 4 files with ~20 lines of meaningful changes. Adds an async asset preload to an existing plugin's initialization — follows existing patterns exactly. Does not modify ECS systems, async flow patterns, or plugin registration.
Step 7 — QA: YES
Changes runtime code that ships in the build. Affects GPUI tree rendering — could impact what the user sees.
Step 8 — Non-blocking warnings
None. Main scene not modified.
Merge gates
- R25 (comments): No outstanding review comments from prior rounds. This is the first review.
- R26 (QA): Awaiting QA. PR affects rendering (landscape trees). Test instructions in PR body: verify trees render with same density/LOD, verify no GPUI warnings in log. Needs verification on both Windows and Mac.
REVIEW_RESULT: PASS ✅
COMPLEXITY: SIMPLE
COMPLEXITY_REASON: Adds an async asset preload to LandscapePlugin's existing initialization flow — no ECS, no new systems, no architectural changes.
QA_REQUIRED: YES
Reviewed by Jarvis 🤖 · Requested by decentraland-bot via GitHub
decentraland-bot
left a comment
decentraland-bot
left a comment
There was a problem hiding this comment.
Summary
What: Pre-loads GPUIShaderBindings asynchronously via the project's assetsProvisioner and assigns it to GPUIShaderBindings.Instance before the GPUI rendering system is created, eliminating a synchronous WaitForCompletion() call that blocks the main thread during startup.
Cleanup (commit 2): member reordering in LandscapeData, removal of unused OnDetailDistanceChanged event, [UsedImplicitly] on enum values, null! initializers on serialized fields.
Step 2 — Root-cause check: PASS ✅
The fix addresses the actual root cause. The ANR occurs because GPUIShaderBindings.Instance getter calls Addressables.LoadAssetAsync<T>().WaitForCompletion(), which synchronously completes ALL in-flight async loads. By pre-loading and assigning the asset before GPUICoreAPI.RegisterRenderer triggers the getter, the synchronous path never executes. This is a genuine root-cause fix, not a symptom relocation.
Step 3 — Design & integration: PASS ✅
Owner search: GPUICoreAPI.RegisterRenderer — the call that triggers GPUIRenderingSystem.Awake() → GPUIShaderBindings.Instance getter — is called first in LandscapePlugin.InitializeAsync() (line 97). GPUInstancingPlugin does not call RegisterRenderer; it only passes LandscapeData to GPUInstancingService. LandscapePlugin is the correct and sole owner of this preload.
enableLandscape guard: The fix is placed after if (!enableLandscape) return; (line 79). When landscape is disabled, RegisterRenderer is never called, so the synchronous load never triggers. Correct placement.
Static singleton write: GPUIShaderBindings.Instance is a third-party library static (GPUInstancerPro), not a project container. CLAUDE.md's "plugins read from containers, don't write" rule targets project containers. Setting a vendor singleton is an acceptable pragmatic workaround.
No new long-lived units introduced. The change adds a field to an existing plugin, a typed asset reference (standard *Ref pattern), and a serialized settings field.
Teardown trace: gpuiShaderBindings stores a ProvidedAsset<GPUIShaderBindings> holding an Addressables handle. The existing landscapeData field of the same type is also never explicitly disposed — both are "main assets" that outlive any single plugin. assetsProvisioner manages their lifecycle. Consistent with the established pattern.
Step 4 — Member audit
GpuiShaderBindingsRef— typed Addressable reference followingLandscapeDataRefpattern. Used by Unity serialization. No audit issue.gpuiShaderBindings(field) —.Valueused once to set the staticInstance, but the field must persist to keep the Addressables handle alive (prevents asset unloading). Follows the existinglandscapeDatapattern.OnDetailDistanceChangedremoved — confirmed zero consumers via repo-wide search. Dead code removal.
Step 5 — Findings
One inline finding (P1 R23). See inline comment.
Categories checked clean:
- R1 (alloc): No hot-path allocations — runs once during plugin init ✅
- R2 (LINQ): No LINQ ✅
- R4 (ECS): No ECS queries or systems modified ✅
- R6 (teardown):
gpuiShaderBindingsfollows the same undisposedProvidedAsset<T>pattern aslandscapeData— pre-existing convention, not introduced by this PR ✅ - R7 (nullability):
null!initializers on serialized fields are correct (Unity-managed). Annotations consistent ✅ - R8 (root cause): Genuine fix, not symptom relocation ✅
- R10 (catch-scope): No try/catch ✅
- R11 (cancellation):
CancellationTokenproperly threaded throughProvideMainAssetAsync✅ - R12 (abstraction): No new interfaces or wrappers ✅
- R13 (reuse): Uses existing
assetsProvisionerandProvidedAsset<T>patterns ✅ - R15 (dead weight): Commented-out code (lines 41, 61, 119, 126) is pre-existing, not added by this PR ✅
- R17 (naming families):
GpuiShaderBindingsReffollows*Refsuffix convention (LandscapeDataRef) ✅ - R18 (magic numbers): None ✅
- R19 (file hygiene):
GpuiShaderBindingsRefco-located withLandscapeDataRef— follows existing convention ✅ - R20 (.NET idioms): Field can't be
readonly— assigned in async init, not constructor ✅ - R22 (tests): No regression test.
LandscapePluginhas zero existing test coverage. The ANR depends on timing of concurrent Addressables loads — inherently non-deterministic. The fix is two sequential lines with trivially correct ordering ✅ - R24 (scope): Lint cleanup isolated in its own commit, same files, non-behavioral. Acceptable ✅
Lint pre-flight: scripts/lint/custom-rules.sh not present on this branch — rules checked by hand above.
Merge gates
- R25 (comments): 1 open inline thread from the prior review round (P1 R23 at
LandscapePlugin.cs:85— comment narrates external behavior). Must be addressed before merge. - R26 (QA): Awaiting QA. PR affects rendering (landscape trees via GPUI). Test instructions in PR body cover tree rendering and GPUI log warnings. Needs verification on both Windows and Mac.
REVIEW_RESULT: PASS ✅
COMPLEXITY: SIMPLE
COMPLEXITY_REASON: Adds an async asset preload to LandscapePlugin's existing initialization flow — no ECS, no new systems, no architectural changes.
QA_REQUIRED: YES
Reviewed by Jarvis 🤖 · Requested by decentraland-bot via GitHub
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Ludmilafantaniella
left a comment
Ludmilafantaniella
left a comment
There was a problem hiding this comment.
QA Update
Verified on Windows and Mac. Checked landscape/tree rendering around Genesis Plaza, at the Genesis City borders, Casa Roustan and in ryumi.dcl.eth, rowan.dcl.eth and cozyfarm.dcl.eth worlds.
Trees render with the same density and LOD distances as before, no missing shader bindings or replacement material warnings in the log.
Approved from QA side. ✅
10111-evi.mp4
✅Smoke test performed:
- ✔️ Backpack and wearables in world
- ✔️ Emotes in world and in backpack
- ✔️ Teleport with map/coordinates/Jump In
- ✔️ Chat and multiplayer
- ✔️ Profile card
- ✔️ Skybox
3 participants
What
Fixes a pre-in-world ANR cluster — 6 of 200 sampled events
(UNITY-EXPLORER-PBX).
Problem
The first
GPUICoreAPI.RegisterRenderercall createsGPUIRenderingSystem, and itsAwakereadsGPUIShaderBindings.Instance. That getter loads the asset with AddressablesWaitForCompletion()— the one Addressables API that is explicitly synchronous.WaitForCompletion()can't wait for just its own asset. It makes Unity finish every async loadalready in flight before returning, so this one small request freezes the main thread for as long
as the slowest unrelated load takes.
Fix
Load the asset ourselves and hand it to GPUI before the rendering system exists:
With
GPUIShaderBindings.Instancealready set, that code never runs — the load isn't made faster, it doesn't happen.Testing