-
Notifications
You must be signed in to change notification settings - Fork 15
Performance: Store packed block in signed_block #1148
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
Changes from 6 commits
4fa1edf
002f31c
0d772f3
d00a4f7
6ae62d8
c0b6443
21b06bd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -661,10 +661,11 @@ namespace eosio { namespace chain { | |
|
||
signed_block_ptr read_block_by_num(uint32_t block_num) final { | ||
try { | ||
uint64_t pos = get_block_pos(block_num); | ||
auto [ pos, size ] = get_block_position_and_size(block_num); | ||
if (pos != block_log::npos) { | ||
block_file.seek(pos); | ||
return read_block(block_file, block_num); | ||
fc::datastream_mirror ds(block_file, size); | ||
return read_block(ds, block_num); | ||
} | ||
return retry_read_block_by_num(block_num); | ||
} | ||
|
@@ -801,7 +802,7 @@ namespace eosio { namespace chain { | |
|
||
void reset(const genesis_state& gs, const signed_block_ptr& first_block) override { | ||
this->reset(1, gs, default_initial_version); | ||
this->append(first_block, first_block->calculate_id(), fc::raw::pack(*first_block)); | ||
this->append(first_block, first_block->calculate_id(), first_block->packed_signed_block()); | ||
} | ||
|
||
void reset(const chain_id_type& chain_id, uint32_t first_block_num) override { | ||
|
@@ -1094,9 +1095,13 @@ namespace eosio { namespace chain { | |
} | ||
|
||
signed_block_ptr retry_read_block_by_num(uint32_t block_num) final { | ||
auto ds = catalog.ro_stream_for_block(block_num); | ||
if (ds) | ||
return read_block(*ds, block_num); | ||
uint64_t block_size = 0; | ||
|
||
auto ds = catalog.ro_stream_and_size_for_block(block_num, block_size); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not from your changes. I wish |
||
if (ds) { | ||
fc::datastream_mirror dsm(*ds, block_size); | ||
return read_block(dsm, block_num); | ||
} | ||
return {}; | ||
} | ||
|
||
|
@@ -1277,9 +1282,8 @@ namespace eosio { namespace chain { | |
} | ||
|
||
void block_log::append(const signed_block_ptr& b, const block_id_type& id) { | ||
std::vector<char> packed_block = fc::raw::pack(*b); | ||
std::lock_guard g(my->mtx); | ||
my->append(b, id, packed_block); | ||
my->append(b, id, b->packed_signed_block()); | ||
} | ||
|
||
void block_log::append(const signed_block_ptr& b, const block_id_type& id, const std::vector<char>& packed_block) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -231,7 +231,7 @@ struct assembled_block { | |
block_id_type id; | ||
pending_block_header_state_legacy pending_block_header_state; | ||
deque<transaction_metadata_ptr> trx_metas; | ||
mutable_signed_block_ptr unsigned_block; | ||
mutable_block_ptr unsigned_block; | ||
|
||
// if the unsigned_block pre-dates block-signing authorities this may be present. | ||
std::optional<producer_authority_schedule> new_producer_authority_cache; | ||
|
@@ -728,7 +728,7 @@ struct building_block { | |
} | ||
|
||
// in dpos, we create a signed_block here. In IF mode, we do it later (when we are ready to sign it) | ||
auto block_ptr = std::make_shared<signed_block>(bb.pending_block_header_state.make_block_header( | ||
auto block_ptr = signed_block::create_mutable_block(bb.pending_block_header_state.make_block_header( | ||
transaction_mroot, action_mroot, bb.new_pending_producer_schedule, std::move(new_finalizer_policy), | ||
vector<digest_type>(bb.new_protocol_feature_activations), pfs)); | ||
|
||
|
@@ -1567,20 +1567,8 @@ struct controller_impl { | |
return irreversible_mode() || bsp->is_valid(); | ||
}; | ||
|
||
using packed_block_future = std::future<std::vector<char>>; | ||
std::vector<packed_block_future> v; | ||
if (!irreversible_mode()) { | ||
v.reserve( branch.size() ); | ||
for( auto bitr = branch.rbegin(); bitr != branch.rend() && should_process(*bitr); ++bitr ) { | ||
v.emplace_back( post_async_task( thread_pool.get_executor(), [b=(*bitr)->block]() { return fc::raw::pack(*b); } ) ); | ||
} | ||
} | ||
auto it = v.begin(); | ||
|
||
for( auto bitr = branch.rbegin(); bitr != branch.rend() && should_process(*bitr); ++bitr ) { | ||
packed_block_future f; | ||
if (irreversible_mode()) { | ||
f = post_async_task( thread_pool.get_executor(), [b=(*bitr)->block]() { return fc::raw::pack(*b); } ); | ||
result = apply_irreversible_block(fork_db, *bitr); | ||
if (result != controller::apply_blocks_result::complete) | ||
break; | ||
|
@@ -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() ); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Now it is cleaner. |
||
|
||
db.commit( (*bitr)->block_num() ); | ||
root_id = (*bitr)->id(); | ||
|
@@ -1659,7 +1647,7 @@ struct controller_impl { | |
auto head = std::make_shared<block_state_legacy>(); | ||
static_cast<block_header_state_legacy&>(*head) = genheader; | ||
head->activated_protocol_features = std::make_shared<protocol_feature_activation_set>(); // no activated protocol features in genesis | ||
head->block = std::make_shared<signed_block>(genheader.header); | ||
head->block = signed_block::create_signed_block(signed_block::create_mutable_block(genheader.header)); | ||
chain_head = block_handle{head}; | ||
|
||
db.set_revision( chain_head.block_num() ); | ||
|
@@ -5531,7 +5519,7 @@ signed_block_ptr controller::fetch_block_by_number( uint32_t block_num )const { | |
|
||
std::vector<char> controller::fetch_serialized_block_by_number( uint32_t block_num)const { try { | ||
if (signed_block_ptr b = my->fork_db_fetch_block_on_best_branch_by_num(block_num)) { | ||
return fc::raw::pack(*b); | ||
return b->packed_signed_block(); | ||
} | ||
|
||
return my->blog.read_serialized_block_by_num(block_num); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -85,21 +85,28 @@ namespace eosio { namespace chain { | |
|
||
using block_extension = block_extension_types::block_extension_t; | ||
|
||
using signed_block_ptr = std::shared_ptr<const signed_block>; | ||
// mutable_block_ptr is built up until it is signed and converted to signed_block_ptr | ||
// mutable_block_ptr is not thread safe and should be moved into signed_block_ptr when complete | ||
using mutable_block_ptr = std::unique_ptr<signed_block>; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why not keep the original name There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
|
||
/** | ||
*/ | ||
struct signed_block : public signed_block_header{ | ||
private: | ||
signed_block( const signed_block& ) = default; | ||
explicit signed_block( const signed_block_header& h ):signed_block_header(h){} | ||
public: | ||
signed_block() = default; | ||
explicit signed_block( const signed_block_header& h ):signed_block_header(h){} | ||
signed_block( signed_block&& ) = default; | ||
signed_block& operator=(const signed_block&) = delete; | ||
signed_block& operator=(signed_block&&) = default; | ||
signed_block clone() const { return *this; } | ||
mutable_block_ptr clone() const { return std::unique_ptr<signed_block>(new signed_block(*this)); } | ||
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)}; } | ||
greg7mdp marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
deque<transaction_receipt> transactions; /// new or generated transactions | ||
extensions_type block_extensions; | ||
extensions_type block_extensions; | ||
|
||
flat_multimap<uint16_t, block_extension> validate_and_extract_extensions()const; | ||
std::optional<block_extension> extract_extension(uint16_t extension_id)const; | ||
|
@@ -108,9 +115,17 @@ namespace eosio { namespace chain { | |
return std::get<Ext>(*extract_extension(Ext::extension_id())); | ||
} | ||
bool contains_extension(uint16_t extension_id)const; | ||
|
||
const bytes& packed_signed_block() const { assert(!packed_block.empty()); return packed_block; } | ||
|
||
private: | ||
friend struct block_state; | ||
friend struct block_state_legacy; | ||
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 | ||
greg7mdp marked this conversation as resolved.
Show resolved
Hide resolved
|
||
}; | ||
using signed_block_ptr = std::shared_ptr<const signed_block>; | ||
using mutable_signed_block_ptr = std::shared_ptr<signed_block>; | ||
|
||
struct producer_confirmation { | ||
block_id_type block_id; | ||
|
@@ -129,3 +144,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) ) | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. Not sure I follow. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I meant |
||
namespace fc::raw { | ||
template <typename Stream> | ||
void unpack(Stream& s, eosio::chain::signed_block& v) { | ||
try { | ||
if constexpr (requires { s.extract_mirror(); }) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Don't quite get this code. Why the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Because if There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks. I got confused by the method name is |
||
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); | ||
greg7mdp marked this conversation as resolved.
Show resolved
Hide resolved
|
||
fc::reflector<eosio::chain::signed_block>::visit( fc::raw::detail::unpack_object_visitor<fc::datastream_mirror<Stream>, eosio::chain::signed_block>( v, ds ) ); | ||
v.packed_block = ds.extract_mirror(); | ||
} | ||
} FC_RETHROW_EXCEPTIONS(warn, "error unpacking signed_block") | ||
} | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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({}))); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. The idea is to make it difficult to misuse |
||
|
||
//in this case it's not really empty since the "genesis block" is present. These tests only | ||
// work because the default ctor of a block_header (used above) has previous 0'ed out which | ||
|
@@ -54,11 +54,12 @@ struct block_log_fixture { | |
std::vector<char> a; | ||
a.assign(size, fillchar); | ||
|
||
auto p = std::make_shared<eosio::chain::signed_block>(); | ||
auto p = eosio::chain::signed_block::create_mutable_block({}); | ||
p->previous._hash[0] = fc::endian_reverse_u32(index-1); | ||
p->header_extensions.push_back(std::make_pair<uint16_t, std::vector<char>>(0, std::vector<char>(a))); | ||
|
||
log->append(p, p->calculate_id(), fc::raw::pack(*p)); | ||
auto sp = eosio::chain::signed_block::create_signed_block(std::move(p)); | ||
log->append(sp, sp->calculate_id(), sp->packed_signed_block()); | ||
|
||
if(index + 1 > written_data.size()) | ||
written_data.resize(index + 1); | ||
|
Uh oh!
There was an error while loading. Please reload this page.