Conversation
ledwards2225
force-pushed
the
lde/ultra_bench_update
branch
from
afdbf8c to
0000ce9
Compare
ledwards2225
force-pushed
the
lde/ultra_bench_update
branch
from
cd02df3 to
e7498b3
Compare
| # Enable Job Summary for PRs | ||
| summary-always: true | ||
| # Show alert with commit comment on detecting possible performance regression | ||
| alert-threshold: '120%' # alert if bench result is 1.2x worse |
There was a problem hiding this comment.
Will this be enough if there are incremental changes that drain performance?
There was a problem hiding this comment.
To be honest I'm viewing the GH Action portion of this work as just a nice to have with pretty pictures. I think it needs to be incumbent upon the developer to manually check the benchmarks in their branch any time they suspect a change. I suppose we could also automate running the benchmarks and comparing with master but the dev would still have to manually check the result
There was a problem hiding this comment.
It's a nice facility, so why not use it? If it's possible to add several alerts, I'd do a 90% alert, too. It can be even more disturbing if the circuit shrinks for no reason
There was a problem hiding this comment.
Looking at the docs it seems that there's no way to have an upper and lower bound for the alert. The nice thing about having the autogenerated plots is that unexpected changes will not go unnoticed and the offending commit is displayed. Maybe I'll change this threshold to 1.05x and we'll see how that goes. I'm not sure what the random variation will be
|
|
||
| typename curve::g1_bigfr_ct public_key = curve::g1_bigfr_ct::from_witness(&composer, account.public_key); | ||
|
|
||
| proof_system::plonk::stdlib::ecdsa::signature<Composer> sig{ typename curve::byte_array_ct(&composer, rr), |
There was a problem hiding this comment.
I'd generate several separate unique signatures, because at some point this can devolve into basically one, if we do optimisations correctly.
There was a problem hiding this comment.
good call, done
| field_ct root = witness_ct(&composer, db.root()); | ||
|
|
||
| for (size_t i = 0; i < num_iterations; i++) { | ||
| merkle_tree::check_membership( |
There was a problem hiding this comment.
Here, too. Ideally for several iterations of benchmarks it shouldn't be absolutely the same action. I assume we'll add filtering at some point and a repeat action on the same witness indices would not lead to additional gates
There was a problem hiding this comment.
Updated to check a different leaf each time
| @@ -0,0 +1,224 @@ | |||
| #include "barretenberg/crypto/ecdsa/ecdsa.hpp" | |||
There was a problem hiding this comment.
It's not pressing now, but this file could be split into a cpp and hpp file, so that all instantiations of benchmarks don't reinclude all these hpp files
There was a problem hiding this comment.
Leaving this as a TODO. I'm sure I'll be mucking around in here again soon
ledwards2225
force-pushed
the
lde/ultra_bench_update
branch
from
4a4b88c to
5bd1b00
Compare
feat: Benchmark suite update
feat: Benchmark suite update
4 participants
Description
This PR has two primary components:
Establishes some common circuits for benchmarking the Standard and Ultra variants of Plonk and Honk and introduces scripts for generating comparisons between Plonk and Honk for both Standard/Ultra. It also updates the script for comparing a development branch to master to use Ultra Honk. (This is the script to be used during development to evaluate the effectiveness of an optimization).
Sets up a GitHub Action to run the UltraHonk benchmarks on pushes to
master. The results are automatically used to update the plots here.Checklist:
/markdown/specshave been updated.@briefdescribing the intended functionality.