-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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 crypto & blockchain to fuel-benches #630
Conversation
Is it possible to reduce the amount of files/lines in this PR? I understand many are auto-generated reports, but adding some items to a |
run_group( | ||
&mut group, | ||
"or", | ||
VmBench::new(Opcode::OR(0x10, 0x11, 0x12)).with_prepare_script(vec![ |
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 see this script popping up a lot, could these maybe be moved to a single constant or single expression in case it is ever updated?
let cases = vec![ | ||
1, 10, 100, 1_000, 10_000, | ||
100_000, | ||
// 1_000_000 - I/O error in BAL; investigate |
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.
Should this be added to the issue tracker for fuel-core
?
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'm investigating that one to understand if its a bug in the benchmark implementation or something more. The early conclusion is for the latter, but I'm not sure yet
"k256", | ||
VmBench::new(Opcode::K256(0x10, 0x00, 0x11)) | ||
.with_prepare_script(vec![ | ||
Opcode::MOVI(0x10, Bytes32::LEN as Immediate18), |
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.
Same issue on moving this script to a constant or single expression if possible
format!("mcl ({})", i), | ||
VmBench::new(Opcode::MCL(0x10, 0x11)).with_prepare_script(vec![ | ||
Opcode::MOVI(0x11, *i), | ||
Opcode::ALOC(0x11), |
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.
Could be nice to have comments explaining why each script was chosen for each benchmark for those consulting benchmarks and to keep every up to speed with logic used for benchmarking
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.
Cool, will add some docs and simplify the code a bit
0c46378
to
a0007c3
Compare
@ControlCplusControlV removed the json files. Still have a lot of files, but half the original amount |
a0007c3
to
6a27ee3
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.
Hmm - this is quite a bit more generated files than I expected 🤔
It would be better to figure out a solution using the new gh page automation actions to upload these from ci rather than managing all of this in our git history. This seems like way too much to check in.
6a27ee3
to
5b7d6f6
Compare
The problem is that When the database grows large, some path references are reconstructed so the original tempdir reference path is lost - hence, it gets dropped so the database can't find the dir in the disk because it is entirely removed. We clone every instance of the prepared case before we run the benchmarks so one instance will not affect the other. Instead of making a super hacky way for the reference to outlive this database reorg, the best solution is to clone properly the prepared case by copying all internal data. Added issue to fix that: |
I got the balance benchmarks working by simply using |
* using arc'd tmpfile to fix 1M test case
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.
approving (assuming we can automerge from master once the rust 1.64 upgrade goes in)
No description provided.