Performance: Store packed block in signed_block #1148

heifner · 2025-02-06T13:00:54Z

Cache the packed block data in signed_block so that it can be retrieved without having to pack it at time of serialization.

Resolves #1062

…ta as it reads.

…ock was the correct block id. Not used, so just removed it.

…block so it does not need to be re-packed when needed for the block log or for P2P. Refactor signed_block construction so that it is harder to use incorrectly. It is necessary to always store the packed block in case it is needed. Specialize signed_block unpack so that it always fills in the packed_block of signed_block.

linh2931 · 2025-02-06T14:05:39Z

libraries/chain/include/eosio/chain/block.hpp

   using block_extension = block_extension_types::block_extension_t;

+   using signed_block_ptr = std::shared_ptr<const signed_block>;
+   using mutable_block_ptr = std::unique_ptr<signed_block>;


Why not keep the original name mutable_signed_block_ptr? That's more clearer.

Because in most cases it is not "signed" yet.

Can you add a comment? We have other cases where mutable and non-mutable use the same root like mutale_db() and db().

linh2931 · 2025-02-06T14:10:56Z

libraries/chain/include/eosio/chain/block.hpp

+   template <typename Stream>
+   void unpack(Stream& s, eosio::chain::signed_block& v) {
+      try {
+         if constexpr (requires { s.extract_mirror(); }) {


Don't quite get this code. Why the if constexpr check? Why the 4096 below?

Because if signed_block is being unpacked with a datastream_mirror then we can use the extract_mirror() to populate signed_block::packed_block. If a different Stream type is being used, then wrap it in a datastream_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.

Thanks. I got confused by the method name is unpack but then it sets packed_block. Add a comment about 4096.

linh2931 · 2025-02-06T14:11:25Z

libraries/chain/controller.cpp

               // blog.append could fail due to failures like running out of space.
               // Do it before commit so that in case it throws, DB can be rolled back.
-               blog.append( (*bitr)->block, (*bitr)->id(), irreversible_mode() ? f.get() : it++->get() );
+               blog.append( (*bitr)->block, (*bitr)->id(), (*bitr)->block->packed_signed_block() );


Now it is cleaner.

linh2931 · 2025-02-06T14:16:24Z

libraries/chain/include/eosio/chain/block.hpp

 FC_REFLECT(eosio::chain::additional_block_signatures_extension, (signatures));
 FC_REFLECT(eosio::chain::quorum_certificate_extension, (qc));
 FC_REFLECT_DERIVED(eosio::chain::signed_block, (eosio::chain::signed_block_header), (transactions)(block_extensions) )
+


Why not put this code some where in fc?

Not sure I follow. signed_block is defined in libraries/chain which depends on libraries/libfc.
I did try to move the forward declare out of fc, but couldn't get it to work without the fc forward declare.

I meant unpack(Stream& s, eosio::chain::signed_block& v) is defined in raw_fwd.hpp, why not put the implementation there. I think about it more. It is also OK to remain here.

linh2931 · 2025-02-06T14:20:31Z

tests/block_log.cpp

      else {
         eosio::chain::genesis_state gs;
-         log->reset(gs, std::make_shared<eosio::chain::signed_block>());
+         log->reset(gs, eosio::chain::signed_block::create_signed_block(eosio::chain::signed_block::create_mutable_block({})));


Why this complicated?

The idea is to make it difficult to misuse signed_block. We want packed_block data of signed_block to always be populated. Could create a method of signed_block that just does this, but since that would only be useful in tests I didn't.

linh2931 · 2025-02-06T14:30:58Z

libraries/chain/block_log.cpp

-               return read_block(*ds, block_num);
+            uint64_t block_size = 0;
+
+            auto ds = catalog.ro_stream_and_size_for_block(block_num, block_size);


Not from your changes. I wish ro_stream_and_size_for_block returned ds and size explicitly.

ericpassmore · 2025-02-06T15:28:46Z

Note:start
category: Other
component: Internal
summary: Cache the packed block data in signed_block so that it can be retrieved without having to pack it at time of serialization.
Note:end

greg7mdp

I think I know why adding the:
namespace eosio::chain { struct signed_block; }

at the top of block.hpp didn't work. It is because some files like block_state_legacy.cpp include block_state_legacy.hpp first, and then block_state_legacy.hpp includes block_header_state_legacy.hpp before block.hpp.

So for files like this the raw_fwd.hpp is included before the forward delaration of signed_block is seen.

libraries/chain/include/eosio/chain/block.hpp

libraries/libfc/include/fc/io/datastream.hpp

libraries/chain/include/eosio/chain/block.hpp

libraries/chain/block_log.cpp

plugins/net_plugin/net_plugin.cpp

gdb4713

Ok

gdb4713

2

heifner added 5 commits February 5, 2025 10:22

GH-1062 Add new datastream_mirror which keeps a copy of the stream da…

4fa1edf

…ta as it reads.

GH-1062 Remove unused read_block_by_id. It did not verify the read bl…

002f31c

…ock was the correct block id. Not used, so just removed it.

GH-1062 Spell out construction desired for gcc

d00a4f7

GH-1062 Update test to use create_signed_block

6ae62d8

heifner added the OCI Work exclusive to OCI team label Feb 6, 2025

heifner requested review from greg7mdp and linh2931 February 6, 2025 13:00

linh2931 reviewed Feb 6, 2025

View reviewed changes

ericpassmore added the performance label Feb 6, 2025

GH-1062 Add comment

c0b6443

linh2931 approved these changes Feb 6, 2025

View reviewed changes

greg7mdp reviewed Feb 7, 2025

View reviewed changes

libraries/chain/include/eosio/chain/block.hpp Show resolved Hide resolved

libraries/libfc/include/fc/io/datastream.hpp Show resolved Hide resolved

greg7mdp reviewed Feb 7, 2025

View reviewed changes

libraries/chain/include/eosio/chain/block.hpp Show resolved Hide resolved

libraries/chain/include/eosio/chain/block.hpp Show resolved Hide resolved

libraries/chain/block_log.cpp Show resolved Hide resolved

plugins/net_plugin/net_plugin.cpp Show resolved Hide resolved

gdb4713 approved these changes Feb 10, 2025

View reviewed changes

GH-1062 Grow by bit_ceil if needed

21b06bd

greg7mdp approved these changes Feb 10, 2025

View reviewed changes

heifner merged commit 5247431 into main Feb 13, 2025
36 checks passed

heifner deleted the GH-1062-packed-signed_block branch February 13, 2025 13:58

gdb4713 reviewed Mar 5, 2025

View reviewed changes

heifner mentioned this pull request Jul 22, 2025

P2P: Use cached packed signed_block on send #1743

Merged

Performance: Store packed block in signed_block #1148

Performance: Store packed block in signed_block #1148

Uh oh!

Conversation

heifner commented Feb 6, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ericpassmore commented Feb 6, 2025

Uh oh!

greg7mdp left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gdb4713 left a comment

Choose a reason for hiding this comment

Uh oh!

gdb4713 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants