Skip to content

fix(rrm): UploadBuffer doesn't track its VB region in GeometryPool — pool leak when first caller lands #770

Description

@JeanPhilippeKernel

Summary

`RenderResourceManager::UploadBuffer` calls `EnsureBatchOpen` + `AppendToGlobalBuffer` to copy data into a newly-allocated generic `BufferView`, but that buffer is a separate `VmaAllocation` — it is NOT the streaming pool's `m_pool.VertexBuffer` / `m_pool.IndexBuffer`. The data copy goes to the correct destination. However, `UploadBuffer` never calls `m_pool.Allocate()`, so the pool doesn't know the region is in use, and `Release(handle)` never calls `m_pool.Free()` because the handle carries `GBUF_GEN_TAG` (generic buffer) — it goes through `DeferFree` for the `VmaAllocation` only.

This means the bytes written into the pool VB/IB by any `UploadBuffer` call are permanent holes: allocated from the append cursor, never tracked, never reclaimed. `FragmentationRatio()` will not count them. Over time this silently exhausts the pool.

Current state

`UploadBuffer` has zero callers in the current codebase. It becomes dangerous the moment the first real caller lands — the most likely candidate is `SkinningUploadSystem` in Sprint 7 (animation, bone matrix upload).

Root cause

`UploadBuffer` was written before the pool existed (when the global buffer was append-only). After the streaming pool landed in PR #769, the function was not updated.

Fix options

Option A (recommended): Make `UploadBuffer` use the staging path (like `UpdateBuffer`'s staging sub-case) rather than the global pool at all. Bone matrices and particle VBs don't belong in the mesh streaming pool — they should be allocated as standalone device-local buffers via `GpuMem.AllocateBuffer` with their own `VmaAllocation`, uploaded via the batch. This is already what `UploadBuffer` does for the destination buffer; the fix is to also remove the `AppendToGlobalBuffer(m_pool.VertexBuffer, ...)\ call and replace it with a direct staging copy into \buf` using `UpdateBuffer`'s staging sub-path.

Option B: Track the generic allocation in `m_pool` (store a `GeometryRegion` alongside the `Slot`). This is more complex and conflates two different concerns (streaming mesh pool vs. standalone generic buffers).

Option A is cleaner: generic device-local buffers should never share the mesh streaming pool.

References

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Labels

P2High priority — next sprintarea-renderingbugSomething isn't working

Projects

No projects

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions