Skip to content
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

Parallel preprocessing of blocks + transactions #1360

Merged
merged 18 commits into from Dec 14, 2018

Conversation

@pmconrad
Copy link
Contributor

commented Oct 5, 2018

Work in progress, do not merge yet!

Will some day resolve #1079 insofar as it is possible without modifying consensus.

This includes the changes from #1359 . Requires bitshares/bitshares-fc#78 .

Status: seems to be working, at least with boost-1.60. Testing.

@clockworkgr

This comment has been minimized.

Copy link
Member

commented Oct 5, 2018

good stuff peter!

@pmconrad pmconrad referenced this pull request Oct 5, 2018

Merged

Performance opt pt 1 #1359

@pmconrad

This comment has been minimized.

Copy link
Contributor Author

commented Oct 5, 2018

Thanks, but not yet. :-/

@clockworkgr

This comment has been minimized.

Copy link
Member

commented Oct 5, 2018

ah!..got overexcited over the "working version" commit :)

@abitmore
Copy link
Member

left a comment

Since it's still WIP, perhaps will have answer later.

@pmconrad pmconrad force-pushed the performance_opt branch 2 times, most recently to a3a5612 Oct 10, 2018

graphene::chain::database::skip_tapos_check |
graphene::chain::database::skip_witness_schedule_check;
if( _options->count("revalidate-blockchain") ) // see also handle_block()
skip = _options->count("force-validate") ? 0 : graphene::chain::database::skip_transaction_signatures;

This comment has been minimized.

Copy link
@abitmore

abitmore Oct 12, 2018

Member
  1. I think it's better to replace 0 with graphene::chain::database::skip_nothing.
  2. Code readability here looks a bit poor. Now it reads:
declare skip = something;
if (condition A)
{
   skip = condition B ? something else : yet something else;
}

How about change the if to ?:

declare skip = condition A ? condition B ? something else : yet something else : something;

or change the ?: to if else?

This comment has been minimized.

Copy link
@pmconrad

pmconrad Nov 7, 2018

Author Contributor

done

Show resolved Hide resolved libraries/chain/db_block.cpp
@abitmore
Copy link
Member

left a comment

And need to resolve FC conflicts.

@@ -961,9 +979,10 @@ void application::set_program_options(boost::program_options::options_descriptio
"Path to create a Genesis State at. If a well-formed JSON file exists at the path, it will be parsed and any "
"missing fields in a Genesis State will be added, and any unknown fields will be removed. If no file or an "
"invalid file is found, it will be replaced with an example Genesis State.")
("replay-blockchain", "Rebuild object graph by replaying all blocks")
("replay-blockchain", "Rebuild object graph by replaying all blocks without validatino")

This comment has been minimized.

Copy link
@abitmore

This comment has been minimized.

Copy link
@pmconrad

pmconrad Nov 7, 2018

Author Contributor

fixed

uint32_t chunks = fc::asio::default_io_service_scope::get_num_threads();
uint32_t chunk_size = block.transactions.size() / chunks;
if( chunks * chunk_size < block.transactions.size() )
chunk_size++;

This comment has been minimized.

Copy link
@abitmore

abitmore Oct 21, 2018

Member

Is it essentially rounding up? If it's true then can code like this:

uint32_t chunk_size = ( block.transactions.size() + chunks - 1 ) / chunks;

This comment has been minimized.

Copy link
@pmconrad

pmconrad Nov 7, 2018

Author Contributor

fixed

void validate() const;
digest_type digest()const;
virtual const transaction_id_type& id()const;
virtual void validate() const;
/// Calculate the digest used for signature validation

This comment has been minimized.

Copy link
@abitmore

abitmore Oct 21, 2018

Member

This comment should be moved to correct place.

This comment has been minimized.

Copy link
@pmconrad

pmconrad Nov 7, 2018

Author Contributor

done

@@ -307,20 +305,17 @@ const flat_set<public_key_type>& signed_transaction::get_signature_keys( const c
{ try {
// Strictly we should check whether the given chain ID is same as the one used to initialize the `signees` field.
// However, we don't pass in another chain ID so far, for better performance, we skip the check.

This comment has been minimized.

Copy link
@abitmore

abitmore Oct 21, 2018

Member

The comments should be removed if signees are calculated every time.

This comment has been minimized.

Copy link
@pmconrad

pmconrad Nov 7, 2018

Author Contributor

moved

@@ -307,20 +305,17 @@ const flat_set<public_key_type>& signed_transaction::get_signature_keys( const c
{ try {
// Strictly we should check whether the given chain ID is same as the one used to initialize the `signees` field.
// However, we don't pass in another chain ID so far, for better performance, we skip the check.
if( signees.empty() && !signatures.empty() )
auto d = sig_digest( chain_id );
flat_set<public_key_type> result;

This comment has been minimized.

Copy link
@abitmore

abitmore Oct 21, 2018

Member

Use a non-default key_compare here?

This comment has been minimized.

Copy link
@pmconrad

pmconrad Nov 7, 2018

Author Contributor

That wouldn't help, because then the set has to be re-sorted when assigning to _signees field.
Unless we also change the sorting order there, which means the signature of this method has to be changed as well, and all places that use it... it simply doesn't pay out, see #1401 .

precomputable_transaction( const signed_transaction& tx ) : signed_transaction(tx) {};
precomputable_transaction( signed_transaction&& tx ) : signed_transaction(tx) {};

virtual const transaction_id_type& id()const;

This comment has been minimized.

Copy link
@abitmore

abitmore Oct 21, 2018

Member

Can add an override keyword to overridden functions here.

This comment has been minimized.

Copy link
@pmconrad

pmconrad Nov 7, 2018

Author Contributor

done

_index[space][type]->save( _data_dir / "object_database.tmp" / fc::to_string(space)/fc::to_string(type) );
tasks.push_back( fc::do_parallel( [this,space,type] () {
_index[space][type]->save( _data_dir / "object_database.tmp" / fc::to_string(space)/fc::to_string(type) );
} ) );

This comment has been minimized.

Copy link
@abitmore

abitmore Oct 21, 2018

Member
  1. Does this really increase performance? Isn't IO the bottleneck here?
  2. Perhaps better if we adopt the behavior of ChainBase, store not only data but also indexes, to improve startup speed (may slow down shutdown speed though). This belongs to another ticket.

This comment has been minimized.

Copy link
@pmconrad

pmconrad Nov 7, 2018

Author Contributor

IO isn't necessarily the bottleneck, e. g. when restarting a node the previously saved data might still be in system buffers.

This comment has been minimized.

Copy link
@abitmore

abitmore Nov 7, 2018

Member

Here is save aka writing but not reading. When we're writing, usually the data has changed, even if it didn't change much, since we're writing to temporary (aka different) files, so the buffer doesn't help.

This comment has been minimized.

Copy link
@pmconrad

pmconrad Nov 8, 2018

Author Contributor

Of course disk buffer helps when writing.
On a system with a slow disk and the bare minimum of RAM you're right, parallelizing won't help - but it won't hurt either.
With sufficient buffer memory, or on a RAM-disk, parallel saving should be faster.

{
wlog( "Reindexing terminated due to gap: Block ${i} does not exist!", ("i", i) );
uint32_t dropped_count = 0;
while( true )

This comment has been minimized.

Copy link
@abitmore

abitmore Oct 21, 2018

Member

I still believe this (old) code is inefficient. But it may belong to another ticket with lower priority.

This comment has been minimized.

Copy link
@pmconrad

pmconrad Nov 7, 2018

Author Contributor

Agree.

@@ -105,6 +105,7 @@ void delayed_node_plugin::sync_with_trusted_node()
fc::optional<graphene::chain::signed_block> block = my->database_api->get_block( db.head_block_num()+1 );
FC_ASSERT(block, "Trusted node claims it has blocks it doesn't actually have.");
ilog("Pushing block #${n}", ("n", block->block_num()));
db.precompute_parallel( *block, graphene::chain::database::skip_nothing ).wait();

This comment has been minimized.

Copy link
@abitmore

abitmore Oct 21, 2018

Member

Can adopt same approach used in reindex here for even better performance, IMHO low priority though. Add a TODO?

This comment has been minimized.

Copy link
@pmconrad

pmconrad Nov 7, 2018

Author Contributor

added

@@ -170,6 +170,11 @@ extern uint32_t GRAPHENE_TESTING_GENESIS_TIMESTAMP;

namespace graphene { namespace chain {

class clearable_block : public signed_block {

This comment has been minimized.

Copy link
@abitmore

abitmore Oct 21, 2018

Member

Please write some comments for this class.

This comment has been minimized.

Copy link
@pmconrad

pmconrad Nov 7, 2018

Author Contributor

done

@pmconrad

This comment has been minimized.

Copy link
Contributor Author

commented Nov 7, 2018

Rebased. Will bump fc after bitshares/bitshares-fc#78 has been merged.

@pmconrad

This comment has been minimized.

Copy link
Contributor Author

commented Nov 7, 2018

Timings taken up to block 31,000,000 with various core versions (also see #1371), on a quad-core (with hyperthreading) "Intel(R) Core(TM) i7-6700 CPU @ 3.40GHz", 64G RAM, default config:

Version 2.0.180823 develop as of 2018-10-24 #1401 this pr
Replay 2:00:21 1:58:15 1:59:23 2:00:36
Resync 3:35:51 3:46:15 3:44:55 3:24:07
Resync (full validation) 7:15:06 7:21:59 7:18:52 3:48:52
Revalidate n/a n/a 2:51:08 2:46:05
Revalidate (full validation) n/a n/a 6:13:40 3:45:56

(Resync was done at different times of day, fluctuations due to network conditions are to be expected.)

@jmjatlanta
Copy link
Contributor

left a comment

Code looks good. I believe fc is all set. Please fix the conflict and I will approve this after some final testing. Nice work!

@pmconrad pmconrad force-pushed the performance_opt branch from 81f7520 to 2c01109 Dec 7, 2018

@pmconrad

This comment has been minimized.

Copy link
Contributor Author

commented Dec 7, 2018

Rebased on latest develop + bumped fc to latest master

Feature Release (201902) automation moved this from In progress to Reviewer approved Dec 7, 2018

@jmjatlanta
Copy link
Contributor

left a comment

Code looks good. Tested with Ubuntu 18.10 / Boost 1.67 / OpenSSL 1.1.0. chain_test runs with no errors detected. Thank you for tackling this.

@pmconrad pmconrad merged commit 1471c05 into develop Dec 14, 2018

3 checks passed

ci/dockercloud Your tests passed in Docker Cloud
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

Feature Release (201902) automation moved this from Reviewer approved to Done Dec 14, 2018

@pmconrad pmconrad deleted the performance_opt branch Dec 14, 2018

@pmconrad pmconrad referenced this pull request Jan 2, 2019

Open

Make better use of multi-core servers #1079

2 of 17 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.