-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
feat(cli): count the appearances of opcode and opcode sets #5211
Conversation
Co-authored-by: Alessandro Mazza <[email protected]>
Co-authored-by: Alessandro Mazza <[email protected]>
Co-authored-by: Alessandro Mazza <[email protected]>
Co-authored-by: Alessandro Mazza <[email protected]>
bin/reth/src/count_opcodes/mod.rs
Outdated
test_bytecode | ||
} | ||
|
||
fn all_opcodes_test_string() -> String { |
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.
This can be quite noisy in the file, however we wanted to make it as easy as possible to read and modify this test opcodes string if needed
bin/reth/src/cli/mod.rs
Outdated
@@ -163,6 +164,9 @@ pub enum Commands<Ext: RethCliExt = ()> { | |||
/// Scripts for node recovery | |||
#[command(name = "recover")] | |||
Recover(recover::Command), | |||
/// Script for counting opcodes occurrencies | |||
#[command(name = "count-opcodes")] | |||
CountOpcodes(count_opcodes::Command), |
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'd put this under debug or something like that since it's not really relevant to normal node ops
bin/reth/src/count_opcodes/mod.rs
Outdated
|
||
let mut opcode_counter = OpCodeCounter::new(); | ||
|
||
info!("start opcodes processing..."); |
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.
These info!'s are not rly necessary, I imagine it doesn't take a long time?
bin/reth/src/count_opcodes/mod.rs
Outdated
} | ||
|
||
fn print_counts(&self, size: usize) { | ||
println!("Single opcodes:"); |
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: we can use the table layout that we also use for the db commands here 😄
Hey @onbjerg, we should have addressed all your reviews. Now tables are printed using |
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
3a8cdd9
to
46dc762
Compare
Hi @DaniPopes! Thanks for the review, now it should be better. |
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
b97ce1c
to
890c1af
Compare
Hey, sorry to bother you. Is there anything left we may do for this PR? |
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: Can you extract this to an example/
of using the node as a library, with the OpcodeCounter
in a separate file and some explanation on the motivation of why opcode occurences are useful? cc @sslivkoff for parquet
something similar is available in the geth trigram / bigram / unigram tracer with would love to have this new implementation accessible over rpc somehow |
Thanks! We'll look into it |
I moved it into the |
Closes #5067. This PR co-authored by @alessandromazza98 introduces a new command called
count-opcodes
that returns thetake
top occurrences of opcodes and tuples of opcodes.Here below is an example of the result of running this command on a few blocks on Sepolia: