-
Notifications
You must be signed in to change notification settings - Fork 18
perf: Avoid cloning signatures for simple port checks #3149
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 6 commits
d06e853
2a6f020
2d2b13b
9d1b1b8
f515a47
3bb6709
a3cd08c
238ba09
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 | ||
|---|---|---|---|---|
|
|
@@ -648,16 +648,16 @@ impl<N: HugrNode> SiblingSubgraph<N> { | |||
| .iter() | ||||
| .map(|part| { | ||||
| let &(n, p) = part.iter().next().expect("is non-empty"); | ||||
| let sig = hugr.signature(n).expect("must have dataflow signature"); | ||||
| sig.port_type(p).cloned().expect("must be dataflow edge") | ||||
| let op = hugr.get_optype(n); | ||||
| op.value_input_type(p).expect("must be dataflow edge") | ||||
|
Comment on lines
+651
to
+652
Collaborator
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. I'm wondering if we should have a shorthand for this on
Contributor
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. For
Contributor
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. Or
Collaborator
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. Looking more into it, I think it's fine to leave the the port-specific type getters in This PR already adds
Contributor
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. Hang on....HugrView already defines hugr/hugr-core/src/hugr/views.rs Line 496 in e609c66
|
||||
| }) | ||||
| .collect_vec(); | ||||
| let output = self | ||||
| .outputs | ||||
| .iter() | ||||
| .map(|&(n, p)| { | ||||
| let sig = hugr.signature(n).expect("must have dataflow signature"); | ||||
| sig.port_type(p).cloned().expect("must be dataflow edge") | ||||
| let op = hugr.get_optype(n); | ||||
| op.value_output_type(p).expect("must be dataflow edge") | ||||
| }) | ||||
| .collect_vec(); | ||||
|
|
||||
|
|
@@ -1212,12 +1212,14 @@ fn get_edge_type<H: HugrView, P: Into<Port> + Copy>( | |||
| ports: &[(H::Node, P)], | ||||
| ) -> Option<Type> { | ||||
| let &(n, p) = ports.first()?; | ||||
| let edge_t = hugr.signature(n)?.port_type(p)?.clone(); | ||||
| let op = hugr.get_optype(n); | ||||
| let edge_t = op.value_port_type(p.into())?.clone(); | ||||
| ports | ||||
| .iter() | ||||
| .all(|&(n, p)| { | ||||
| hugr.signature(n) | ||||
| .is_some_and(|s| s.port_type(p) == Some(&edge_t)) | ||||
| hugr.get_optype(n) | ||||
| .value_port_type(p.into()) | ||||
| .is_some_and(|t| t == edge_t) | ||||
| }) | ||||
| .then_some(edge_t) | ||||
| } | ||||
|
|
||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -112,7 +112,7 @@ use std::cmp::Ordering; | |
|
|
||
| use crate::extension::simple_op::MakeExtensionOp; | ||
| use crate::extension::{ExtensionId, ExtensionRegistry}; | ||
| use crate::types::{EdgeKind, Signature, Substitution}; | ||
| use crate::types::{EdgeKind, Signature, Substitution, Type}; | ||
| use crate::{Direction, Node, OutgoingPort, Port}; | ||
| use crate::{IncomingPort, PortIndex}; | ||
| use handle::NodeHandle; | ||
|
|
@@ -316,14 +316,13 @@ impl OpType { | |
| /// See [`OpType::dataflow_signature`], [`OpType::static_port_kind`], and | ||
| /// [`OpType::other_port_kind`]. | ||
| pub fn port_kind(&self, port: impl Into<Port>) -> Option<EdgeKind> { | ||
| let signature = self.dataflow_signature().unwrap_or_default(); | ||
| let port: Port = port.into(); | ||
| let dir = port.direction(); | ||
| let port_count = signature.port_count(dir); | ||
| let port_count = self.value_port_count(dir); | ||
|
|
||
| // Dataflow ports | ||
| if port.index() < port_count { | ||
| return signature.port_type(port).cloned().map(EdgeKind::Value); | ||
| return OpTrait::value_port_type(self, port).map(EdgeKind::Value); | ||
| } | ||
|
|
||
| // Constant port | ||
|
|
@@ -430,8 +429,7 @@ impl OpType { | |
| #[inline] | ||
| #[must_use] | ||
| pub fn value_port_count(&self, dir: portgraph::Direction) -> usize { | ||
| self.dataflow_signature() | ||
| .map_or(0, |sig| sig.port_count(dir)) | ||
| OpTrait::value_port_count(self, dir) | ||
| } | ||
|
|
||
| /// The number of Value input ports. | ||
|
|
@@ -577,6 +575,37 @@ pub trait OpTrait: Sized + Clone { | |
| None | ||
| } | ||
|
|
||
| /// Returns the type of a value port. | ||
| /// | ||
| /// Implementations may override this to avoid constructing a complete | ||
| /// [`Signature`] when only one port type is needed. | ||
| fn value_port_type(&self, port: Port) -> Option<Type> { | ||
| self.dataflow_signature()?.port_type(port).cloned() | ||
| } | ||
|
|
||
| /// Returns the type of an input value port. | ||
| /// | ||
| /// Shorthand for `value_port_type(port.into())`. | ||
| fn value_input_type(&self, port: IncomingPort) -> Option<Type> { | ||
| self.value_port_type(port.into()) | ||
| } | ||
|
|
||
| /// Returns the type of an output value port. | ||
| /// | ||
| /// Shorthand for `value_port_type(port.into())`. | ||
| fn value_output_type(&self, port: OutgoingPort) -> Option<Type> { | ||
| self.value_port_type(port.into()) | ||
| } | ||
|
|
||
| /// Returns the number of value ports in one direction. | ||
| /// | ||
| /// Implementations may override this to avoid constructing a complete | ||
| /// [`Signature`] when only its size is needed. | ||
| fn value_port_count(&self, dir: Direction) -> usize { | ||
| self.dataflow_signature() | ||
| .map_or(0, |signature| signature.port_count(dir)) | ||
| } | ||
|
Comment on lines
+599
to
+628
Collaborator
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. The new OpTrait methods are here. |
||
|
|
||
| /// The edge kind for the non-dataflow inputs of the operation, | ||
| /// not described by the signature. | ||
| /// | ||
|
|
@@ -694,3 +723,99 @@ macro_rules! impl_validate_op { | |
| } | ||
|
|
||
| use impl_validate_op; | ||
|
|
||
| #[cfg(test)] | ||
| mod test { | ||
| use super::*; | ||
| use crate::types::{PolyFuncType, Type}; | ||
| use rstest::rstest; | ||
|
|
||
| fn signature() -> Signature { | ||
| Signature::new([Type::UNIT, Type::new_unit_sum(2)], [Type::new_unit_sum(3)]) | ||
| } | ||
|
|
||
| fn input() -> OpType { | ||
| Input { | ||
| types: signature().input, | ||
| } | ||
| .into() | ||
| } | ||
|
|
||
| fn output() -> OpType { | ||
| Output { | ||
| types: signature().output, | ||
| } | ||
| .into() | ||
| } | ||
|
|
||
| fn call_indirect() -> OpType { | ||
| CallIndirect { | ||
| signature: signature(), | ||
| } | ||
| .into() | ||
| } | ||
|
|
||
| fn load_constant() -> OpType { | ||
| LoadConstant { | ||
| datatype: Type::new_unit_sum(2), | ||
| } | ||
| .into() | ||
| } | ||
|
|
||
| fn load_function() -> OpType { | ||
| let instantiation = signature(); | ||
| LoadFunction { | ||
| func_sig: PolyFuncType::new(Vec::new(), instantiation.clone()), | ||
| type_args: Vec::new(), | ||
| instantiation, | ||
| } | ||
| .into() | ||
| } | ||
|
|
||
| fn tag() -> OpType { | ||
| Tag::new(1, vec![vec![Type::UNIT].into(), signature().input]).into() | ||
| } | ||
|
|
||
| fn tail_loop() -> OpType { | ||
| TailLoop { | ||
| just_inputs: vec![Type::UNIT].into(), | ||
| just_outputs: vec![Type::new_unit_sum(2)].into(), | ||
| rest: vec![Type::new_unit_sum(3)].into(), | ||
| } | ||
| .into() | ||
| } | ||
|
|
||
| fn conditional() -> OpType { | ||
| Conditional { | ||
| sum_rows: vec![vec![Type::UNIT].into(), vec![Type::new_unit_sum(2)].into()], | ||
| other_inputs: signature().input, | ||
| outputs: signature().output, | ||
| } | ||
| .into() | ||
| } | ||
|
|
||
| /// Borrow-first queries must remain equivalent to the public signature API. | ||
| #[rstest] | ||
| #[case::input(input())] | ||
| #[case::output(output())] | ||
| #[case::call_indirect(call_indirect())] | ||
| #[case::load_constant(load_constant())] | ||
| #[case::load_function(load_function())] | ||
| #[case::tag(tag())] | ||
| #[case::tail_loop(tail_loop())] | ||
| #[case::conditional(conditional())] | ||
| fn value_ports_match_signature(#[case] op: OpType) { | ||
|
Contributor
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. Good test. You might want to consider |
||
| let signature = op.dataflow_signature().expect("dataflow operation"); | ||
|
|
||
| for dir in [Direction::Incoming, Direction::Outgoing] { | ||
| assert_eq!(op.value_port_count(dir), signature.port_count(dir)); | ||
| for index in 0..signature.port_count(dir) { | ||
| let port = Port::new(dir, index); | ||
| assert_eq!( | ||
| op.port_kind(port), | ||
| signature.port_type(port).cloned().map(EdgeKind::Value) | ||
| ); | ||
| } | ||
| } | ||
| } | ||
| } | ||
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.
yeah, definitely feels like giving back types as well as ports would be good here