fix: evict only the changed GLTF on LSD hot reload instead of draining every cache - #9667
Conversation
…ng every cache The dev server's UpdateModel websocket message already names the exact model that changed (src + path-derived hash), but the client discarded it and force-drained every cache on each reload. Consume it: in raw-GLTF local development, evict just that model (parsed import + pooled container instances) and keep every other cache warm across the reload. Non-model changes (textures, code, scene.json) still arrive as a coarse updateScene with no file information, so they keep the conservative full drain, as does --local-ab mode where the model lives in the asset-bundle caches (guarded via ComposeCacheKey, mirroring the container cache key). Measured on a real scene (editor, LSD): GLB save reload 2.4s -> 1.5s median, with the win growing on heavy models (3.4s -> 1.6s) since unchanged assets no longer re-download, re-parse or re-instantiate. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
🚦 CI Status
Windows and Mac build successful in Unity Cloud! You can find a link to the downloadable artifact below. Warnings count reduced: 13676 => 13659 Warnings/errors in files changed by this PR (20)All Unity tests passed ✅
|
decentraland-bot
left a comment
decentraland-bot
left a comment
There was a problem hiding this comment.
PR Review — opti: evict only the changed GLTF on LSD hot reload instead of draining every cache
Step 2 — Root-cause check: PASS ✅
The problem is real: the sdk-commands dev server's UpdateModel websocket message already names the exact GLTF that changed (src + hash), but the client discarded this information and force-drained every cache on each hot reload. The diff fixes the cause (ignoring the per-model message) rather than a symptom: it consumes the UpdateModel payload, evicts only the named asset, and leaves every other cache warm. The conservative full-drain fallback is correctly preserved for all cases where scoped eviction is unsafe (non-model changes, --local-ab mode, missing model info).
Step 3 — Design & integration: PASS ✅
Lifecycle owner search:
ChangedGltfModel— a readonly struct carrying two strings from the websocket message to the reload logic. Not a long-lived unit; it's a data carrier with no lifecycle of its own. Appropriate.ICacheCleaner.EvictGltfModel— placed onCacheCleaner, which already owns references togltfContainerAssetsCacheandgltfLoadCacheand knows how toUnloadCache(). This is the natural home for scoped cache eviction. ✅IStreamableCache.Remove/IGltfContainerAssetsCache.Remove— added to the existing cache interfaces that already ownUnload().RemovemirrorsUnloadscoped to one key — same owner, same abstraction level. ✅IsRawGltfModel— static helper onECSReloadScenethat guards the scoped-eviction branch. UsesSceneEntityDefinition.AssetBundleManifestVersionOrFailed.ComposeCacheKey(hash)to determine whether the hash addresses a raw GLTF or an asset-bundle-wrapped model. This is tightly coupled to the reload decision logic;ECSReloadSceneis the right home. ✅
No new long-lived units are introduced. The new methods are extensions of existing owners (CacheCleaner, the cache interfaces). No duplicate lifecycle reconciliation, no per-frame scanning, no persistent state outside ECS.
Teardown trace: The eviction calls (Remove) dispose assets synchronously at the point of call. No subscriptions, callbacks, or connections are opened. GltfContainerAssetsCache.Remove disposes every pooled GltfContainerAsset under the key, clears the list, removes from the unload queue, and clears irrecoverable failures. RefCountStreamableCacheBase.Remove checks CanBeDisposed() (respecting refcounts) before disposing. Both are complete teardown paths. ✅
Step 4 — Member audit: PASS ✅
| Member | Consumers | Notes |
|---|---|---|
IStreamableCache.Remove(key) |
CacheCleaner.EvictGltfModel (via gltfLoadCache) |
Default interface method (no-op); overridden in RefCountStreamableCacheBase. Legitimate interface extension mirroring Unload. |
RefCountStreamableCacheBase.Remove(key) |
Via interface dispatch | Correctly mirrors Unload scoped to one key, honoring refcounts. |
IGltfContainerAssetsCache.Remove(key) |
CacheCleaner.EvictGltfModel |
Required interface method; implemented by GltfContainerAssetsCache and test mock. |
GltfContainerAssetsCache.Remove(key) |
Via interface dispatch | Disposes all pooled assets, updates profiling counter, clears cache/queue/failures. |
ICacheCleaner.EvictGltfModel(hash, src) |
ECSReloadScene.DisposeAndRestartAsync |
Single consumer. Legitimate — it's a specialized operation on the cache cleaner triggered by a specific event. |
ECSReloadScene.IsRawGltfModel(definition, hash) |
DisposeAndRestartAsync, ECSReloadSceneShould (tests) |
Internal static, well-tested. Encapsulates the asset-bundle guard logic. |
ChangedGltfModel struct |
LocalSceneDevelopmentController, ECSReloadScene |
Simple data carrier between websocket parsing and reload logic. |
No single-use derived predicates that should be merged. No re-derived values. ✅
Step 5 — Line-level findings
See inline comment(s) below.
Step 6 — Complexity: COMPLEX
Touches cache unloading/eviction paths (IStreamableCache, RefCountStreamableCacheBase, GltfContainerAssetsCache, CacheCleaner), scene lifecycle (ECSReloadScene), and asset loading pipeline interfaces. Modifies shared interfaces used across assemblies.
Step 7 — QA assessment: YES
Modifies runtime code that affects scene loading behavior during local scene development. User-facing impact: reload speed and asset correctness during hot reload.
Step 8 — Non-blocking warnings
semantic / title-matches-convention is failing. The PR title prefix opti: is not a recognized semantic commit type. Consider renaming to perf: (performance optimization) to match the project's branch & PR standards.
Security review
No security issues found. The websocket data originates from the local dev server (same-machine, developer-controlled). No user input reaches external systems. No secrets, credentials, or sensitive data exposure. No auth/authz changes.
REVIEW_RESULT: PASS ✅
COMPLEXITY: COMPLEX
COMPLEXITY_REASON: Modifies cache eviction paths (IStreamableCache, RefCountStreamableCacheBase, GltfContainerAssetsCache, CacheCleaner) and scene lifecycle (ECSReloadScene) — core asset loading and memory management subsystems.
QA_REQUIRED: YES
Reviewed by Jarvis 🤖 · Requested by decentraland-bot via GitHub
pravusjif
changed the title
pravusjif
left a comment
pravusjif
left a comment
There was a problem hiding this comment.
Currently the case or changing a model in runtime is only having an effect on the 2nd hot-reload (this case).
It can be tested as follows:
1 - start locally this test scene and change the Animator.create(.. call to just Animator.create(shark) so that it automatically plays any animation on the model
2 - While the scene is running, connected to this PR Explorer, replace the shark.glb file in the scene with another model that has been renamed to have the exact same filename (e.g. 1Monster.glb1 from this other scene
3 - The hot-reload happens, but the model doesn't change (the cached one is still served), the actual model update happens on a 2nd hot-reload (change anything in the code to trigger that 2nd hot reload and confirm that)
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…on LSD scoped eviction The UpdateModel websocket message's hash is minted from the watcher-relative path, while every cache key derives from the content-mapping hash minted from the absolute path — they never match, so the eviction removed a key that was never cached and the edited model kept loading stale. Join the changed file to the definition's content list by src to get the real hash, and evict the parsed-import cache by hash alone (its identity includes the verbatim src casing from scene code, which the watcher cannot reproduce). The message's hash is no longer used at all, so the carrier struct collapses to the src string. Repro: replacing models/shark.glb's bytes with another model kept showing the shark; now the new content loads. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
pravusjif
left a comment
pravusjif
left a comment
There was a problem hiding this comment.
LGTM, now it works as expected in the previously mentioned case.
Screen.Recording.2026-08-10.at.5.36.32.PM.mp4
decentraland-bot
left a comment
decentraland-bot
left a comment
There was a problem hiding this comment.
PR Review: fix: evict only the changed GLTF on LSD hot reload instead of draining every cache
STEP 2 — Root-cause check: PASS
The problem is clear: the dev server's UpdateModel websocket message already identifies the exact model that changed, but the client discarded that info and force-drained every cache on each hot reload. This PR consumes the message's src field to scope the eviction. This is a genuine fix for the root cause — not a symptom workaround.
STEP 3 — Design & integration: PASS
No new long-lived units introduced. All changes extend existing owners:
CacheCleaneralready ownsUnloadCache(full drain). AddingEvictGltfModel(scoped drain) is consistent with its single responsibility as the cache eviction coordinator.ECSReloadScenealready owns the reload decision logic. The newTryResolveContentHashandIsRawGltfModelinternal statichelpers serve that decision and live with it.LocalSceneDevelopmentControlleralready parses websocket messages and calls the reload. ExtractingchangedModelSrcfromUpdateModelis naturally placed here.
Owner search: The caches being evicted (GltfContainerAssetsCache, GltfLoadCache) already have their lifecycle managed by CacheCleaner via the existing Register() / UnloadCache() contract. The new Remove methods are scoped versions of the existing Unload, consistent with each cache's existing disposal pattern.
Teardown/consumption trace:
GltfContainerAssetsCache.Remove: disposes pooled assets, clears list, removes fromcachedict +unloadQueue, clearsIrrecoverableFailures. Complete cleanup — no leak.RefCountStreamableCacheBase.Remove: checksCanBeDisposed()(ref count ≤ 0), disposes, removes fromcachedict +listedCache. The scene is fully disposed (SceneState.Disposed) before eviction runs, so references should be released. Silent no-op if ref count > 0 — consistent withUnloadbehavior.GltfLoadCache.RemoveByHash: iterateslistedCachebackwards callingRemoveper match. Safe: backwards iteration + per-itemRemoveAtpreserves indices below the removal point; unique entries per key (ensured byTryAddinAdd).
STEP 4 — Member audit
| New member | Consumers | Assessment |
|---|---|---|
IGltfContainerAssetsCache.Remove(in string key) |
CacheCleaner.EvictGltfModel (1) |
Scoped counterpart to Unload; appropriate interface addition |
IStreamableCache.Remove(in TLoadingIntention key) |
Default no-op; overridden by RefCountStreamableCacheBase |
Avoids breaking all existing implementations; single override is fine |
GltfLoadCache.RemoveByHash(string hash) |
CacheCleaner.EvictGltfModel (1) |
GltfLoadCache-specific: full key is (Name, Hash) but caller only has hash |
ICacheCleaner.EvictGltfModel(string hash) |
ECSReloadScene.DisposeAndRestartAsync (1) |
Complements existing UnloadCache |
ECSReloadScene.TryResolveContentHash |
DisposeAndRestartAsync + tests (2) |
Internal static helper; tested |
ECSReloadScene.IsRawGltfModel |
DisposeAndRestartAsync + tests (2) |
Internal static helper; tested |
All new members are appropriately scoped. No single-use indirection or merge candidates.
STEP 5 — Line-level findings
See inline comments below.
Security review
No security issues found. The UpdateModel.Src string from the websocket is used only for a case-insensitive string.Equals lookup against ContentDefinition.file entries — never as a file path, shell argument, or interpolated into any unsafe operation.
STEP 8 — Non-blocking warnings
None. Main.unity is not modified.
REVIEW_RESULT: PASS ✅
COMPLEXITY: COMPLEX
COMPLEXITY_REASON: Touches GLTF cache eviction pipeline, scene lifecycle reload, and multiple cache hierarchies across assemblies
QA_REQUIRED: YES
Reviewed by Jarvis 🤖 · Requested by Juan Ignacio Molteni [Dalkia] (<@U03JSUQ5Z7U>) via Slack
|
🔍 Jarvis reviewed this PR and found no blocking issues, but assessed it as complex — human DEV review is still required before merging. |
The local dev server file watcher reports the changed model with the platform path separator, while content mappings always spell paths with '/'. On Windows the watcher sends "assets\models\out\models\Bench.glb" against a mapping entry of "assets/models/out/models/Bench.glb", so the ordinal join in TryResolveContentHash never matched: the hash stayed unresolved and every Windows reload silently fell back to the full cache drain. Scoped eviction has therefore never been active on Windows, while macOS matched by luck since its separator is already '/'. Compare paths in place, treating '\' and '/' as equal and ignoring case, keeping the reload path allocation-free. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-authored-by: Muna <44584806+decentraland-bot@users.noreply.github.com> Signed-off-by: Juan Ignacio Molteni <juanignaciomolteni@gmail.com>
|
Given that we want it on the next release, I got @anicalbano okay to merge and do the final test there. I have validated working on both platforms |
decentraland-bot
left a comment
decentraland-bot
left a comment
There was a problem hiding this comment.
Re-review — fix: evict only the changed GLTF on LSD hot reload instead of draining every cache
Changes since last Jarvis review
Three commits landed after the previous review (2026-08-10T16:37):
| Commit | What changed |
|---|---|
7d6e06bf |
fix: match LSD changed-model paths regardless of platform separator — ContentPathEquals normalizes \ → / and compares case-insensitively, keeping the reload path allocation-free. Windows LSD scoped eviction was silently broken. |
80156d7b |
Merge dev |
79cc5001 |
Applied previous review suggestion: as GltfLoadCache → is GltfLoadCache typedCache in CacheCleaner.EvictGltfModel |
Previous review findings status
| # | Finding | Status |
|---|---|---|
| 1 | [P2] IsRawGltfModel — hash should be string? |
|
| 2 | [P2] as cast → is pattern in EvictGltfModel |
✅ Applied in 79cc5001 |
| 3 | [P2] Remove returning bool |
Dismissed by author — scene is fully disposed before eviction |
STEP 2 — Root-cause check: PASS
Unchanged from previous review. The PR correctly consumes the UpdateModel websocket message to scope eviction to the changed model, fixing the root cause (discarding per-model information and draining every cache).
STEP 3 — Design & integration: PASS
No new long-lived units introduced by the new commits. ContentPathEquals is a private static helper on ECSReloadScene — the natural home since it serves TryResolveContentHash. No lifecycle duplication, no per-frame reconciliation, no subscription leaks.
STEP 4 — Member audit
| New member (since last review) | Consumers | Notes |
|---|---|---|
ContentPathEquals(string?, string) |
TryResolveContentHash (1) |
Private static helper, allocation-free. Single-use but encapsulates non-trivial separator + case normalization logic — extraction is justified. |
STEP 5 — Line-level findings
See inline comments below. Both are the same nullability pattern: a non-nullable parameter guarded with string.IsNullOrEmpty, violating CLAUDE.md NRT rules.
STEP 6 — Complexity: COMPLEX
Unchanged — touches GLTF cache eviction paths and scene lifecycle across assemblies.
STEP 7 — QA: YES
Unchanged — modifies runtime code affecting scene loading during local development.
STEP 8 — Non-blocking warnings
None. Main.unity not modified.
Security review
No issues found. The websocket Src string is used only for case-insensitive string comparison against content definitions, then the resolved hash (from the definition's own data) is used as a cache key. No file I/O, shell execution, or external calls with the input.
REVIEW_RESULT: PASS ✅
COMPLEXITY: COMPLEX
COMPLEXITY_REASON: Touches GLTF cache eviction pipeline (IStreamableCache, RefCountStreamableCacheBase, GltfContainerAssetsCache, GltfLoadCache, CacheCleaner) and scene lifecycle (ECSReloadScene) across assemblies
QA_REQUIRED: YES
Reviewed by Jarvis 🤖 · Requested by Juan Ignacio Molteni [Dalkia] (<@U03JSUQ5Z7U>) via Slack
| /// which returns the bare hash only in that case; under <c>--local-ab</c> the key differs and | ||
| /// the model lives in the asset-bundle caches instead, so scoped eviction must not be used. | ||
| /// </summary> | ||
| internal static bool IsRawGltfModel(SceneEntityDefinition? definition, string hash) |
There was a problem hiding this comment.
[P2] Nullability annotation (repeat from previous review) — hash is declared string (non-nullable) but is null-checked via string.IsNullOrEmpty. Per CLAUDE.md anti-patterns: "Defensive null-checks against non-null declarations — if the declared type is T (not T?), don't null-check it." Since null is a legitimate defensive case (the test in ECSReloadSceneShould exercises it), the parameter should honestly declare string?. This also lets the test pass null directly instead of null!.
| internal static bool IsRawGltfModel(SceneEntityDefinition? definition, string hash) | |
| internal static bool IsRawGltfModel(SceneEntityDefinition? definition, string? hash) |
| /// derives from the content-mapping hash (minted from the absolute path) — the two never | ||
| /// match, so the file must be joined to the definition's content list by its src instead. | ||
| /// </summary> | ||
| internal static bool TryResolveContentHash(SceneEntityDefinition? definition, string src, out string hash) |
There was a problem hiding this comment.
[P2] Nullability annotation — same pattern as IsRawGltfModel: src is declared non-nullable string but guarded with string.IsNullOrEmpty(src). The caller in DisposeAndRestartAsync passes a string? (changedModelSrc) that flows through a null-check before reaching here, so in practice src is always non-null at the call site — but the method's own guard implies it accepts null. Either remove the IsNullOrEmpty guard on src (trusting the non-nullable contract) or declare string? to match the actual behavior.
| internal static bool TryResolveContentHash(SceneEntityDefinition? definition, string src, out string hash) | |
| internal static bool TryResolveContentHash(SceneEntityDefinition? definition, string? src, out string hash) |
3 participants
Pull Request Description
What does this PR change?
Resolves the long-standing TODO in
LocalSceneDevelopmentController: the sdk-commands dev server'sUpdateModelwebsocket message already names the exact GLTF that changed (src+ path-derivedhash), but the client discarded it and force-drained every cache on each hot reload.This PR consumes that message. When a
.glb/.gltfis saved during local scene development (raw-GLTF mode):GltfLoadCache) and its pooled container instances (GltfContainerAssetsCache) — via a newICacheCleaner.EvictGltfModel(hash, src).Everything else keeps today's conservative behavior:
scene.json) arrive as a coarseupdateScenecarrying only the scene id — we can't know what went stale (e.g. a.gltf's external texture), so the full drain remains.--local-abmode: the model lives in the asset-bundle caches, so scoped eviction is skipped. Guarded byIsRawGltfModel, which mirrors the exact key composition the container cache uses (AssetBundleManifestVersionOrFailed.ComposeCacheKey(hash) == hash), so the guard can't drift from the real cache keying.New primitives:
IStreamableCache.Remove(key)(default no-op; real implementation inRefCountStreamableCacheBasemirrorsUnloadscoped to one key, honoring refcounts) andIGltfContainerAssetsCache.Remove(key).Performance (measured in editor, LSD, dev vs this branch)
Genesis Plaza
central-plaza(70 parcels, 390 GLBs, 1.13M triangles):Two timings are reported, because they diverge badly on a scene this size:
SceneLoadingConcluded). This only tracks GLTFs declared in the scene's first ticks, so it fires while most content is still streaming in.(Memory budget was disabled during measurement so budget-driven eviction/throttling wouldn't pollute the numbers.)
.glbsave.glbsave (1KB / 3.5MB / 4.2MB model)Numbers re-measured after the content-hash resolution fix, i.e. with the edited model genuinely evicted and re-downloaded — model size adds ~0.5s at most against the local dev server. Eviction correctness was verified with a real content swap (replacing one model's bytes with a different model): the new content shows after the reload.
On heavy scenes the win is qualitative: a model save no longer wipes the world for ~20s of visible popping — the scene is whole ~5s after hitting save, regardless of how big the scene or the edited model is.
Test Instructions
Steps (standard run):
Run against a local scene (
npm run startin any SDK7 scene, connect with the local-scene-development launch args, without--local-ab).Expected result:
toucha.glbin the scene's assets → the scene reloads noticeably faster than ondev, and the edited model shows the new content.IE:
BenchStreet.glbandBushPot.glbon a separate folder than the one you are running the build from (so you dont accidentlaly trigger reloading)scene.json→ reload behaves exactly as ondev(full drain), changes show up.Additional Testing Notes
--local-ab, a.glbsave must still do the full drain (scoped eviction is guarded off)..gltfthat references an external texture: editing the texture triggers full drain (correct); editing the.gltfitself takes the scoped path.localSceneDevelopment.Quality Checklist
🤖 Generated with Claude Code