-
Notifications
You must be signed in to change notification settings - Fork 4.5k
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 use_durable_nonce option #27151
add use_durable_nonce option #27151
Conversation
4feeb4a
to
dba5f83
Compare
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.
Looks good! I mainly had a few questions for my own info
19de07a
to
d4e9d85
Compare
@joncinque I've updated PR with |
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.
Just some small nits, then this is good to go!
bench-tps/src/bench.rs
Outdated
let blockhashes: Vec<Hash> = pubkeys | ||
.chunks(MAX_MULTIPLE_ACCOUNTS) | ||
.map(|pubkeys| get_nonce_blockhashes(&client, &pubkeys)) | ||
.flatten() | ||
.collect(); |
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.
nit: Instead of chunking here, it might be faster to pass all of them to get_nonce_blockhashes
and then do the chunking in the function, that way you can always fetch MAX_MULTIPLE_ACCOUNTS
. What do you think?
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.
I've updated the code and tested it with private cluster.
I've added UPD section to the PR description with some plots from this run.
It looks like tps is spiky because we request all the blockhashes and later execute all the transactions. Spike width is ~4s while the gap is ~2s => it is possible to increase tps at most by 30% by overlapping these two phases.
I created an issue for this as a follow up since it requires major code reorganization: #27416
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.
Awesome, and yes it makes sense to do that work separately
d4e9d85
to
f03417e
Compare
f03417e
to
d8a2480
Compare
d8a2480
to
bab6632
Compare
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.
Great job!
thanks! it would be not possible without your comments |
Problem
Final PR in the chain of PRs adding durable nonce transactions to bench-tps.
Requires #27176Requires #27379
Summary of Changes
--use-durable-nonce
option to cliUPD:
TPS is now ~5k
Some plots with running with this option:
The bottom plot is for data shreds: at the beggining there are many data shreds because we create nonce durable accounts. During transactions execution this metric is lower.
If we zoom into the transaction phase, we observe that the tps is spiky:
It happens because it takes time to request blockhashes before executing transactions