Skip to content
This repository has been archived by the owner on Aug 2, 2022. It is now read-only.

Improve signature recovery #7721

Merged
merged 29 commits into from
Aug 13, 2019
Merged

Improve signature recovery #7721

merged 29 commits into from
Aug 13, 2019

Conversation

heifner
Copy link
Contributor

@heifner heifner commented Aug 2, 2019

Change Description

  • Improve the performance of signature recovery by reverting back to 1.8.x method of thread_pool usage. The develop branch approach of passing a next function does not perform as well as just calling wait() on the future.
  • Transaction id is now cached on packed_transaction instead of transaction_metadata.
  • Changed the interface for accept_transaction to take a packed_transaction instead of transaction_metadata.
  • transaction_metadata is now constructed with the recovered public keys and signature cpu usage.
  • transaction_metadata is now constructed with implicit and scheduled const values set.
  • transaction_metadata is now immutable except for the accepted flag.
  • transaction_metadata construction is now restricted to static factory methods start_recover_keys and create_no_recover_keys.
  • Use cached transaction_metadata in apply_block if a previously applied block with cached transaction_metadata.

Consensus Changes

  • Consensus Changes

API Changes

  • [ x ] API Changes (plugin interface)
  • eosio::chain::plugin_interface::incoming::channels::transaction changed from transaction_metadata_ptr to packed_transaction_ptr
  • eosio::chain::plugin_interface::incoming::methods::transaction_async changed from void(const transaction_metadata_ptr&, bool, next_function<transaction_trace_ptr>), first_provider_policy> to void(const packed_transaction_ptr&, bool, next_function<transaction_trace_ptr>), first_provider_policy>

Documentation Additions

  • Documentation Additions

} CATCH_AND_CALL(next);
} );
}
});
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the key performance change. As odd as this looks, it is demonstratively better than before. And it is not the switch back to producer thread pool. Either thread pool (chain or producer) showed same results. The difference is in passing in a next std::function over calling future.wait().

@heifner heifner requested a review from arhag August 2, 2019 18:41
auto after_sig_recovery =
[self = this, trx, persist_until_expired, next{std::move(next)}]() mutable {
app().post(priority::low, [self, trx{std::move(trx)}, persist_until_expired, next{std::move(next)}]() {
self->process_incoming_transaction_async( trx, persist_until_expired, next );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know whether this matters, but next is being copied here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes it was. I tested a version where the lambda was mutable and it std::move but didn't make any difference.

b734e56

libraries/chain/fork_database.cpp Outdated Show resolved Hide resolved
plugins/chain_plugin/chain_plugin.cpp Show resolved Hide resolved
plugins/net_plugin/net_plugin.cpp Show resolved Hide resolved
unittests/misc_tests.cpp Outdated Show resolved Hide resolved
@heifner
Copy link
Contributor Author

heifner commented Aug 7, 2019

@arhag ready for review.

libraries/chain/controller.cpp Outdated Show resolved Hide resolved
libraries/chain/controller.cpp Show resolved Hide resolved
libraries/chain/controller.cpp Outdated Show resolved Hide resolved
libraries/chain/controller.cpp Outdated Show resolved Hide resolved
@heifner heifner merged commit d30392c into develop Aug 13, 2019
@heifner heifner deleted the dev-perf branch August 13, 2019 19:29
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants