From f84b2a1ea4a9dcbb548807e45e1c4dfe180f14fe Mon Sep 17 00:00:00 2001 From: janis Date: Sun, 25 Oct 2026 02:27:28 +0100 Subject: [PATCH] response to llm review --- crates/renderer_rendergraph/src/graph.rs | 364 ++++++++++++++++++++--- 1 file changed, 326 insertions(+), 38 deletions(-) diff --git a/crates/renderer_rendergraph/src/graph.rs b/crates/renderer_rendergraph/src/graph.rs index c16e0f2..7be8e8b 100644 --- a/crates/renderer_rendergraph/src/graph.rs +++ b/crates/renderer_rendergraph/src/graph.rs @@ -156,14 +156,15 @@ impl Kahn { } } - fn pop_ready(&mut self) -> Option { + fn pop_batch(&mut self) -> Vec { + let mut batch = Vec::new(); while let Some(Reverse(node)) = self.ready.pop() { if self.is_ready[node] { self.is_ready[node] = false; - return Some(node); + batch.push(node); } } - None + batch } fn complete(&mut self, node: usize) { @@ -242,16 +243,18 @@ impl RenderGraph { let command_count = self.commands.len(); let accesses = self.commands.side_effects.command_accesses(); let compiler = GraphCompiler::new(&self.resources, accesses, command_count); - let order = compiler.topo_sort()?; + let batches = compiler.topo_batches()?; compiler.validate_resources()?; - let barriers = compiler.build_barriers(&order); + let flat_order = batches.iter().flatten().copied().collect::>(); + let barriers = compiler.build_barriers(&flat_order, &batches); let (lifetimes, interference) = compiler.bind_lifetimes(); Ok(CompiledGraph { resources: self.resources, resolved: HashMap::new(), commands: self.commands, - order, + batches, + flat_order, barriers, lifetimes, interference, @@ -296,17 +299,22 @@ impl<'a> GraphCompiler<'a> { kahn } - fn topo_sort(&self) -> Result, GraphError> { + fn topo_batches(&self) -> Result>, GraphError> { let mut kahn = self.dependencies(); - let mut order = Vec::with_capacity(self.command_count); - while let Some(index) = kahn.pop_ready() { - order.push(index.into()); - kahn.complete(index); + let mut batches = Vec::new(); + while !kahn.is_complete() { + let batch = kahn.pop_batch(); + if batch.is_empty() { + return Err(GraphError::DependencyCycle); + } + let batch = batch + .into_iter() + .inspect(|&index| kahn.complete(index)) + .map(CommandIndex::from) + .collect(); + batches.push(batch); } - if !kahn.is_complete() { - return Err(GraphError::DependencyCycle); - } - Ok(order) + Ok(batches) } fn validate_resources(&self) -> Result<(), GraphError> { @@ -320,7 +328,17 @@ impl<'a> GraphCompiler<'a> { Ok(()) } - fn build_barriers(&self, order: &[CommandIndex]) -> Vec { + fn build_barriers( + &self, + order: &[CommandIndex], + batches: &[Vec], + ) -> Vec { + let mut batch_indices = vec![usize::MAX; self.command_count]; + for (batch_index, batch) in batches.iter().enumerate() { + for command in batch { + batch_indices[command.index()] = batch_index; + } + } let mut result = Vec::new(); for (ordered_index, &command_index) in order.iter().enumerate() { let mut barrier_resources = Vec::new(); @@ -328,13 +346,25 @@ impl<'a> GraphCompiler<'a> { let previous = order[..ordered_index] .iter() .rev() - .flat_map(|&prior_index| self.accesses[prior_index.index()].iter().rev()) - .find(|previous| { - previous.resource == current.resource - && previous.scope.conflicts(¤t.scope) + .find_map(|&prior_index| { + self.accesses[prior_index.index()] + .iter() + .rev() + .find(|previous| { + previous.resource == current.resource + && previous.scope.conflicts(¤t.scope) + }) + .map(|access| (prior_index, access)) }); - if let Some(previous) = previous { + + if let Some((previous_command, previous)) = previous { + debug_assert!( + batch_indices[previous_command.index()] + < batch_indices[command_index.index()] + ); + let (old_layout, new_layout) = Self::layouts(previous.scope, current.scope); + barrier_resources.push(BarrierResource { id: current.resource, range: scope_range(current.scope), @@ -345,19 +375,24 @@ impl<'a> GraphCompiler<'a> { old_layout, new_layout, }); + continue; } let desc = &self.resources[current.resource.0 as usize]; let initial = desc.initial_state(); + let (old_layout, new_layout) = Self::layouts_from_state(initial, current.scope); + let layout_transition = old_layout != new_layout && new_layout.is_some(); + let initial_hazard = matches!( desc, ResourceDesc::ImportedBuffer { .. } | ResourceDesc::ImportedTexture { .. } ) && !initial.access.is_empty() && (access_flag_is_write(initial.access) || access_flag_is_write(current.scope.access)); + if layout_transition || initial_hazard { barrier_resources.push(BarrierResource { id: current.resource, @@ -371,13 +406,15 @@ impl<'a> GraphCompiler<'a> { }); } } + if !barrier_resources.is_empty() { result.push(PipelineBarrier { - before_command: ordered_index.into(), + before_command: command_index, resources: barrier_resources, }); } } + result } @@ -389,6 +426,7 @@ impl<'a> GraphCompiler<'a> { SubresourceScopeInner::Texture { layout, .. } => Some(layout), SubresourceScopeInner::Buffer(_) => None, }; + (layout(previous), layout(current)) } @@ -400,6 +438,7 @@ impl<'a> GraphCompiler<'a> { SubresourceScopeInner::Texture { layout, .. } => Some(layout), SubresourceScopeInner::Buffer(_) => None, }; + (initial.layout, current_layout) } @@ -475,7 +514,8 @@ pub struct CompiledGraph { resources: Vec, resolved: HashMap, commands: CommandList, - order: Vec, + batches: Vec>, + flat_order: Vec, barriers: Vec, lifetimes: HashMap, interference: ResourceInterference, @@ -487,8 +527,12 @@ enum ResolvedResource { } impl CompiledGraph { + pub fn batches(&self) -> &[Vec] { + &self.batches + } + pub fn command_order(&self) -> &[CommandIndex] { - &self.order + &self.flat_order } pub fn barriers(&self) -> &[PipelineBarrier] { @@ -600,20 +644,39 @@ impl CompiledGraph { } } - let mut barriers = self.barriers.iter().peekable(); - for (ordered_index, &command_index) in self.order.iter().enumerate() { - while barriers - .peek() - .is_some_and(|barrier| barrier.before_command == ordered_index.into()) - { - recorder.pipeline_barrier(barriers.next().expect("peeked barrier")); + // TODO: `batches` are currently recorded sequentially into one command buffer; + // record per-batch command buffers and submit sequentially/in parallel, using split barriers/events or timeline semaphores for overlap. + for step in recording_steps(&self.batches, &self.barriers) { + match step { + // TODO: Merge `PipelineBarrier.resources` across commands or a batch to reduce vkCmdPipelineBarrier2 calls. + RecordingStep::Barrier(barrier) => recorder.pipeline_barrier(barrier), + RecordingStep::Command(command_index) => { + self.commands.commands[command_index.index()].apply(recorder)?; + } } - self.commands.commands[command_index.index()].apply(recorder)?; } Ok(()) } } +enum RecordingStep<'a> { + Barrier(&'a PipelineBarrier), + Command(CommandIndex), +} + +fn recording_steps<'a>( + batches: &'a [Vec], + barriers: &'a [PipelineBarrier], +) -> impl Iterator> + 'a { + batches.iter().flatten().flat_map(move |&command_index| { + barriers + .iter() + .filter(move |barrier| barrier.before_command == command_index) + .map(RecordingStep::Barrier) + .chain(std::iter::once(RecordingStep::Command(command_index))) + }) +} + /// The skeleton switches to a dedicated allocation at 64 MiB. This is a /// placeholder heuristic until resource lifetime/aliasing allocation is real. const DEDICATED_ALLOCATION_THRESHOLD: u64 = 64 * 1024 * 1024; @@ -850,13 +913,17 @@ mod tests { kahn.add_edge(1, 2); kahn.add_edge(2, 3); - let mut order = Vec::new(); - while let Some(node) = kahn.pop_ready() { - order.push(node); - kahn.complete(node); + let mut batches = Vec::new(); + while !kahn.is_complete() { + let batch = kahn.pop_batch(); + assert!(!batch.is_empty()); + for &node in &batch { + kahn.complete(node); + } + batches.push(batch); } - assert_eq!(order, [0, 1, 2, 3]); + assert_eq!(batches, [vec![0, 1], vec![2], vec![3]]); assert!(kahn.is_complete()); } @@ -867,7 +934,7 @@ mod tests { kahn.add_edge(1, 2); kahn.add_edge(2, 1); - while let Some(node) = kahn.pop_ready() { + for node in kahn.pop_batch() { kahn.complete(node); } @@ -1032,6 +1099,227 @@ mod tests { assert_eq!(compiled.barriers()[0].resources[0].id, b.id()); } + #[test] + fn independent_commands_share_a_batch_and_dependencies_advance_to_later_batches() { + let mut graph = RenderGraph::new(); + let a = graph.create_buffer(GraphBufferDesc { + size: 8, + ..Default::default() + }); + let shared = graph.create_buffer(GraphBufferDesc { + size: 8, + ..Default::default() + }); + let b = graph.create_buffer(GraphBufferDesc { + size: 8, + ..Default::default() + }); + let independent_src = graph.create_buffer(GraphBufferDesc { + size: 8, + ..Default::default() + }); + let independent_dst = graph.create_buffer(GraphBufferDesc { + size: 8, + ..Default::default() + }); + + graph.push(CopyBuffer { + src: a, + src_range: BufferRange { offset: 0, size: 8 }, + dst: shared, + dst_range: BufferRange { offset: 0, size: 8 }, + }); + graph.push(CopyBuffer { + src: shared, + src_range: BufferRange { offset: 0, size: 8 }, + dst: b, + dst_range: BufferRange { offset: 0, size: 8 }, + }); + graph.push(CopyBuffer { + src: independent_src, + src_range: BufferRange { offset: 0, size: 8 }, + dst: independent_dst, + dst_range: BufferRange { offset: 0, size: 8 }, + }); + + let compiled = graph.compile().expect("graph compiles"); + assert_eq!( + compiled.batches(), + &[ + vec![CommandIndex(0), CommandIndex(2)], + vec![CommandIndex(1)] + ] + ); + assert_eq!( + compiled.command_order(), + [CommandIndex(0), CommandIndex(2), CommandIndex(1)] + ); + assert_eq!(compiled.barriers().len(), 1); + assert_eq!(compiled.barriers()[0].before_command, CommandIndex(1)); + + let batch_for = |command| { + compiled + .batches() + .iter() + .position(|batch| batch.contains(&command)) + .expect("command appears in a batch") + }; + for barrier in compiled.barriers() { + assert!( + batch_for(CommandIndex(0)) < batch_for(barrier.before_command), + "barrier target follows its conflicting source in another batch" + ); + } + } + + #[test] + fn recording_steps_place_barriers_immediately_before_their_target_command() { + let first_barrier = PipelineBarrier { + before_command: CommandIndex(1), + resources: Vec::new(), + }; + let second_barrier = PipelineBarrier { + before_command: CommandIndex(1), + resources: Vec::new(), + }; + let barriers = [first_barrier, second_barrier]; + let batches = [vec![CommandIndex(0), CommandIndex(1)]]; + + let steps = recording_steps(&batches, &barriers).collect::>(); + assert!(matches!(steps[0], RecordingStep::Command(CommandIndex(0)))); + assert!( + matches!(steps[1], RecordingStep::Barrier(barrier) if std::ptr::eq(barrier, &barriers[0])) + ); + assert!( + matches!(steps[2], RecordingStep::Barrier(barrier) if std::ptr::eq(barrier, &barriers[1])) + ); + assert!(matches!(steps[3], RecordingStep::Command(CommandIndex(1)))); + } + + #[test] + fn barrier_for_later_command_follows_independent_command_in_same_batch() { + let mut graph = RenderGraph::new(); + let first_src = graph.create_buffer(GraphBufferDesc { + size: 8, + ..Default::default() + }); + let first_shared = graph.create_buffer(GraphBufferDesc { + size: 8, + ..Default::default() + }); + let first_dst = graph.create_buffer(GraphBufferDesc { + size: 8, + ..Default::default() + }); + let second_src = graph.create_buffer(GraphBufferDesc { + size: 8, + ..Default::default() + }); + let second_shared = graph.create_buffer(GraphBufferDesc { + size: 8, + ..Default::default() + }); + let second_dst = graph.create_buffer(GraphBufferDesc { + size: 8, + ..Default::default() + }); + + let copy = |src, dst| CopyBuffer { + src, + src_range: BufferRange { offset: 0, size: 8 }, + dst, + dst_range: BufferRange { offset: 0, size: 8 }, + }; + graph.push(copy(first_src, first_shared)); + graph.push(copy(second_src, second_shared)); + graph.push(copy(second_shared, second_dst)); + graph.push(copy(first_shared, first_dst)); + + let compiled = graph.compile().expect("graph compiles"); + assert_eq!( + compiled.batches(), + &[ + vec![CommandIndex(0), CommandIndex(1)], + vec![CommandIndex(2), CommandIndex(3)] + ] + ); + let steps = recording_steps(compiled.batches(), compiled.barriers()).collect::>(); + let command_two_position = steps + .iter() + .position(|step| matches!(step, RecordingStep::Command(CommandIndex(2)))) + .expect("independent command is recorded"); + let barrier_three_position = steps + .iter() + .position(|step| { + matches!(step, RecordingStep::Barrier(barrier) if barrier.before_command == CommandIndex(3)) + }) + .expect("barrier for command three is recorded"); + assert!(command_two_position < barrier_three_position); + } + + #[test] + fn dependency_chain_occupies_three_batches() { + let mut graph = RenderGraph::new(); + let a = graph.create_buffer(GraphBufferDesc { + size: 8, + ..Default::default() + }); + let b = graph.create_buffer(GraphBufferDesc { + size: 8, + ..Default::default() + }); + let c = graph.create_buffer(GraphBufferDesc { + size: 8, + ..Default::default() + }); + let d = graph.create_buffer(GraphBufferDesc { + size: 8, + ..Default::default() + }); + graph.push(CopyBuffer { + src: a, + src_range: BufferRange { offset: 0, size: 8 }, + dst: b, + dst_range: BufferRange { offset: 0, size: 8 }, + }); + graph.push(CopyBuffer { + src: b, + src_range: BufferRange { offset: 0, size: 8 }, + dst: c, + dst_range: BufferRange { offset: 0, size: 8 }, + }); + graph.push(CopyBuffer { + src: c, + src_range: BufferRange { offset: 0, size: 8 }, + dst: d, + dst_range: BufferRange { offset: 0, size: 8 }, + }); + + let compiled = graph.compile().expect("graph compiles"); + assert_eq!( + compiled.batches(), + &[ + vec![CommandIndex(0)], + vec![CommandIndex(1)], + vec![CommandIndex(2)] + ] + ); + for barrier in compiled.barriers() { + let source_command = CommandIndex(barrier.before_command.index() - 1); + let source_batch = compiled + .batches() + .iter() + .position(|batch| batch.contains(&source_command)) + .expect("conflicting source appears in a batch"); + let target_batch = compiled + .batches() + .iter() + .position(|batch| batch.contains(&barrier.before_command)) + .expect("barrier target appears in a batch"); + assert!(source_batch < target_batch); + } + } + #[test] fn texture_aliasing_size_uses_format_block_size() { let mut graph = RenderGraph::new();