From ad285f5f3840488ca7522a32f1b6b0140ea04bb8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Agust=C3=ADn=20Borgna?= <121866228+aborgna-q@users.noreply.github.com> Date: Mon, 20 Jan 2025 13:46:02 +0000 Subject: [PATCH] chore!: Update to `portgraph 0.13` / `petgraph 0.7` (#1878) The new portgraph release comes with some perf improvements. We require this update for #1869. Closes #1872. I added a benchmark variation for subgraphs with few nodes as a drive-by. This got improved by the new portgraph (skipping a full graph traversal), but the runtime is still mostly the `O(n)` full-graph traversal required for the convexity checking when building the subgraph. BREAKING CHANGE: Bumped public dependency `portgraph` to breaking version `0.13`. --- Cargo.toml | 4 +- hugr-core/src/hugr/views/descendants.rs | 4 +- hugr-core/src/hugr/views/sibling.rs | 4 +- hugr-core/src/hugr/views/sibling_subgraph.rs | 52 ++++++++++++++------ hugr/benches/benchmarks/subgraph.rs | 30 +++++++++++ 5 files changed, 72 insertions(+), 22 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 268f95e49..25ec5a5c2 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -31,7 +31,7 @@ missing_docs = "warn" debug_assert_with_mut_call = "warn" [workspace.dependencies] -portgraph = { version = "0.12.2" } +portgraph = { version = "0.13.0" } insta = { version = "1.34.0" } bitvec = "1.0.1" cgmath = "0.18.0" @@ -47,7 +47,7 @@ jsonschema = "0.27.0" lazy_static = "1.4.0" num-rational = "0.4.1" paste = "1.0" -petgraph = { version = "0.6.3", default-features = false } +petgraph = { version = "0.7.1", default-features = false } proptest = "1.4.0" proptest-derive = "0.5.0" regex = "1.9.5" diff --git a/hugr-core/src/hugr/views/descendants.rs b/hugr-core/src/hugr/views/descendants.rs index 78500c9f5..fcf77a04d 100644 --- a/hugr-core/src/hugr/views/descendants.rs +++ b/hugr-core/src/hugr/views/descendants.rs @@ -137,7 +137,7 @@ where let hugr = hugr.base_hugr(); Ok(Self { root, - graph: RegionGraph::new_region(&hugr.graph, &hugr.hierarchy, root.pg_index()), + graph: RegionGraph::new(&hugr.graph, &hugr.hierarchy, root.pg_index()), hugr, _phantom: std::marker::PhantomData, }) @@ -247,7 +247,7 @@ pub(super) mod test { Some(Signature::new(vec![usize_t()], vec![usize_t()])) ); assert_eq!(inner_region.node_count(), 3); - assert_eq!(inner_region.edge_count(), 2); + assert_eq!(inner_region.edge_count(), 1); assert_eq!(inner_region.children(inner).count(), 2); assert_eq!(inner_region.children(hugr.root()).count(), 0); assert_eq!( diff --git a/hugr-core/src/hugr/views/sibling.rs b/hugr-core/src/hugr/views/sibling.rs index 2ad2c735b..34452d66a 100644 --- a/hugr-core/src/hugr/views/sibling.rs +++ b/hugr-core/src/hugr/views/sibling.rs @@ -149,7 +149,7 @@ impl<'a, Root: NodeHandle> SiblingGraph<'a, Root> { let hugr = hugr.base_hugr(); Self { root, - graph: FlatRegionGraph::new_flat_region(&hugr.graph, &hugr.hierarchy, root.pg_index()), + graph: FlatRegionGraph::new(&hugr.graph, &hugr.hierarchy, root.pg_index()), hugr, _phantom: std::marker::PhantomData, } @@ -249,7 +249,7 @@ impl<'g, Root: NodeHandle> HugrInternals for SiblingMut<'g, Root> { Root: 'p; fn portgraph(&self) -> Self::Portgraph<'_> { - FlatRegionGraph::new_flat_region( + FlatRegionGraph::new( &self.base_hugr().graph, &self.base_hugr().hierarchy, self.root.pg_index(), diff --git a/hugr-core/src/hugr/views/sibling_subgraph.rs b/hugr-core/src/hugr/views/sibling_subgraph.rs index 470cd762a..bb967ac9c 100644 --- a/hugr-core/src/hugr/views/sibling_subgraph.rs +++ b/hugr-core/src/hugr/views/sibling_subgraph.rs @@ -15,6 +15,7 @@ use std::mem; use itertools::Itertools; use portgraph::algorithms::ConvexChecker; +use portgraph::boundary::Boundary; use portgraph::{view::Subgraph, Direction, PortView}; use thiserror::Error; @@ -179,14 +180,8 @@ impl SiblingSubgraph { ) -> Result { let pg = hugr.portgraph(); - let to_pg = |(n, p): (Node, Port)| { - pg.port_index(n.pg_index(), p.pg_offset()) - .expect("invalid port") - }; - // Ordering of the edges here is preserved and becomes ordering of the signature. - let subpg = - Subgraph::new_subgraph(pg.clone(), combine_in_out(&inputs, &outputs).map(to_pg)); + let subpg = Subgraph::new_subgraph(pg.clone(), make_boundary(hugr, &inputs, &outputs)); let nodes = subpg.nodes_iter().map_into().collect_vec(); validate_subgraph(hugr, &nodes, &inputs, &outputs)?; @@ -414,7 +409,7 @@ impl SiblingSubgraph { .is_some_and(|s| s.port_type(p).is_some()) }); - if combine_in_out(&vec![out_order_ports], &in_order_ports) + if iter_io(&vec![out_order_ports], &in_order_ports) .any(|(n, p)| is_order_edge(&replacement, n, p)) { unimplemented!("Found state order edges in replacement graph"); @@ -485,15 +480,40 @@ impl SiblingSubgraph { } } -fn combine_in_out<'a>( +/// Returns an iterator over the input ports. +fn iter_incoming(inputs: &IncomingPorts) -> impl Iterator + '_ { + inputs.iter().flat_map(|part| part.iter().copied()) +} + +/// Returns an iterator over the output ports. +fn iter_outgoing(outputs: &OutgoingPorts) -> impl Iterator + '_ { + outputs.iter().copied() +} + +/// Returns an iterator over both incoming and outgoing ports. +fn iter_io<'a>( inputs: &'a IncomingPorts, outputs: &'a OutgoingPorts, ) -> impl Iterator + 'a { - inputs - .iter() - .flatten() - .map(|(n, p)| (*n, (*p).into())) - .chain(outputs.iter().map(|(n, p)| (*n, (*p).into()))) + iter_incoming(inputs) + .map(|(n, p)| (n, Port::from(p))) + .chain(iter_outgoing(outputs).map(|(n, p)| (n, Port::from(p)))) +} + +fn make_boundary<'a>( + hugr: &impl HugrView, + inputs: &'a IncomingPorts, + outputs: &'a OutgoingPorts, +) -> Boundary { + let to_pg_index = |n: Node, p: Port| { + hugr.portgraph() + .port_index(n.pg_index(), p.pg_offset()) + .unwrap() + }; + Boundary::new( + iter_incoming(inputs).map(|(n, p)| to_pg_index(n, p.into())), + iter_outgoing(outputs).map(|(n, p)| to_pg_index(n, p.into())), + ) } /// Precompute convexity information for a HUGR. @@ -591,11 +611,11 @@ fn validate_subgraph( } // Check there are no linked "other" ports - if combine_in_out(inputs, outputs).any(|(n, p)| is_order_edge(hugr, n, p)) { + if iter_io(inputs, outputs).any(|(n, p)| is_order_edge(hugr, n, p)) { unimplemented!("Connected order edges not supported at the boundary") } - let boundary_ports = combine_in_out(inputs, outputs).collect_vec(); + let boundary_ports = iter_io(inputs, outputs).collect_vec(); // Check that the boundary ports are all in the subgraph. if let Some(&(n, p)) = boundary_ports.iter().find(|(n, _)| !node_set.contains(n)) { Err(InvalidSubgraphBoundary::PortNodeNotInSet(n, p))?; diff --git a/hugr/benches/benchmarks/subgraph.rs b/hugr/benches/benchmarks/subgraph.rs index 58ff24cdd..a24400a23 100644 --- a/hugr/benches/benchmarks/subgraph.rs +++ b/hugr/benches/benchmarks/subgraph.rs @@ -30,6 +30,35 @@ fn bench_singleton_subgraph(c: &mut Criterion) { group.finish(); } +fn bench_fewnode_subgraph(c: &mut Criterion) { + let mut group = c.benchmark_group("fewnode_subgraph"); + group.plot_config(PlotConfiguration::default().summary_scale(AxisScale::Logarithmic)); + + let num_layers = [10, 100, 1000]; + + for layers in num_layers.into_iter() { + let (hugr, layer_ids) = circuit(layers); + + // Get a subgraph with a fixed number of nodes in the middle of the circuit. + group.bench_with_input( + BenchmarkId::from_parameter(layers), + &layers, + |b, &_layers| { + // Pick all the nodes in four layers in the middle of the circuit. + let nodes: Vec<_> = layer_ids + .iter() + .skip(layers / 2) + .take(4) + .flat_map(|ids| [ids.h, ids.cx1, ids.cx2]) + .collect(); + b.iter(|| black_box(SiblingSubgraph::try_from_nodes(nodes.clone(), &hugr))) + }, + ); + } + + group.finish(); +} + fn bench_multinode_subgraph(c: &mut Criterion) { let mut group = c.benchmark_group("multinode_subgraph"); group.plot_config(PlotConfiguration::default().summary_scale(AxisScale::Logarithmic)); @@ -59,5 +88,6 @@ criterion_group! { config = Criterion::default(); targets = bench_singleton_subgraph, + bench_fewnode_subgraph, bench_multinode_subgraph, }