-
Notifications
You must be signed in to change notification settings - Fork 217
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
R1CS: Add constraints_len
, deferred_constraints_len
and total_constraints_len
#322
R1CS: Add constraints_len
, deferred_constraints_len
and total_constraints_len
#322
Conversation
src/r1cs/constraint_system.rs
Outdated
@@ -69,6 +69,17 @@ pub trait ConstraintSystem { | |||
/// Counts the amount of allocated multipliers. | |||
fn multipliers_len(&self) -> usize; | |||
|
|||
/// Counts the amount of first-phase constraints in the circuit. | |||
fn constraints_len(&self) -> usize; |
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.
To my taste, i'd leave this generic sounding name for all the constraints, not just the first phase. If it's important to distinguish between first phase/second phase, I'd have specific methods for them.
Also: it may be useful to just have one method stats() -> Stats
that returns a non-exhaustive struct (with private PhantomData or smth like that?) that contains .multipliers: usize, .constraints: usize, first_phase_constraints: usize, .second_phase_constraints: usize
etc.
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.
Ack on the naming, I'll swap that out.
The most valuable thing about such a Stats
struct would be its (derived) Debug
implementation. On the other hand, these methods are not really to be "used in production" anyway, so having a separate struct may be a tid of overengineering...
Anyhow, your pick, let me know what you prefer!
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.
For debugging it seems to be more useful to have all stats/metrics in one nifty struct, so we don't have to fiddle with calling individual methods. We don't need obnoxious API for that thing, other than pub fields and derive(Debug).
Having a private field is only to be future-proof and be able to add more fields w/o breaking people's code.
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.
Feel free to move the current stats under that struct and break the API.
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.
Maybe another question: what would be the preferred method and struct name? Stats
sounds like it's a statistic, while everything is a constant. Should I prefer Metrics
or even CircuitMetrics
or something like that?
Beyond multipliers and constraint counts, I don't think there's other metrics to include, right?
Another question: the private PhantomData you refer to, why would the Stats
struct need to be generic; do you mean should it reference to the ConstraintSystem, such that an instance of Stats
is always correct (by making the CS immutable for the lifetime of the Stats instance)?
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.
Yeah, something alongside "metrics" sounds better. I'd prefer to avoid using the term "circuit" since we are not using it anywhere in the API. Maybe simply r1cs::Metrics
is good enough?
As of PhantomData: sorry, i meant having a dummy private field with a zero-sized type, so the struct cannot be used in any other way than just reading its fields. E.g. no destructuring etc.
#[derive(Debug)]
struct Metrics {
pub multipliers_count: usize,
pub constraints_count: usize,
_private: ()
}
But it makes Debug printout uglier and you cannot opt out easily. Maybe we should just list the fields w/o any extra precautions.
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.
FWIW, I'd leave destructuring open for the user. It may be handy for writing benchmarks, etc; I don't think it leaves a lot open for abuse.
67f4bd7
to
4161af2
Compare
Made a |
Add Metrics struct to count the different kinds of constraints, which comes in handy when developing and benchmarking larger circuits. This deprecates and removes the `multipliers_len()` method.
1ed7164
to
26c7e2c
Compare
Squashed together; didn't make any sense anymore as separate commits. Note that this removes |
I've added documentation suggestions - please feel free to reword how you please. This patch looks great! |
Co-authored-by: Oleg Andreev <oleganza@gmail.com>
I hadn't seen your update, I have accepted your suggestions! |
Nightly is already broken for everyone using dalek libraries since Rust renamed |
Add methods to count the different kinds of constraints, which comes in handy when developing and benchmarking larger circuits. This is analogue with #284,
ConstraintSystem::multipliers_len()
.