response to llm review

crates/renderer_rendergraph/src/graph.rs

@@ -156,14 +156,15 @@ impl Kahn {
         }
     }
 
-    fn pop_ready(&mut self) -> Option<usize> {
+    fn pop_batch(&mut self) -> Vec<usize> {
+        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::<Vec<_>>();
+        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<Vec<CommandIndex>, GraphError> {
+    fn topo_batches(&self) -> Result<Vec<Vec<CommandIndex>>, 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);
-        }
-        if !kahn.is_complete() {
+        let mut batches = Vec::new();
+        while !kahn.is_complete() {
+            let batch = kahn.pop_batch();
+            if batch.is_empty() {
                 return Err(GraphError::DependencyCycle);
             }
-        Ok(order)
+            let batch = batch
+                .into_iter()
+                .inspect(|&index| kahn.complete(index))
+                .map(CommandIndex::from)
+                .collect();
+            batches.push(batch);
+        }
+        Ok(batches)
     }
 
     fn validate_resources(&self) -> Result<(), GraphError> {
@@ -320,7 +328,17 @@ impl<'a> GraphCompiler<'a> {
         Ok(())
     }
 
-    fn build_barriers(&self, order: &[CommandIndex]) -> Vec<PipelineBarrier> {
+    fn build_barriers(
+        &self,
+        order: &[CommandIndex],
+        batches: &[Vec<CommandIndex>],
+    ) -> Vec<PipelineBarrier> {
+        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_map(|&prior_index| {
+                        self.accesses[prior_index.index()]
+                            .iter()
+                            .rev()
                             .find(|previous| {
                                 previous.resource == current.resource
                                     && previous.scope.conflicts(&current.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<ResourceDesc>,
     resolved: HashMap<ResourceId, ResolvedResource>,
     commands: CommandList,
-    order: Vec<CommandIndex>,
+    batches: Vec<Vec<CommandIndex>>,
+    flat_order: Vec<CommandIndex>,
     barriers: Vec<PipelineBarrier>,
     lifetimes: HashMap<ResourceId, ResourceLifetime>,
     interference: ResourceInterference,
@@ -487,8 +527,12 @@ enum ResolvedResource {
 }
 
 impl CompiledGraph {
+    pub fn batches(&self) -> &[Vec<CommandIndex>] {
+        &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)?;
                 }
+            }
+        }
         Ok(())
     }
 }
 
+enum RecordingStep<'a> {
+    Barrier(&'a PipelineBarrier),
+    Command(CommandIndex),
+}
+
+fn recording_steps<'a>(
+    batches: &'a [Vec<CommandIndex>],
+    barriers: &'a [PipelineBarrier],
+) -> impl Iterator<Item = RecordingStep<'a>> + '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);
+        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::<Vec<_>>();
+        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::<Vec<_>>();
+        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();