Add StagingBelt::finish_and_recall_on_submit#9506
Add StagingBelt::finish_and_recall_on_submit#9506ruihe774 wants to merge 1 commit intogfx-rs:trunkfrom
StagingBelt::finish_and_recall_on_submit#9506Conversation
| /// the next call that needs free staging-belt chunks. If the encoder is | ||
| /// never submitted, the belt's closed chunks will not be returned and the | ||
| /// belt will allocate new buffers indefinitely. |
There was a problem hiding this comment.
Seems sketchy to me no? Is calling this and then not submitting the encoder just a guaranteed memory leak?
There was a problem hiding this comment.
It just does not provide more guarantee than finish; the semantic is similar. If finish is called without recall, the memory is leaked as well. Actually, finish_and_recall_on_submit is relatively safer as it's rarer to forget to submit a CommandEncoder than forgetting to call recall after submission.
There was a problem hiding this comment.
If it is called without recall, surely you can still recall the memory later right? Is this any different? I may have misread your comment.
There was a problem hiding this comment.
Yes. And for finish_and_recall_on_submit, if it is called without Queue::submit, surely you can still recall the memory by submitting the CommandEncoder. The guarantee is just similar: for finish, reclaim is on recall; for this, reclaim is on submission. An edge case may be you encode commands but never submit them and deliberately drop them; but it's rare and in that case you can just use finish
There was a problem hiding this comment.
@ruihe774 So then if you call recall on a command encoder that doesn't get submitted, you can never reclaim that memory without destroying the StagingBelt? Thats probably fine I guess
There was a problem hiding this comment.
Should be:
if you call finish_and_recall_on_submit on a command encoder that doesn't get submitted, you can never reclaim that memory without destroying the StagingBelt
There was a problem hiding this comment.
I assume the same would happen if you drop the command encoder before submitting it?
There was a problem hiding this comment.
Yes. Dropped CommandEncoder never gets submitted. The name of this method explicitly states that: recall on submit
inner-daemons
left a comment
There was a problem hiding this comment.
I'm a little slow but this LGTM
kpreid
left a comment
There was a problem hiding this comment.
[GitHub is giving me internal errors when I try to add file comments, so you get this unassociated comment instead.]
I would suggest, instead of making a copy of the existing staging_belt_random_test, putting the logic in one function called by two test functions, like
fn staging_belt_random_test(use_recall_on_submit: bool) {
...
if use_recall_on_submit {
belt.finish_and_recall_on_submit(&encoder);
queue.submit([encoder.finish()]);
// No explicit recall() needed.
} else {
belt.finish();
queue.submit([encoder.finish()]);
belt.recall();
}
...
}
#[test]
fn staging_belt_manual_recall() {
staging_belt_random_test(false);
}
#[test]
fn staging_belt_finish_and_recall_on_submit() {
staging_belt_random_test(true);
}My experience is that a very common maintenance error is to modify a test (e.g. to strengthen or weaken an assertion) and not modify other similar tests, so I’d like to keep the rest of this code un-duplicated.
Convenience that combines `finish` and `recall` by deferring the buffer re-map via `CommandEncoder::map_buffer_on_submit`, so callers no longer need an explicit `recall()` after submission.
Connections
Builds on the original
StagingBeltintroduction in gfx-rs/wgpu-rs#408.Description
Adds
StagingBelt::finish_and_recall_on_submit, a convenience method that combinesfinish()andrecall()into a single call. It defers the closed-chunk re-map viaCommandEncoder::map_buffer_on_submit, so the chunks are returned automatically once the encoder is submitted — no explicitrecall()is needed afterward.This shortens the typical staging-belt usage from four steps to three:
write_buffer/allocateand write data.finish_and_recall_on_submit(&encoder).The semantics match
finish()followed byrecall(), the only difference being that the re-map is scheduled to run when the submission completes rather than triggered eagerly by the user.Testing
Added
staging_belt_finish_and_recall_on_submitintests/tests/wgpu-validation/util.rs, mirroring the existingstaging_belt_random_testbut using the new method and omitting the explicitrecall()call. The test exercises 100 submission batches and verifies the belt continues to recycle chunks correctly.Squash or Rebase?
Single commit — ready to rebase onto
trunkas-is.Checklist
wgpumay be affected behaviorally.CHANGELOG.mdentries for the user-facing effects of this change are present.