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

@@ -85,21 +85,26 @@ namespace eosio { namespace chain {

   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

@@ -1590,7 +1578,7 @@ struct controller_impl {

               // 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

@@ -129,3 +142,20 @@ FC_REFLECT_DERIVED(eosio::chain::transaction_receipt, (eosio::chain::transaction
 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

@@ -36,7 +36,7 @@ struct block_log_fixture {
      }
      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

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Performance: Store packed block in signed_block #1148

Performance: Store packed block in signed_block #1148

heifner commented Feb 6, 2025

linh2931 Feb 6, 2025

heifner Feb 6, 2025

linh2931 Feb 6, 2025

heifner Feb 6, 2025

linh2931 Feb 6, 2025

heifner Feb 6, 2025

linh2931 Feb 6, 2025

linh2931 Feb 6, 2025

linh2931 Feb 6, 2025

heifner Feb 6, 2025

linh2931 Feb 6, 2025

linh2931 Feb 6, 2025

heifner Feb 6, 2025

linh2931 Feb 6, 2025

ericpassmore commented Feb 6, 2025

greg7mdp left a comment

Performance: Store packed block in signed_block #1148

Performance: Store packed block in signed_block #1148

Conversation

heifner commented Feb 6, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericpassmore commented Feb 6, 2025

greg7mdp left a comment

Choose a reason for hiding this comment