Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Performance: Store packed block in signed_block #1148
Performance: Store packed block in signed_block #1148
Changes from all commits
4fa1edf
002f31c
0d772f3
d00a4f7
6ae62d8
c0b6443
21b06bd
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Not from your changes. I wish
ro_stream_and_size_for_block
returnedds
andsize
explicitly.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.
Now it is cleaner.
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.
Why not keep the original name
mutable_signed_block_ptr
? That's more clearer.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.
Because in most cases it is not "signed" yet.
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.
Can you add a comment? We have other cases where mutable and non-mutable use the same root like
mutale_db()
anddb()
.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.
c0b6443
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.
Why not put this code some where in fc?
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.
Not sure I follow.
signed_block
is defined inlibraries/chain
which depends onlibraries/libfc
.I did try to move the forward declare out of
fc
, but couldn't get it to work without thefc
forward declare.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 meant
unpack(Stream& s, eosio::chain::signed_block& v)
is defined inraw_fwd.hpp
, why not put the implementation there. I think about it more. It is also OK to remain 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.
Don't quite get this code. Why the
if constexpr
check? Why the4096
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.
Because if
signed_block
is being unpacked with adatastream_mirror
then we can use theextract_mirror()
to populatesigned_block::packed_block
. If a different Stream type is being used, then wrap it in adatastream_mirror
to capture the packed block data. The 4096 is used to reserve a size in the vector; open to removing the+ 4096
if we think it might be wasteful.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. I got confused by the method name is
unpack
but then it setspacked_block
. Add a comment about 4096.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.
Why this complicated?
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.
The idea is to make it difficult to misuse
signed_block
. We wantpacked_block
data ofsigned_block
to always be populated. Could create a method ofsigned_block
that just does this, but since that would only be useful in tests I didn't.