add fibonacchi bench #690
add fibonacchi bench #690
Conversation
ceno_zkvm/benches/fibonacci.rs
Outdated
| use mpcs::BasefoldDefault; | ||
|
|
||
| criterion_group! { | ||
| name = op_add; |
There was a problem hiding this comment.
nice catch and fixed 19b6164
| ..CENO_PLATFORM | ||
| }; | ||
|
|
||
| for max_steps in [1usize << 20, 1usize << 21, 1usize << 22] { |
There was a problem hiding this comment.
Do you think it makes sense to have a smaller number like 1 << 10 as well? It works much faster and it can be a good sanity check.
By the way, do you know a simple way to not run for all three sizes every time? Or even to pass size from the command-line?
There was a problem hiding this comment.
For "1 << 20" per iteration it take ~ 20 secs to warm up (on ceno server), so for sanity check purpose it's also fine without wait too long time. (most of the time are on key setup actually, 1 << 20 proof time is < 5 secs, thus smaller size not help much though)
For run with custom start/end, I tried to have Args + pass from CLI, but unfortunately the argument parsing will conflicts with Criteria crate, both complain unexpected argument, atm I dont have idea, will try look back to it later :)
| run_e2e_gen_witness::<E, Pcs>( | ||
| program, | ||
| platform, | ||
| args.stack_size, | ||
| args.heap_size, | ||
| hints, | ||
| max_steps, | ||
| ); |
There was a problem hiding this comment.
I see you've moved the high-level logic back to bin/. Is this temporary or do you want it to stay this way?
There was a problem hiding this comment.
Most of the logic stay in src/e2e.rs. bin/e2e.rs will call sub step function.
To clarify, the parts I move to higher level are
- timing statistics print out, since this information are across different sub function
- sanity check for verifier with malicious public input, it's not very general to stay in src/e2e.rs (unless there is a flag to control on/off). Besides, to make generics type E, PCS work with
panic::catch_unwindwe need to add quite of trait bound. Above 2 reason makes the decision to stay in high level e2e :)
|
|
||
| run_e2e_verify(&verifier, zkvm_proof.clone(), exit_code, max_steps); | ||
|
|
||
| // do sanity check |
There was a problem hiding this comment.
Below are sanity check logic, which might not be general in other use case :)
| fn bench_e2e(c: &mut Criterion) { | ||
| type Pcs = BasefoldDefault<E>; | ||
| let mut file_path = PathBuf::from(env!("CARGO_MANIFEST_DIR")); | ||
| file_path.push("examples/fibonacci.elf"); |
There was a problem hiding this comment.
Please see how test_elf.rs builds and reads in the examples.
See also examples-builder.
There was a problem hiding this comment.
Build and read elf is outside of the scope. there are other ongoing task for build fibonacci elf.
| let e2e_time = e2e_start.elapsed().as_secs_f64(); | ||
| let witgen_time = e2e_time - proving_time; | ||
| println!( | ||
| "Proving finished.\n\ |
There was a problem hiding this comment.
You can just use 'natural' newlines?
| "Proving finished.\n\ | |
| "Proving finished. |
There was a problem hiding this comment.
This is from PR #652 .
Current PR just move the code to different place. I guess it's there for some reason
ceno_zkvm/src/bin/e2e.rs
Outdated
| // do sanity check | ||
| let transcript = Transcript::new(b"riscv"); | ||
| // change public input maliciously should cause verifier to reject proof | ||
| zkvm_proof.raw_pi[0] = vec![<E as ff_ext::ExtensionField>::BaseField::ONE]; |
There was a problem hiding this comment.
That should be easier to read:
| zkvm_proof.raw_pi[0] = vec![<E as ff_ext::ExtensionField>::BaseField::ONE]; | |
| zkvm_proof.raw_pi[0] = vec![Goldilocks::ONE]; |
(You need to import Goldilocks.)
There was a problem hiding this comment.
Indeed it improve readibility, fixed in dc1f245
| zkvm_proof.raw_pi[0] = vec![<E as ff_ext::ExtensionField>::BaseField::ONE]; | ||
| zkvm_proof.raw_pi[1] = vec![<E as ff_ext::ExtensionField>::BaseField::ONE]; | ||
|
|
||
| // capture panic message, if have |
There was a problem hiding this comment.
Ah, I recognise this code from elsewhere. The whole file is mostly copy-and-pasted, isn't it?
Please consider less duplication.
There was a problem hiding this comment.
See https://github.com/scroll-tech/ceno/pull/603/files for a cleanup that's already in the works. (Feel free to take it over, or clean it up yourself; but making copies of the old code will make it harder to clean up in multiple places.)
There was a problem hiding this comment.
PR 603 with_panic_hook idea is worthy and improve on readibilty. I refactored two place to share this utility and fix in dc1f245
hero78119
force-pushed
the
feat/add_fibonacchi_bench
branch
from
19b6164
to
dc1f245
Compare
Context
Existing benchmark
riscv_addare just for testing single opcode performance. This PR aim to have a first e2e bench which is representative to a workload in real world.What has been cover
what's not being covered
Hey @mcalancea, key generation + witness assignment are still encaptulated in same function and not including in bench latency meature. Break down key generation and extract witness assignment will expose massive intermediate variables in between, so probably need another refactor and design to support #624