decentraland / decentraland/unity-explorer
[TECH DEBT] StreamableLoading | Asymmetric ref-counting: AddReference routes through the cache, Dereference through the asset
- Dominant language
- C#
- Stars
- 23
- Forks
- 17
- Avg merge
- 2d 16h
- Merged PRs (30d)
- 101
Description
### π **Priority Level:**
Low
### π **Area/Component:**
`ECS.StreamableLoading` β `IStreamableCache.AddReference`, `LoadSystemBase.ApplyLoadedResult`, `IStreamableRefCountData`, cache implementations (`RefCountStreamableCacheBase`, `NoCache`)
### π **Description:**
The two halves of asset ref-counting take different routes. The increment is cache-mediated: `LoadSystemBase.ApplyLoadedResult` calls `cache.AddReference(key, asset)`, letting cache policy decide whether the reference is counted. The decrement is asset-direct: consumers (`ReleaseMaterial`, `UIBackgroundReleaseSystem`, NFT shape cleanup, `SpriteData`β¦) hold only the asset and call `Dereference()` on it β `IStreamableCache` has no `Dereference` member at all.
Because the decrement is unconditional while the increment is policy-controlled, any cache whose policy is "don't count" desynchronizes the counter. This bit concretely: `NoCache` inherited the default no-op `AddReference`, so every `NoCache`-served texture went negative on release and spammed the negative-count guard. The current fix makes `NoCache` forward `AddReference` to the asset β correct, but it deepens the asymmetry rather than removing it.
### π **Current State:**
- `IStreamableCache.AddReference(in key, asset)` β default no-op interface member.
- `RefCountStreamableCacheBase.AddReference` β ignores `key`, forwards to `asset.AddReference()`.
- `NoCache.AddReference` β type-checks for `IStreamableRefCountData`, forwards to `asset.AddReference()`.
- No implementation uses the `key` or touches cache state; the call is a vestigial indirection to the asset's own counter.
### π‘ **Proposed Solution:**
Make both halves symmetric on the asset:
1. `LoadSystemBase.ApplyLoadedResult` calls `asset.AddReference()` directly (via `IStreamableRefCountData`, type-checked β mirroring how consumers already call `Dereference()`).
2. Delete `AddReference` from `IStreamableCache` and every implementation, including the `NoCache` forwarding.
Result: the cache is pure storage; the counter has one owner (the asset) and no policy knob that can desynchronize it.
Verify first:
- No cache implementation beyond `RefCountStreamableCacheBase`/`NoCache` does real work in `AddReference` (wearable/emote storages also implement `IStreamableCache`).
- All `TAsset` types flowing through `ApplyLoadedResult` are either `IStreamableRefCountData` or safely skipped by the type check (e.g. `ISceneFacade`, scene definitions).
### βοΈ **Impact Assessment:**
#### Performance Impact:
None; same calls, one fewer virtual hop.
#### Maintainability Impact:
Removes a per-cache policy surface that has already produced a real desync; makes the negative-count guard trustworthy.
#### Risk of Refactoring:
Touches the shared load pipeline for every streamable asset type; a missed implementation that relied on cache-mediated referencing would silently stop counting. Mitigated by the verification pass above.
### π **Effort Estimate:**
S
### π **Dependencies:**
- #9900 (refcount-0 window between cache Add and consumer AddReference) β same subsystem; ideally fixed together, since the pre-acquired-reference solution proposed there also reshapes who calls `AddReference`.
- Branch `feat/lsd-content-versioned-reload`, commit `d1002aa54` (the `NoCache` forwarding this issue would supersede).
### π₯οΈ **Additional Notes:**
Surfaced while running local scene development against an old-SDK (path-only hash) server, where scene worlds use `NoCache` for textures: every texture release tripped the "Reference count should never be negative!" guard in `StreamableRefCountData.Dereference`.
Contributor guide
Assessment
This issue has not been assessed yet.