Small optimization to prepare_surface_texture_for_submit#9473
Small optimization to prepare_surface_texture_for_submit#9473andyleiserson wants to merge 1 commit intogfx-rs:trunkfrom
prepare_surface_texture_for_submit#9473Conversation
andyleiserson
force-pushed
the
jj-push-xrty
branch
2 times, most recently
from
6e50167 to
be0be2a
Compare
andyleiserson
force-pushed
the
jj-push-xrty
branch
from
be0be2a to
03998cb
Compare
inner-daemons
left a comment
inner-daemons
left a comment
There was a problem hiding this comment.
Broadly LGTM. One maybe-suggestion
| ) -> Result<(), DeviceError> { | ||
| let snatch_guard = self.device.snatchable_lock.read(); | ||
| let (mut submission, _index) = self.allocate_submission(snatch_guard); | ||
| let (submission, _index) = self.allocate_submission(snatch_guard); |
There was a problem hiding this comment.
Could've sworn this exact line is modified by another recent PR.
There was a problem hiding this comment.
I'd believe it has gone back and forth as the calls made on the submission have changed. This is a case where unused_mut is perhaps not that valuable, but easier just to remove the mut than to waive the lint.
| // we haven't done anything yet to ensure it stays alive. If we | ||
| // cleared the texture, then we must have produced a barrier to put | ||
| // it in PRESENT state, so `pending` will not be empty. | ||
| debug_assert!(!needs_clear); |
There was a problem hiding this comment.
What's the reasoning for this being a debug_assert?
There was a problem hiding this comment.
For me it's a borderline case. It does seem unlikely that the overhead of a release assertion here would be a problem. In favor of debug assert: it wasn't included with the code originally; it's more a defense against changes to the code in the future than against runtime conditions we don't expect; for non-web-content applications the annoyance of a false positive is probably worse than whatever consequences of this being wrong.
I am generally on the side of using release asserts unless there's concrete evidence they're too costly. If I wrote the entire function I'd probably make it a release assert.
There was a problem hiding this comment.
@andyleiserson Ok, I trust your judgement here
| // Flush pending writes through the standard submission path. | ||
| submission | ||
| .surface_textures | ||
| .insert(Arc::as_ptr(texture), texture.clone()); |
There was a problem hiding this comment.
I assume this was just unnecessary?
There was a problem hiding this comment.
It's discussed in the comment linked in the description -- it's redundant; putting the texture in pending_writes will result in it being added to surface_textures later anyways.
inner-daemons
left a comment
inner-daemons
left a comment
There was a problem hiding this comment.
Just ftr, lgtm, abcdef
|
Why was this closed? |
|
Oops, that was an accident. (GitHub auto-closes PRs if you delete the branch.) Thank you for noticing! |
andyleiserson
force-pushed
the
jj-push-xrty
branch
from
03998cb to
fb0e260
Compare
3 participants
Small cleanup as discussed in #9307 (comment).
Also updates some comments and adds a debug assert.
Testing
Relying on existing tests
Squash or Rebase? Squash
Checklist
wgpumay be affected behaviorally.CHANGELOG.mdentries for the user-facing effects of this change are present.