-
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
Introduce --dummy-sig to solana-bench-tps #6575
Conversation
8bc9bd5
to
91d08f7
Compare
@@ -306,7 +309,11 @@ fn generate_system_txs( | |||
.par_iter() | |||
.map(|(from, to)| { | |||
( | |||
system_transaction::transfer(from, &to.pubkey(), 1, *blockhash), | |||
if !dummy_sig { |
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.
- TODO: I'm pretty sure there is still a few places to be branched like this in the
solana-bench-tps
. Once sanctioned, I'm work on it to finish this.
@@ -28,6 +28,12 @@ impl Signature { | |||
Self(GenericArray::clone_from_slice(&signature_slice)) | |||
} | |||
|
|||
pub fn new_rand() -> Self { | |||
let mut random_signature_bytes = [0u8; 64]; |
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.
- Magic number!
pubkey::Pubkey, | ||
signature::{Keypair, KeypairUtil}, | ||
system_instruction, | ||
transaction::Transaction, | ||
}; | ||
|
||
/// Create and sign new SystemInstruction::CreateAccount 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.
Removed this because this comment is too obvious.
@@ -96,6 +97,23 @@ impl Transaction { | |||
Self::new_unsigned(message) | |||
} | |||
|
|||
pub fn new_signed_instructions_random( |
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.
- Add test for this.
space: u64, | ||
program_id: &Pubkey, | ||
) -> Transaction { | ||
Transaction::new_signed_instructions_random( |
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.
API naming: At this layer, we translate to more higher and use-oriented abstract concept from random (implementation details) to dummy (users just want non-verifable fake signatures cheaply).
Codecov Report
@@ Coverage Diff @@
## master #6575 +/- ##
========================================
- Coverage 81.8% 81.8% -0.1%
========================================
Files 244 241 -3
Lines 51902 51917 +15
========================================
- Hits 42504 42470 -34
- Misses 9398 9447 +49 |
@mvines I elaborated the description a bit! Could you take a look at it in spare time? |
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. |
91d08f7
to
ad07f2a
Compare
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. |
ad07f2a
to
736e955
Compare
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. |
I'm planning to play with this at this weekend! |
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. |
Finally, I've flushed all other extra findings via the cource of #7736, this PR is needed to really reproduce the bank hash mismatch error. |
6d38abf
to
9d390cf
Compare
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. |
This stale pull request has been automatically closed. Thank you for your contributions. |
Problem
When profiling on the cryptography-unrelated system logic code path for
solana-validator
to the extreme,solana-bench-tps
can be bottle-necked.Because
solana-validator
does a heavy-lifting of ed25519 verification by default,perf top
is littered with entries related to it. So I first filtered them out by--dev-no-sigverify
to concentrate on the transaction processing code for concurrency and scalability in search of potential bottlenecks in there. Then, in turn,solana-bench-tps
's ed25519 signing got the next bottleneck!I need to disable it too to stress
solana-validator
enough to really expose throughput issues in TPU/TVU. Ultimately, when most of crypto workloads are offloaded to the GPU, the bottlenecks in TPU/TVU will be the next priority.Solution
Introduce
--dummy-sig
flag tosolana-bench-tps
, which is paired withsolana-validator
's--dev-no-sigverify
.This is still a draft, not final, rather collecting feedback from you! I think this option might be useful, or is there other better way to profile for the mentioned profiling scenario?
Background
I'm trying to profile
solana-validator
with Linux'sperf
, already finding some small wins (stay tuned for more PRs!), by the middle course of familiarizing myself with the code base from another viewpoint.