-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Move Option<Box<_>>
into ExtraInstructionAttributes
.
#13127
Conversation
One or more of the following people are relevant to this code:
|
Pull Request Test Coverage Report for Build 10837025267Details
💛 - Coveralls |
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.
Thanks for doing this, it looks like a very sensible idea.
Previously, CircuitInstruction and PackedInstruction held the ExtraInstructionAttributes struct within an Option<Box<_>>. By putting the Option<Box<_>> inside ExtraInstructionAttributes, we can use the struct itself to manage its memory and provide access to the attributes behind a nicer interface. The size of ExtraInstructionAttributes should be the same size as the old Option<Box<ExtraInstructionAttributes>>, so this should not have memory implications.
- Use tuple struct. - Use 'Attributes' over 'Attrs'. - Add doc comment for internal 'ExtraAttributes' struct. - Add doc comments for methods. - Add setters for unit and duration.
919a3b9
to
943b142
Compare
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.
Thanks for this and the changes. I'll leave unmerged because I saw elsewhere that Ray was going to look too.
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.
This is all sensible to me, my only comment was about the usage of a map-like struct instead of a simple tuple struct but it seems it's already been corrected :)
This should be good as is. Thanks for working on it! 🚀
* Move Option<Box<_>> into ExtraInstructionAttributes. Previously, CircuitInstruction and PackedInstruction held the ExtraInstructionAttributes struct within an Option<Box<_>>. By putting the Option<Box<_>> inside ExtraInstructionAttributes, we can use the struct itself to manage its memory and provide access to the attributes behind a nicer interface. The size of ExtraInstructionAttributes should be the same size as the old Option<Box<ExtraInstructionAttributes>>, so this should not have memory implications. * Address review comments. - Use tuple struct. - Use 'Attributes' over 'Attrs'. - Add doc comment for internal 'ExtraAttributes' struct. - Add doc comments for methods. - Add setters for unit and duration. * Fix performance regression from unnecessary dict creation.
Summary
The primary motivation here is to encapsulate the hairy logic of managing memory for extra attributes. By moving the
Option<Box<_>>
into theExtraInstructionAttributes
struct itself, clients are no longer burdened with the responsibility of checking if theOption
contains a value, unwrapping it etc.; the semantics of that aren't relevant to clients likeDAGCircuit
andCircuitData
. Instead, theExtraInstructionAttributes
acts as a container for optional attributes, and can easily drop theBox
internally if all attributes are cleared.A give-away that this pattern could be helpful was
ExtraInstructionAttributes::new
's return type, which was previouslyOption<Self>
.Details and comments
The size of
ExtraInstructionAttributes
(now held by value by instructions) should be the same size as the oldOption<Box<ExtraInstructionAttributes>>
, so this should not have memory implications regarding the size of instructions.Note that
ExtraAttrs
is completely private to thecircuit_instruction
module, i.e. it's only an implementation detail and in no way part of the interface.