Skip to content

Commit

Permalink
Merge gfx-rs#1159
Browse files Browse the repository at this point in the history 
1159: Zero initialize buffers r=kvark a=Wumpf

**Connections**
First half of  gfx-rs#563, focusing solely on buffers and ignoring same issue for textures

**Description**
Tracks for each buffer which parts are initialized (i.e. set to zero). Identified three interaction points for this:
* buffer mapping: Zeroing out ad-hoc and marking as initialized
* queue write to buffer: Marks target buffer regions as initialized (i.e. optimizes away buffer init down the line)
* use in binding or copy operation in a command buffer:
   * fine grained tracking of regions that may require init (or will be initialized implicitly) on each command buffer
   * set in motion on queue submit, init is exclusively with `fill_buffer`

Todo list for Ready-to-Review

- [x] memory barriers for `fill_buffer` calls
- [x] better data structure for `memory_init_tracker`
- [x] coarse filtering on command-buffer buffer init requirements (the list should almost always be empty whereas now it pushes any buffer use)
- [x] improve naming of things
- [x] at least pretend this is adequately tested

Todo list beyond this PR

* make data structures usable for textures
  * and.. well.. implement all this for textures!
* explore reusing barrier tracker for memory init tracking?


**Testing**

* Some basic testing by doing some changes to wgpu-rs samples and watching them in in the debugger.
* Added a ron test file for the player (love those!) to poke the feature a little bit
* MemoryInitTracker comes with simple unit tests

Overall this is a bit shallow but as so often in this area accurate testing is hard because the outcomes are quite indirect



Co-authored-by: Andreas Reich <[email protected]>
  • Loading branch information
@bors @Wumpf
bors[bot] and Wumpf authored Feb 1, 2021
2 parents 1668bb2 + 822424e commit da8d17d
Show file tree
Hide file tree
Showing 14 changed files with 765 additions and 28 deletions.
1 change: 1 addition & 0 deletions player/tests/data/all.ron
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
backends: (bits: 0xF),
tests: [
"buffer-copy.ron",
"buffer-zero-init.ron",
"bind-group.ron",
"quad.ron",
],
Expand Down
70 changes: 70 additions & 0 deletions player/tests/data/buffer-zero-init.ron
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
(
features: (bits: 0x0),
expectations: [
(
name: "mapped_at_creation: false, with MAP_WRITE",
buffer: (index: 0, epoch: 1),
offset: 0,
data: Raw([0x00, 0x00, 0x00, 0x00]),
),
(
name: "mapped_at_creation: false, without MAP_WRITE",
buffer: (index: 1, epoch: 1),
offset: 0,
data: Raw([0x00, 0x00, 0x00, 0x00]),
),
(
name: "partially written buffer",
buffer: (index: 2, epoch: 1),
offset: 0,
data: Raw([0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x80, 0xBF, 0x00, 0x00, 0x80, 0xBF, 0x00, 0x00, 0x80, 0x3F, 0x00, 0x00, 0x80, 0x3F, 0x00, 0x00, 0x00, 0x00]),
),
],
actions: [
CreateBuffer(
Id(0, 1, Empty),
(
label: Some("mapped_at_creation: false, with MAP_WRITE"),
size: 16,
usage: (
bits: 131, // STORAGE + MAP_READ + MAP_WRITE
),
mapped_at_creation: false,
),
),
CreateBuffer(
Id(1, 1, Empty),
(
label: Some("mapped_at_creation: false, without MAP_WRITE"),
size: 16,
usage: (
bits: 129, // STORAGE + MAP_READ
),
mapped_at_creation: false,
),
),
CreateBuffer(
Id(2, 1, Empty),
(
label: Some("partially written"),
size: 24,
usage: (
bits: 9, // MAP_READ + COPY_DST
),
mapped_at_creation: false,
),
),
WriteBuffer(
id: Id(2, 1, Empty),
data: "data1.bin",
range: (
start: 4,
end: 20,
),
queued: true,
),

// TODO: Add a buffer with `mapped_at_creation: true`. As of writing there's no unmap action we could insert here.
Submit(1, []),
],
)
2 changes: 2 additions & 0 deletions wgpu-core/src/binding_model.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ use crate::{
},
hub::Resource,
id::{BindGroupLayoutId, BufferId, DeviceId, SamplerId, TextureViewId, Valid},
memory_init_tracker::MemoryInitTrackerAction,
track::{TrackerSet, DUMMY_SELECTOR},
validation::{MissingBufferUsageError, MissingTextureUsageError},
FastHashMap, Label, LifeGuard, MultiRefCount, Stored, MAX_BIND_GROUPS,
Expand Down Expand Up @@ -588,6 +589,7 @@ pub struct BindGroup<B: hal::Backend> {
pub(crate) layout_id: Valid<BindGroupLayoutId>,
pub(crate) life_guard: LifeGuard,
pub(crate) used: TrackerSet,
pub(crate) used_buffer_ranges: Vec<MemoryInitTrackerAction<BufferId>>,
pub(crate) dynamic_binding_info: Vec<BindGroupDynamicBindingData>,
}

Expand Down
1 change: 1 addition & 0 deletions wgpu-core/src/command/allocator.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,7 @@ impl<B: GfxBackend> CommandAllocator<B> {
device_id,
trackers: TrackerSet::new(B::VARIANT),
used_swap_chains: Default::default(),
buffer_memory_init_actions: Default::default(),
limits,
private_features,
has_labels: label.is_some(),
Expand Down
48 changes: 45 additions & 3 deletions wgpu-core/src/command/bundle.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -49,14 +49,15 @@ use crate::{
},
hub::{GfxBackend, GlobalIdentityHandlerFactory, Hub, Resource, Storage, Token},
id,
memory_init_tracker::{MemoryInitKind, MemoryInitTrackerAction},
resource::BufferUse,
span,
track::{TrackerSet, UsageConflict},
validation::check_buffer_usage,
Label, LabelHelpers, LifeGuard, Stored, MAX_BIND_GROUPS,
};
use arrayvec::ArrayVec;
use std::{borrow::Cow, iter, ops::Range};
use std::{borrow::Cow, iter, mem, ops::Range};
use thiserror::Error;

/// Describes a [`RenderBundleEncoder`].
Expand Down Expand Up @@ -159,6 +160,7 @@ impl RenderBundleEncoder {
let mut commands = Vec::new();
let mut base = self.base.as_ref();
let mut pipeline_layout_id = None::<id::Valid<id::PipelineLayoutId>>;
let mut buffer_memory_init_actions = Vec::new();

for &command in base.commands {
match command {
Expand Down Expand Up @@ -204,6 +206,8 @@ impl RenderBundleEncoder {
.map_pass_err(scope);
}

buffer_memory_init_actions.extend_from_slice(&bind_group.used_buffer_ranges);

state.set_bind_group(index, bind_group_id, bind_group.layout_id, offsets);
state
.trackers
Expand Down Expand Up @@ -262,6 +266,11 @@ impl RenderBundleEncoder {
Some(s) => offset + s.get(),
None => buffer.size,
};
buffer_memory_init_actions.push(MemoryInitTrackerAction {
id: buffer_id,
range: offset..end,
kind: MemoryInitKind::NeedsInitializedMemory,
});
state.index.set_format(index_format);
state.index.set_buffer(buffer_id, offset..end);
}
Expand All @@ -284,6 +293,11 @@ impl RenderBundleEncoder {
Some(s) => offset + s.get(),
None => buffer.size,
};
buffer_memory_init_actions.push(MemoryInitTrackerAction {
id: buffer_id,
range: offset..end,
kind: MemoryInitKind::NeedsInitializedMemory,
});
state.vertex[slot as usize].set_buffer(buffer_id, offset..end);
}
RenderCommand::SetPushConstant {
Expand Down Expand Up @@ -379,7 +393,7 @@ impl RenderBundleEncoder {
}
RenderCommand::MultiDrawIndirect {
buffer_id,
offset: _,
offset,
count: None,
indexed: false,
} => {
Expand All @@ -396,13 +410,26 @@ impl RenderBundleEncoder {
check_buffer_usage(buffer.usage, wgt::BufferUsage::INDIRECT)
.map_pass_err(scope)?;

buffer_memory_init_actions.extend(
buffer
.initialization_status
.check(
offset..(offset + mem::size_of::<wgt::DrawIndirectArgs>() as u64),
)
.map(|range| MemoryInitTrackerAction {
id: buffer_id,
range,
kind: MemoryInitKind::NeedsInitializedMemory,
}),
);

commands.extend(state.flush_vertices());
commands.extend(state.flush_binds());
commands.push(command);
}
RenderCommand::MultiDrawIndirect {
buffer_id,
offset: _,
offset,
count: None,
indexed: true,
} => {
Expand All @@ -420,6 +447,19 @@ impl RenderBundleEncoder {
check_buffer_usage(buffer.usage, wgt::BufferUsage::INDIRECT)
.map_pass_err(scope)?;

buffer_memory_init_actions.extend(
buffer
.initialization_status
.check(
offset..(offset + mem::size_of::<wgt::DrawIndirectArgs>() as u64),
)
.map(|range| MemoryInitTrackerAction {
id: buffer_id,
range,
kind: MemoryInitKind::NeedsInitializedMemory,
}),
);

commands.extend(state.index.flush());
commands.extend(state.flush_vertices());
commands.extend(state.flush_binds());
Expand Down Expand Up @@ -454,6 +494,7 @@ impl RenderBundleEncoder {
ref_count: device.life_guard.add_ref(),
},
used: state.trackers,
buffer_memory_init_actions,
context: self.context,
life_guard: LifeGuard::new(desc.label.borrow_or_default()),
})
Expand Down Expand Up @@ -502,6 +543,7 @@ pub struct RenderBundle {
base: BasePass<RenderCommand>,
pub(crate) device_id: Stored<id::DeviceId>,
pub(crate) used: TrackerSet,
pub(crate) buffer_memory_init_actions: Vec<MemoryInitTrackerAction<id::BufferId>>,
pub(crate) context: RenderPassContext,
pub(crate) life_guard: LifeGuard,
}
Expand Down
32 changes: 32 additions & 0 deletions wgpu-core/src/command/compute.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ use crate::{
},
hub::{GfxBackend, Global, GlobalIdentityHandlerFactory, Storage, Token},
id,
memory_init_tracker::{MemoryInitKind, MemoryInitTrackerAction},
resource::{Buffer, BufferUse, Texture},
span,
track::{TrackerSet, UsageConflict},
Expand Down Expand Up @@ -146,6 +147,8 @@ pub enum ComputePassErrorInner {
end_offset: u64,
buffer_size: u64,
},
#[error("buffer {0:?} is invalid or destroyed")]
InvalidBuffer(id::BufferId),
#[error(transparent)]
ResourceUsageConflict(#[from] UsageConflict),
#[error(transparent)]
Expand Down Expand Up @@ -329,6 +332,22 @@ impl<G: GlobalIdentityHandlerFactory> Global<G> {
.validate_dynamic_bindings(&temp_offsets)
.map_pass_err(scope)?;

cmd_buf.buffer_memory_init_actions.extend(
bind_group.used_buffer_ranges.iter().filter_map(
|action| match buffer_guard.get(action.id) {
Ok(buffer) => buffer
.initialization_status
.check(action.range.clone())
.map(|range| MemoryInitTrackerAction {
id: action.id,
range,
kind: action.kind,
}),
Err(_) => None,
},
),
);

if let Some((pipeline_layout_id, follow_ups)) = state.binder.provide_entry(
index as usize,
id::Valid(bind_group_id),
Expand Down Expand Up @@ -513,6 +532,19 @@ impl<G: GlobalIdentityHandlerFactory> Global<G> {
.ok_or(ComputePassErrorInner::InvalidIndirectBuffer(buffer_id))
.map_pass_err(scope)?;

let stride = 3 * 4; // 3 integers, x/y/z group size

cmd_buf.buffer_memory_init_actions.extend(
indirect_buffer
.initialization_status
.check(offset..(offset + stride))
.map(|range| MemoryInitTrackerAction {
id: buffer_id,
range,
kind: MemoryInitKind::NeedsInitializedMemory,
}),
);

state
.flush_states(
raw,
Expand Down
13 changes: 2 additions & 11 deletions wgpu-core/src/command/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ use crate::{
device::{all_buffer_stages, all_image_stages},
hub::{GfxBackend, Global, GlobalIdentityHandlerFactory, Storage, Token},
id,
memory_init_tracker::MemoryInitTrackerAction,
resource::{Buffer, Texture},
span,
track::TrackerSet,
Expand All @@ -46,6 +47,7 @@ pub struct CommandBuffer<B: hal::Backend> {
pub(crate) device_id: Stored<id::DeviceId>,
pub(crate) trackers: TrackerSet,
pub(crate) used_swap_chains: SmallVec<[Stored<id::SwapChainId>; 1]>,
pub(crate) buffer_memory_init_actions: Vec<MemoryInitTrackerAction<id::BufferId>>,
limits: wgt::Limits,
private_features: PrivateFeatures,
has_labels: bool,
Expand All @@ -56,17 +58,6 @@ pub struct CommandBuffer<B: hal::Backend> {
}

impl<B: GfxBackend> CommandBuffer<B> {
fn get_encoder(
storage: &Storage<Self, id::CommandEncoderId>,
id: id::CommandEncoderId,
) -> Result<&Self, CommandEncoderError> {
match storage.get(id) {
Ok(cmd_buf) if cmd_buf.is_recording => Ok(cmd_buf),
Ok(_) => Err(CommandEncoderError::NotRecording),
Err(_) => Err(CommandEncoderError::Invalid),
}
}

fn get_encoder_mut(
storage: &mut Storage<Self, id::CommandEncoderId>,
id: id::CommandEncoderId,
Expand Down
Loading

0 comments on commit da8d17d

Please sign in to comment.