-
Notifications
You must be signed in to change notification settings - Fork 120
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
add threading to add block #1637
base: dev
Are you sure you want to change the base?
Conversation
@@ -185,11 +182,24 @@ namespace wallet | |||
return; | |||
} | |||
|
|||
std::vector<std::thread> threads; | |||
|
|||
auto db_tx = db->db_transaction(); | |||
for (const auto& block : blocks) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if we could std::move
the block in this foreach into that lambda, we can capture [this, block=std::move(block)]
we can avoid a capture all by reference (which is something i always try to avoid using). that or capture with [this, &block]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also the insides of add_block
sound possibly thread unsafe.
do we mark all functions that are pure with const
or no?
@@ -34,6 +34,7 @@ namespace wallet | |||
|
|||
// A derivation is simply the private view key multiplied by the tx public key | |||
// do this for every tx public key in the transaction | |||
//TODO SEAN THIS IS A BOTTLENECK |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thinking here: what if we used a threadpool kind of entity where we put heavy but pure jobs onto. then have the these heavy functions (if they can be pure) return a std::future
that will be fulfilled when the operation is done in the thread pool. we could have those fan out onto as many threads as we want.
the following idea could look something looking like:
auto ftr_derivations = wallet_keys->async_generate_key_derivation(tx_public_keys);
auto derivation = ftr_derivations.get();
That was basically the original plan, at least conceptually, so given that you independently considered it as well, I'd say it's not a bad one. |
This PR is to open the discussion on adding multithreading to wallet3 to improve performance. After putting wallet3 through perf it highlighted 2 functions where wallet3 spent most of its time to scan the chain.
output_and_derivation_ours()
andgenerate_key_derivations()
Making some rough changes to wallet3 and adding threads around add_block resulted in a significant increase in block processing speed. Increasing wallet3 from around 2.5k blocks/second up to 13k blocks/second.
Before:
After:
However processing blocks out of order in this form isn't viable with the wallet outputting incorrect final balances after it finished scanning (in fact every time it resulted in a different balance). I suspect this is due to it ignoring outputs spent because the input hadnt been received yet
I do note that the two bottleneck functions are both in transaction scanning for outputs received and not outputs spent, so we possibly could split the scanning into first stage multithreaded scan of the outputs received, followed by a linear scan of outputs spent.
This is still pretty early days for wallet3 so adding multithreading might not be appropriate yet. But is good to know what the bottlenecks are atm