-
Notifications
You must be signed in to change notification settings - Fork 11.3k
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
Adding a benchmark for transaction signing #14174
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
3 Ignored Deployments
|
69fab0f
to
365fdd2
Compare
.get_validator() | ||
.handle_transaction( | ||
&self.epoch_store, | ||
VerifiedTransaction::new_unchecked(transaction), |
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.
Not 100% sure this is the right way to make a transaction verified in the benchmark.
Please wait for @lxfind to take a look on code organizations. |
.get_validator() | ||
.handle_transaction( | ||
&self.epoch_store, | ||
VerifiedTransaction::new_unchecked(transaction), |
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 think that verification is an important part of tx signing flow, which we probably want to include in the benchmarking. You may want to expose the handle_transaction function in ValidatorService instead. (you can call it by passing a Tonic request)
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.
Done. PTAL. I tried to use the Transaction function, which follows Component::ValidatorService
code path.
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.
Comments are addressed. PTAL!
f66bdc3
to
1f6d845
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.
LGTM. Thanks!
1f6d845
to
c597dcb
Compare
.into_iter() | ||
.map(|tx| { | ||
let validator = self.validator(); | ||
tokio::spawn(async move { validator.sign_transaction(tx).await }) |
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.
there is no need to spawn a task here, since you do tasks.collect().await
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.
Good catch. That's on me (I wrote it that way in all other places by copy paste).
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.
Actually Zhe points out that this may not run in parallel, so we may want to leave the spawn here.
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.
At least for tx signing benchmark, removing the spawn actually makes it slightly faster. It could be because signing is too cheap that parallelism doesn't help
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.
Yeah I thought about the spawn()
pattern for the initial PRs. Maybe only for executions using a separate task makes sense.
Description
Adding a benchmark to measure validator signing transaction performace in the single node benchmark suite.
Sample output:
Test Plan
How did you test the new or updated feature?
If your changes are not user-facing and not a breaking change, you can skip the following section. Otherwise, please indicate what changed, and then add to the Release Notes section as highlighted during the release process.
Type of Change (Check all that apply)
Release notes