queue: stop pinning every resource a command buffer referenced

[samyfodil]

Problem

CommandBuffer carries three encode-time validation sets — usedBuffers, usedTextures, usedBindGroups — populated during encoding so validateCommandBufferForSubmit can reject a submit that references a released buffer, a mapped buffer, a destroyed texture or a released bind group.

They are hard Go references to every resource the frame touched. An application using gogpu that builds bind groups per frame accumulates them without bound, and the live BindGroup count grows for as long as the command buffers stay reachable — while descriptor-set and destroy-queue counts both stay flat, which is what makes this hard to see from the outside.

Changes

Drop the per-draw fan-out. SetBindGroup copied every bound buffer and texture of a bind group into the encoder's maps, once per draw. That was duplication: CreateBindGroup already records boundBuffers/boundTextures on the group, and the group itself is tracked in usedBindGroups, so those slices were already reachable. validateCommandBufferForSubmit now walks them from the bind group and the passes track only the group. The buffer and texture checks move into validateSubmitBuffer/validateSubmitTexture, shared by the direct and transitive paths.

One behavioural nuance: a resource reachable only through a bind group that is itself released now reports the released bind group rather than the released or mapped resource. Both fail the submit; only the message differs.

Release the sets on every spent path. postSubmit returned early when the device had no DestroyQueue, skipping both the clear and cb.submitted, so the loop moves above that lookup — the HAL submit has already succeeded by then and the buffers are spent regardless. CommandBuffer.Release() never cleared them at all, so a Finish() followed by Release() — the documented path after hal.Submit fails — pinned the whole frame. Both now go through cb.dropUsedSets().

Clearing inside validateCommandBufferForSubmit instead was tempting, since it is the only consumer, but a batch where an early buffer validates and a later one fails returns without marking anything submitted; the caller is expected to retry, and the retry would revalidate the early buffer against nil sets and skip its checks silently.

What was considered and rejected

Replacing the maps with slices. The maps exist for O(1) dedup and SetVertexBuffer/SetIndexBuffer are per-draw; renderers interleave buffers (vb-A, vb-B, vb-A, …), so a "same as last entry" guard never fires and a slice would grow per draw call rather than per distinct buffer. The maps are load-bearing.

Verification

Measured in an application using gogpu that builds bind groups per frame: live BindGroup count goes from unbounded growth to flat.

• TestSubmitWithReleasedBufferInBindGroup — a buffer reachable only through a bind group is still caught at submit, covering the deleted fan-out.
• TestCommandBufferReleaseDropsUsedSets — Release() drops the sets.

Full suite passes, including the lifecycle_test.go additions from #288.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[kolkov]

Nice catch — the per-draw fan-out into usedBuffers/usedTextures was load-bearing for memory. We didn't see it because descriptor-set and destroy-queue counts stayed flat while BindGroup live count grew silently.

The fix aligns with how both Rust wgpu and Dawn handle this:

• Rust wgpu: StatelessTracker<BindGroup> tracks the group itself; queue_submit validation walks bind_group.used (the BindGroupStates) to reach buffers/textures transitively (queue.rs:1810-1818). No per-draw fan-out.
• Dawn: CommandBufferResourceUsage stores raw pointers for validation only (not ownership). Bind group entries are checked at submit, not copied per draw.

Your transitive walk in validateCommandBufferForSubmit matches this pattern exactly.

A few questions for discussion:

1. Discovery context — was this visible as memory growth in Taubyte, or did you find it via profiling? Curious how it manifested in practice — we have users building bind groups per frame in gg and hadn't caught the accumulation.

2. dropUsedSets timing — you chose to clear in postSubmit (after HAL submit succeeds) rather than in validateCommandBufferForSubmit (after validation passes). The PR body explains why — a batch where an early buffer validates but a later one fails would lose the validation sets on retry. Good reasoning. Did you consider clearing only on successful Submit return instead? Or is the postSubmit placement more robust because it also covers the Release() path?

3. Behavioral nuance — a resource reachable only through a released bind group now reports the bind group error rather than the specific buffer/texture error. For debugging, the buffer-level error is more actionable ("buffer X is released" vs "bind group is released"). Would it be worth checking bg.released last (after walking its resources) so the more specific error wins? Or is the current ordering intentional — fail fast on the bind group before walking its contents?

4. Interaction with ADR-056 — we just landed unified resource lifecycle (#288-#290) where BindGroup.Release() goes through ref.Drop() → onZero → dq.Defer. Your PR is orthogonal (validation maps vs lifecycle), but worth noting: after your change, the validation maps no longer pin *BindGroup Go objects, which means onZero can fire earlier (GC collects the BindGroup sooner). This is correct behavior — just confirming you're aware of the interaction.

Before merge — one question that needs a clear answer:

Point 3 (error message specificity): do you consider the current ordering a deliberate trade-off that's fine to ship, or should we fix it before merge? If a user gets "released bind group" when the actual problem is a released buffer inside it, they'll have a harder time debugging. If you think the current behavior is acceptable — say so explicitly and we'll merge as-is. If not — a small reorder (walk resources before checking bg.released) would fix it without changing the rest of the PR.

Approve — enterprise quality, well-tested, matches reference implementations.

[samyfodil]

Thanks for the review, and for digging up the Rust/Dawn precedent — good to know
the transitive walk lands where both of them already are.

3. Error specificity — fixed, and it was more than a preference.

You're right, and the reason is stronger than debuggability: the current ordering
was a regression this PR introduced, not a deliberate trade-off.

Before the fan-out was removed, SetBindGroup copied every bound buffer into the
flat usedBuffers set and every bound texture into usedTextures, checked in
steps 2 and 3 — ahead of the bind group loop in step 4. So a released buffer
or texture inside a released bind group already reported the resource.
Checking bg.released first inverted that for both. I flagged it in the commit
as an acceptable message change, but it wasn't: it silently changed behaviour
that existed before the branch.

Fixed, but not with the single reorder you suggested — that turned out to be
half a fix.

Walking the resources before bg.released within each group's iteration
handles the one-bind-group case. It leaves the outcome dependent on map
iteration order as soon as two groups are at fault: one released with sound
resources, one live but binding a released buffer. Whichever range cb.usedBindGroups reaches first decides the error, so the same program reports
different errors on different runs. The flat sets never had that property —
every resource was checked before any bind group, full stop.

So it's two passes now: one over every group's boundBuffers/boundTextures,
then a second over bg.released. Every resource error beats every bind group
error regardless of iteration order. Matches the Rust ordering you cited
(buffers and textures at queue.rs:1780-1808, bind groups at 1815-1817).

Safe to walk a released group's slices — Release() never touches
boundBuffers/boundTextures, so they stay valid for the group's lifetime. A
bind group with no bound resources still reports ErrSubmitBindGroupDestroyed
exactly as before; TestSubmitWithDestroyedBindGroup covers that and still
passes.

Two tests, since nothing exercised both conditions at once — which is why the
inversion got through in the first place.
TestSubmitReleasedBufferBeatsReleasedBindGroup covers the single group.
TestSubmitResourceErrorWinsAcrossBindGroups covers the multi-group case, and
its shape is deliberate: eight released groups against one live group binding a
released buffer, rebound through the same slot so they all land in
usedBindGroups without hitting the maxBindGroups limit. With only two groups
the one-pass version still passes most attempts — measured around 88% — so the
test would have been a coin flip dressed up as a guard. At eight it fails within
the first couple of iterations on every run.

1. Discovery context.

Not Taubyte — an application built on gogpu. It was leaking, and profiling is
what surfaced it. From the outside it looked like nothing was wrong: the
descriptor-set count and the destroy queue both stayed flat, so the usual places
you'd look were clean. The live BindGroup count was the only thing moving.

Given you have users building bind groups per frame in gg, they'd hit exactly
this — the accumulation is invisible unless you're looking at Go object counts
specifically.

2. dropUsedSets timing — one correction to the framing.

postSubmit runs only after hal.Submit returns successfully, so it already
is "clear on successful Submit" — there's no separate placement for that.

But it doesn't cover Release(). Those are two independent call sites:

• postSubmit — the submitted path.
• CommandBuffer.Release() — the released-without-submitting path, which is the
  documented contract after hal.Submit fails.

Release() previously dropped trackedRefs and the HAL encoder but left the
three validation sets populated, so a Finish() → failed Submit → Release()
sequence pinned the whole frame. Both sites now call dropUsedSets().

One more thing in that area: postSubmit used to return early when the device
had no DestroyQueue, which skipped both the clear and cb.submitted. The
bookkeeping loop moved above that lookup — by then the HAL submit has already
succeeded, so the command buffers are spent regardless.
TestPostSubmitDropsSetsWithoutDestroyQueue covers it.

4. ADR-056 interaction — aware, but your framing of it is more generous to this
PR than the code supports.

During encoding nothing changes: usedBindGroups still pins the group, and the
group transitively pins its boundBuffers/boundTextures, so removing the
fan-out doesn't alter reachability while the encoder is live. It only removes a
second, redundant path to objects that were already reachable.

Where I'd push back is "onZero can fire earlier". For the documented path it
can't. The three sets hold bare *Buffer/*Texture/*BindGroup pointers and
never Clone()/Drop() anything — the refcount pipeline is driven entirely by
trackRef() into cb.trackedRefs, which postSubmit hands to
dq.TrackSubmission/dq.Triage. That's a separate mechanism. When an
application calls bg.Release(), ref.Drop() fires at that call site
regardless of what usedBindGroups still holds, so dropUsedSets can't move
it earlier — it already happened. What the maps were actually delaying is
ordinary Go GC of the wrapper structs.

The one case where your description is exactly right is the finalizer fallback:
an application that never calls Release() and relies on
registerBindGroupCleanup needs the group unpinned before the cleanup can run,
and that cleanup is what calls ref.Drop(). That wasn't the situation here
though — the destroy queue and descriptor-set counts stayed flat throughout,
which is what told us the HAL side was being released correctly and only the Go
wrappers were accumulating.

So: correct that the interaction exists, but it's GC-level rather than
lifecycle-level for anything calling Release() properly.

Rebased onto current main (through #290) before pushing, so the lifecycle changes
are in the test run.

[kolkov]

All questions addressed. Error precedence fix is better than what I suggested — two-pass approach eliminates map iteration order dependency entirely. Tests are thorough (especially the 8-group multi-iteration case). ADR-056 interaction clarification accepted. LGTM.