-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Avoid unnecessary copying and dynamic memory allocations #4261
Conversation
Co-authored-by: Chenna Keshava B S <ckbs.keshava56@gmail.com>
if (sig.size() == 0) | ||
return false; |
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 don't think this added check is needed as this is taken care of by ecdsaCanonicality
below.
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.
Agreed, I'll remove it.
PublicKey publicKey_; | ||
uint256 suppression_; | ||
Proposal proposal_; | ||
boost::container::static_vector<std::uint8_t, 72> signature_; |
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 have a hypothetical question. If the size of the signature_
variable was a multiple of 32 bytes, then
can we use a ripple::base_uint<33>
for storing the signature_
data? Will that be slower than boost::static_vector
?
Is one of them faster than the other?
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.
ripple::base_uint<33>
would not work as it specifies 33 bits (you need 72 bytes or 72 x 8 bits), and it is designed to store uint32_t not an array of bytes. Also base_uint
doesn't manage a size, so you'd have to keep track separately of how long the signature is. static_vector
is the right choice here!
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 see okay, thanks for the clarification.
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.
Nice job.
This commit implements an optimization that @chennakeshava1998 and I identified; this commit reduces unnecessary dynamic memory allocations when processing proposals.