-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Small optimization to prepare_surface_texture_for_submit
#9473
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -126,7 +126,7 @@ impl Queue { | |
| texture: &Arc<Texture>, | ||
| ) -> Result<(), DeviceError> { | ||
| let snatch_guard = self.device.snatchable_lock.read(); | ||
| let mut submission = self | ||
| let submission = self | ||
| .allocate_submission(snatch_guard) | ||
| .map_err(|(_index, e)| e)?; | ||
| let device = &self.device; | ||
|
|
@@ -143,6 +143,8 @@ impl Queue { | |
| let mut pending_writes = self.pending_writes.lock(); | ||
|
|
||
| if needs_clear { | ||
| // After encoding the clear operation, we must not return without | ||
| // adding the texture to `pending_writes`. | ||
| let encoder = pending_writes.activate(); | ||
| let mut trackers = device.trackers.lock(); | ||
| crate::command::clear_texture( | ||
|
|
@@ -184,6 +186,12 @@ impl Queue { | |
| }; | ||
|
|
||
| if pending.is_empty() { | ||
| // This assert checks that we don't return here if we encoded a | ||
| // clear operation for the texture, which would be a problem since | ||
| // 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); | ||
| return Ok(()); | ||
| } | ||
|
|
||
|
|
@@ -200,21 +208,20 @@ impl Queue { | |
| let encoder = pending_writes.activate(); | ||
| // SAFETY: | ||
| // - The encoder is in the recording state after `activate()` | ||
| // - The texture is kept alive by the Arc from `acquired_texture` | ||
| // - The texture is kept alive by adding it to `PendingWrites` below | ||
| unsafe { | ||
| encoder.transition_textures(&barriers); | ||
| } | ||
| } | ||
|
|
||
| // Keep the texture alive in the submission so its clear_view isn't | ||
| // destroyed before the GPU finishes the submitted commands. | ||
| // Add the texture to `PendingWrites`. This will cause `submit()` to: | ||
| // - Flush any pending writes to the texture. | ||
| // - Include the texture in `surface_textures` for the submission. | ||
| // - Keep the texture alive so the texture and its clear_view aren't | ||
| // destroyed before the GPU finishes the `clear_texture` operation | ||
| // encoded above. | ||
| pending_writes.insert_texture(texture); | ||
|
|
||
| // Flush pending writes through the standard submission path. | ||
| submission | ||
| .surface_textures | ||
| .insert(Arc::as_ptr(texture), texture.clone()); | ||
|
Comment on lines
-213
to
-216
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I assume this was just unnecessary?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's discussed in the comment linked in the description -- it's redundant; putting the texture in |
||
|
|
||
| submission.submit(pending_writes)?; | ||
|
|
||
| Ok(()) | ||
|
|
@@ -575,7 +582,7 @@ pub(crate) struct PendingSubmission<'a> { | |
| // Surface textures referenced by command buffers in this submission. These need to be | ||
| // passed to the HAL `submit` call. Deduplicated using a hashmap to avoid vulkan | ||
| // deadlocking from the same surface texture being submitted multiple times. | ||
| pub surface_textures: FastHashMap<*const Texture, Arc<Texture>>, | ||
| surface_textures: FastHashMap<*const Texture, Arc<Texture>>, | ||
| pub index: SubmissionIndex, | ||
| } | ||
|
|
||
|
|
@@ -1545,8 +1552,8 @@ impl Queue { | |
| /// The caller passes in the already-acquired [`SnatchGuard`]. This function acquires | ||
| /// the fence lock and the command index lock. | ||
| /// | ||
| /// The caller should update the [`PendingSubmission`] members `executions` and | ||
| /// `surface_textures` with details of the submission. | ||
| /// The caller should update [`PendingSubmission::executions`] with details of the | ||
| /// submission. | ||
| /// | ||
| /// To finalize and submit the submission, call [`PendingSubmission::submit`] (which is | ||
| /// a convenience wrapper around [`Queue::submit_pending_submission`]). | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the reasoning for this being a
debug_assert?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@andyleiserson Ok, I trust your judgement here