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

Improve block generation performance #1251

Merged
merged 5 commits into from Aug 29, 2018

Conversation

Projects
2 participants
@abitmore
Member

abitmore commented Aug 13, 2018

Improved block generating performance by caching public keys extracted from transaction signatures which reduces number of ECDSA verification calls.

Side effect: due to additional data copy, replay is slightly slower, profiling data says the difference is around 4%.Update: latest code avoided the performance penalty.

Perhaps todo:

  • extract signing keys for popped transactions in the first place. Update: it's done implicitly.

@abitmore abitmore added this to In progress in Feature release (201810) via automation Aug 13, 2018

@pmconrad

This comment has been minimized.

pmconrad commented Aug 19, 2018

IMO at this time 4% replay time hurts more than block generation performance.

@abitmore

This comment has been minimized.

Member

abitmore commented Aug 19, 2018

To improve replay performance, we need to change transactions field in block struct from vector<processed_transaction> to vector<processed_transaction_with_signees>, but this will change serialization, so will break both on-disk block database storage and p2p. A potential solution is to remove the added signees field from FC_REFLECT. Thoughts?

@abitmore abitmore changed the title from Improve block generating performance to Improve block generation performance Aug 19, 2018

@abitmore

This comment has been minimized.

Member

abitmore commented Aug 19, 2018

Update: it's even slower to replay after changed transactions in signed_block struct to processed_transaction_with_signees type. :-/ Code is here: abitmore@5b45f82

By the way, I tried to extract trx signees before pushing a block with this commit: abitmore@7309e85, don't know if it's worthwhile. With the patch, I assume that the performance will be better when there is frequent chain reorganization, but perhaps will have reduced performance under normal conditions due to additional data copy, although both affect validation nodes only (nodes that configured with a witness_id or started with --force-validate).

Thoughts?

@abitmore

This comment has been minimized.

Member

abitmore commented Aug 19, 2018

IMO at this time 4% replay time hurts more than block generation performance.

Yes, at this time the chain is not under any meaningful pressure.

In the long run, faster block generation means higher throughput, aka higher TPS. This PR is the first step towards multi-threaded signature verification. If we're going to do another stress testing, I think we need something like this.

@abitmore

This comment has been minimized.

Member

abitmore commented Aug 21, 2018

I guess the performance loss is due to additional object construction when applying transactions. I guess performance can be restored by moving the signees field into signed_transaction struct directly. Need to revert quite some code, so I'll rebase.

@abitmore

This comment has been minimized.

Member

abitmore commented Aug 22, 2018

Rebased. The code is much cleaner now, and replay performance is fine. @pmconrad please review.

@pmconrad

Good first steps.

@@ -162,7 +162,8 @@ namespace graphene { namespace app {
void network_broadcast_api::broadcast_transaction(const signed_transaction& trx)
{
trx.validate();
_app.chain_database()->push_transaction(trx);
const auto& chain_db = _app.chain_database();

This comment has been minimized.

@pmconrad

pmconrad Aug 23, 2018

Remove the '&', it's useless here (chain_database() returns a shared_ptr, not a database reference.
OR dereference the shared_ptr right here, might even be a bit more efficient.

@@ -188,7 +189,8 @@ namespace graphene { namespace app {
{
trx.validate();
_callbacks[trx.id()] = cb;
_app.chain_database()->push_transaction(trx);
const auto& chain_db = _app.chain_database();

This comment has been minimized.

@pmconrad
{
signees = _get_signature_keys( chain_id );
}

This comment has been minimized.

@pmconrad

pmconrad Aug 23, 2018

Please add move constructor and move assignment operator, for moving signees and signatures efficiently.

This comment has been minimized.

@abitmore

abitmore Aug 23, 2018

Member

Won't move constructor and move assignment operator be implicitly added by compiler? I think this constructor is not a copy constructor, so won't prevent compilers from doing so.

This comment has been minimized.

@pmconrad

pmconrad Aug 27, 2018

Oops, you're right.

@@ -303,6 +303,15 @@ void verify_authority( const vector<operation>& ops, const flat_set<public_key_t
flat_set<public_key_type> signed_transaction::get_signature_keys( const chain_id_type& chain_id )const

This comment has been minimized.

@pmconrad

pmconrad Aug 23, 2018

Can change method signature to return a const reference to the set, to avoid useless copying.

// However, we don't pass in another chain ID so far, for better performance, we skip the check.
if( !signees.empty() || signatures.empty() )
return signees;
return _get_signature_keys( chain_id );

This comment has been minimized.

@pmconrad

pmconrad Aug 23, 2018

I would swap the logic:

if( signees.empty() ) signees = _get_signature_keys;
return signees;

This comment has been minimized.

@abitmore

abitmore Aug 23, 2018

Member

I'd like to do that as well, but it breaks the const modifier, need to update quite some other code to adapt the change. Not returning a reference is due to the same reason.

This comment has been minimized.

@pmconrad

This comment has been minimized.

@abitmore

abitmore Aug 27, 2018

Member

Thanks for the link. I was thinking about using a shared_ptr or similar for the signees field. Looks like it's easier to use the mutable keyword?

This comment has been minimized.

@pmconrad

pmconrad Aug 27, 2018

Yes (although slightly ugly).

This comment has been minimized.

@abitmore

abitmore Aug 27, 2018

Member

Update: after updated it to return a reference, a lot of test cases are broken due to reuse of signed_transaction objects. For example, construct a transaction, sign it, verify signatures, then clear the signatures, sign it with another key, verify signatures again -- this will fail because signees field is not refreshed. I think the main routine doesn't reuse signed_transaction objects so I'll fix the test cases.

abitmore added some commits Aug 23, 2018

Updated transaction::signees to mutable
and
* updated get_signature_keys() to return a const reference,
* get_signature_keys() will update signees on first call,
* modified test cases and wallet.cpp accordingly,
* no longer construct a new signed_transaction object before pushing
No longer extract public keys before pushing a trx
and removed unused new added constructor and _get_signature_keys() function from signed_transaction struct
@abitmore

This comment has been minimized.

Member

abitmore commented Aug 27, 2018

Updated signees to mutable, in the meanwhile reverted a lot of code (again).

result.insert( fc::ecc::public_key(sig,d) ).second,
tx_duplicate_sig,
"Duplicate Signature detected" );
auto d = sig_digest( chain_id );

This comment has been minimized.

@abitmore

abitmore Aug 27, 2018

Member

Should we add a static modifier here, or cache it somewhere E.G. globally? It looks inefficient to me.

This comment has been minimized.

@pmconrad

pmconrad Aug 28, 2018

sig_digest computes hash( chain_id || *this ), so we can't really cache it. Also, it should only be computed once when validating signatures, so I think it isn't worth it either.

This comment has been minimized.

@abitmore

abitmore Aug 29, 2018

Member

Oh, I didn't notice *this. Thanks.

Feature release (201810) automation moved this from In progress to In Testing Aug 28, 2018

@abitmore abitmore merged commit 0dbf181 into develop Aug 29, 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 (201810) automation moved this from In Testing to Done Aug 29, 2018

@abitmore abitmore deleted the trx-signees branch Aug 29, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment