-
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 all 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 |
|---|---|---|
|
|
@@ -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 | ||
|
|
@@ -410,6 +409,27 @@ impl OpType { | |
| (0..self.value_port_count(dir)).map(move |i| Port::new(dir, i)) | ||
| } | ||
|
|
||
| /// Return the dataflow value ports and their types for the given direction. | ||
| #[inline] | ||
| pub fn value_types(&self, dir: Direction) -> impl Iterator<Item = (Port, Type)> { | ||
| self.value_ports(dir) | ||
| .map(|port| (port, self.value_port_type(port).unwrap())) | ||
| } | ||
|
|
||
| /// Return the dataflow value input ports and their types. | ||
| #[inline] | ||
| pub fn value_input_types(&self) -> impl Iterator<Item = (IncomingPort, Type)> { | ||
| self.value_types(Direction::Incoming) | ||
| .map(|(port, typ)| (port.as_incoming().unwrap(), typ)) | ||
| } | ||
|
|
||
| /// Return the dataflow value output ports and their types. | ||
| #[inline] | ||
| pub fn value_output_types(&self) -> impl Iterator<Item = (OutgoingPort, Type)> { | ||
| self.value_types(Direction::Outgoing) | ||
| .map(|(port, typ)| (port.as_outgoing().unwrap(), typ)) | ||
| } | ||
|
|
||
| /// Return the dataflow value input ports for the given direction. | ||
| #[inline] | ||
| #[must_use] | ||
|
|
@@ -430,8 +450,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 +596,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 +744,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.
I'm wondering if we should have a shorthand for this on
HugrView.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.
For
get_optype(n).value_(in/out)put_type, forvalue_(in/out)put_type.expect, or all three? Not opposed to any of thoseThere 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.
Or
(Node) -> impl Iterator<Item=Type>? (+direction, or *2 for in/out)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.
Looking more into it, I think it's fine to leave the the port-specific type getters in
OpType.This PR already adds
HugrView::value_types(node, dir) -> Iterator<(Port, Type)>andin_/out_variants. That should be enough to simplify relevant calls.Uh oh!
There was an error while loading. Please reload this page.
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.
Hang on....HugrView already defines
hugr/hugr-core/src/hugr/views.rs
Line 496 in e609c66