Introduce a gate name interner to avoid duplicating String in the Target
#12917
Conversation
… instances. - Remove `variable_operation_class`, use enum nature to discriminate. - Hide `gate_name_map` from Python, use `operation_from_name`. - Other small tweaks and fixes.
|
One or more of the following people are relevant to this code:
|
Pull Request Test Coverage Report for Build 10928640225Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
There was a problem hiding this comment.
I'm glad to see this, I meant to open an issue from: #12292 (comment) but it's good you beat me to that. I haven't had a chance to review the code in depth yet, but I had a high level question inline off the top.
| qarg_gate_map: NullableIndexMap<Qargs, Option<HashSet<String>>>, | ||
| non_global_strict_basis: Option<Vec<String>>, | ||
| non_global_basis: Option<Vec<String>>, | ||
| gate_name_map: IndexMap<usize, TargetOperation, RandomState>, |
There was a problem hiding this comment.
One small question here, is there a reason we the full width of a usize? It is natural because that's the index type for IndexSet, but I'm pretty confident we don't the full 64 bit (or 32 bit on those platform) address space. Realistically I feel like we could get away with a u8 because it seems unlikely for a target to support more than >255 gates at once. If we wanted to be a bit conservative we could use a u16, there is almost no possibility of a target having over 65k gates names it.
There was a problem hiding this comment.
I can imagine a world with >255 operation names, if we had something like multiple fractional 2q gates (heterogeneous arch) exposed both continuously and with various precisely calibrated points on the parametrisation.
There was a problem hiding this comment.
255 is more than it sounds like. I would be surprised if anyone spent the time to calibrate all those different variants even on a far future system. But I also agree with you that it is within the realm of plausibility, however u16::MAX is definitely sufficiently large though so I think using a u16 newtype here for the target string index would make a lot of sense.
There was a problem hiding this comment.
This makes sense. Although the default indexing unit for IndexSet is mainly usize, I see how it could help to use a smaller datatype to store these indices in.
|
Ray: I don't know your exact plans for the type here, but I think you should be able to create an |
|
Then all the magic |
|
Hey @jakelishman yes I'm currently playing with it. However, there are certain traits that I will need to implement beforehand. Will get back to you on this. |
|
The concept of the existing |
|
The efforts for this PR will be superseded by the results of #13308, in which we will hopefully address this. |
Summary
The current implementation of
Targetin Rust, performs a lot of cloning when adding an instruction to the Target. These commits allow it to keep one copy of theseStringinstances and have copy-able (usize) values represent them in the other structures of the targetDetails and comments
The following commits add an interner which enable us to avoid multiple cloning of attributes with repeated use in multiple mapping structures within the target; In this case, with the gate names.
These changes are made along the following other changes:
variable_operation_class, use enum nature to discriminate.gate_name_mapfrom Python, useoperation_from_name.Known issues:
Nothing so far, make sure to bring up during review.