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.

greg7mdp · 2025-02-07T20:22:31Z

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

+      static mutable_block_ptr create_mutable_block(const signed_block_header& h) { return std::unique_ptr<signed_block>(new signed_block(h)); }
+      static signed_block_ptr  create_signed_block(mutable_block_ptr&& b) { b->pack(); return signed_block_ptr{std::move(b)}; }


I don't see why these should be inside struct signed_block. even if pack() has tobe made public.

It is also a little strange to me that bytes packed_block; is inside signed_block? Why not have a separate type packed_signed_block? Maybe it would require more changes?

They are constructor like methods. Seems like they belong in signed_block, also allows pack() to be private without adding friends of the external functions.

We emit/signal signed_block out to plugins. signed_block is a well known and used type. I see this as an internal cached optimization to make packed_signed_block() efficient. Creating a different type means that we have to pass around a packed_signed_block everywhere we use signed_block today.

greg7mdp · 2025-02-07T20:32:13Z

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

+   bool read( char* d, size_t s ) {
+      if (ds.read(d, s)) {
+         auto size = mirror.size();
+         mirror.resize(size + s);


This is potentially very inefficient. I suggest mirror.resize(std::bit_ceil(size + s));

greg7mdp · 2025-02-07T22:59:58Z

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

+      template<typename Stream> friend void fc::raw::unpack(Stream& s, eosio::chain::signed_block& v);
+      void pack() { packed_block = fc::raw::pack( *this ); }
+
+      bytes packed_block; // packed this


should this be a std::future<bytes>, so we can post the packed_block = fc::raw::pack( *this ); to a thread pool?

greg7mdp · 2025-02-07T23:08:29Z

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

+            fc::reflector<eosio::chain::signed_block>::visit( fc::raw::detail::unpack_object_visitor<Stream, eosio::chain::signed_block>( v, s ) );
+            v.packed_block = s.extract_mirror();
+         } else {
+            fc::datastream_mirror<Stream> ds(s, sizeof(eosio::chain::signed_block) + 4096);


I don't think the sizeof(eosio::chain::signed_block) is very significant since most of the memory used is likely in the deque<transaction_receipt> transactions; whose sizeof() is probably around 16. I think I would just simplify to just 4096, but it probably doesn't matter much and is fine as is.

greg7mdp · 2025-02-07T23:13:48Z

libraries/chain/block_log.cpp

               if (pos != block_log::npos) {
                  block_file.seek(pos);
-                  return read_block(block_file, block_num);
+                  fc::datastream_mirror ds(block_file, size);


Cool, here we dimension the mirror vector to exactly the right size.

greg7mdp · 2025-02-07T23:22:16Z

plugins/net_plugin/net_plugin.cpp

+      auto mb_ds = pending_message_buffer.create_datastream();
+      fc::raw::unpack( mb_ds, which );
+
+      fc::datastream_mirror ds(mb_ds, message_length);


cool, good dimensioning of the vector here as well. I see that my suggestion on the resize() was not as critical as I originally thought.

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

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

greg7mdp Feb 7, 2025

heifner Feb 7, 2025

greg7mdp Feb 7, 2025

greg7mdp Feb 7, 2025

greg7mdp Feb 7, 2025

greg7mdp Feb 7, 2025

greg7mdp Feb 7, 2025

		static mutable_block_ptr create_mutable_block(const signed_block_header& h) { return std::unique_ptr<signed_block>(new signed_block(h)); }
		static signed_block_ptr create_signed_block(mutable_block_ptr&& b) { b->pack(); return signed_block_ptr{std::move(b)}; }

Performance: Store packed block in signed_block #1148

Are you sure you want to change the base?

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

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