fix(hal/vulkan): count textures created by create_texture - #10022
fix(hal/vulkan): count textures created by create_texture#10022dustyleary wants to merge 2 commits into
Conversation
HalCounters::textures was only incremented in add_raw_texture (the create_texture_from_hal path), while destroy_texture decremented it unconditionally. Every texture created and destroyed through the ordinary create_texture path therefore drove the counter negative, and Device::get_internal_counters() would report e.g. "0 textures" alongside gigabytes of correctly tracked texture_memory. The other backends (dx12, metal, gles) all increment the counter in create_texture. Do the same on Vulkan, mirroring create_buffer by doing the counter bookkeeping after the last fallible operation -- which also stops texture_memory from leaking an increment when bind_image_memory fails. Adds a backend-agnostic regression test that checks hal.textures and hal.texture_memory return to their baseline across a create/destroy cycle, and counts the texture while it is alive. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
25a49c1 to
e5c4bab
Compare
There was a problem hiding this comment.
This seems like a PR where the LLM did the brunt of the work for prose, so I felt little chagrin in changing things as I saw fit both in the PR's contents (see 538bcc4) and the OP. The OP especially was far more verbose than helpful. 😅
style: Using imperative tense in the OP and in commits really saves on the amount you have to type. You should use that, instead of explaining everything. I've fixed this to my satisfaction, so no action items needed (unless you disagree with something that got changed, OFC).
I used Conventional Comments in this review! I hope they help with clarity and tone. 🙂
| /// Regression test for the Vulkan backend never incrementing | ||
| /// `HalCounters::textures` in `create_texture` while still decrementing it in | ||
| /// `destroy_texture`, which made the reported texture count drift negative. |
There was a problem hiding this comment.
nit: This is a regression test, so let's move it over to regressions/ for the issue I just filed for this PR (#10038). I don't think we have appetite for a general suite of tests for counters right now, so let's not try to tackle that right this second.
| alive.textures.read(), | ||
| before.textures.read() + 1, | ||
| "hal.textures should count the live texture", | ||
| ); |
There was a problem hiding this comment.
question: Is there a reason it's not feasible to check that the memory has increased, too? We might want to avoid checking for a specific difference, but it seems easy to check that texture_memory is just bigger somehow.
| // The webgpu backend does not implement internal counters. | ||
| .skip(FailureCase::backend(wgpu::Backends::BROWSER_WEBGPU)), |
There was a problem hiding this comment.
question: Isn't this also not implemented on the WebGL backend?
| self.counters.texture_memory.add(allocation.size() as isize); | ||
|
|
There was a problem hiding this comment.
praise: Ugh, good catch on needing to put this after the last fallible operation.
3 participants
Connections
Description
On the Vulkan backend,
HalCounters::texturesis only ever decremented for textures created through the ordinarycreate_texturepath, soDevice::get_internal_counters()reports a texture count that drifts negative whiletexture_memorystays correct (e.g. "-37 textures / 1.31 GB of texture memory").Apply two fixes:
counters.texturesin Vulkan'screate_texture.bind_image_memoryfails partway through.Testing
Add
wgpu_gpu::regression::issue_10038::texture_counters_balanced. Add thecountersfeature to the test crate'swgpudependency. Also confirmed as fixed in a downstream project, with this PR backported to 29.0.3.Squash or Rebase?
Squash.
Checklist
wgpumay be affected behaviorally.CHANGELOG.mdentries for the user-facing effects of this change are present.