decentraland / decentraland/unity-explorer

[TECH DEBT] StreamableLoading | Asymmetric ref-counting: AddReference routes through the cache, Dereference through the asset

Open
#9,901 1 comment 0 reactions 1 assignee Claimed by @dalkia View on GitHub
3-low refactor tech debt
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

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.