pass configurable window_size to jubjub #783
Conversation
sidke
requested review from
dignifiedquire,
laser and
porcuquine
as code owners
sidke
force-pushed
the
features/configurable-window-size
branch
from
8634aa3
to
ce2ba67
Compare
There was a problem hiding this comment.
LGTM. I have one comment which is not blocking for this PR, but is important for benchmarks in general. It might be better if addressed here and now. So consider semi-blocking, pending @laser's call either way.
| .unwrap() | ||
| .pedersen_hash_exp_window_size; | ||
| let jubjub_params = JubjubBls12::new_with_window_size(window_size); | ||
| let jubjub_params2 = JubjubBls12::new_with_window_size(window_size); |
There was a problem hiding this comment.
It's weird that we need to do this, and I suspect it skews our view of memory consumption in such a benchmark (cc: @laser).
Context reminder: the larger the window size, the faster the hash but the (exponentially) larger the cache. Allocating two instances of Jubjub parameters will allocate double the cache. If we don't account for that, we might make bad judgments about the optimal window size. Can we make sure this issue is accounted for when providing memory-consumption stats for micro-benchmarks?
There was a problem hiding this comment.
We do not measure memory consumption for micro benchmarks at all. They are neither written in a way that makes sense, nor are we set up for that.
There was a problem hiding this comment.
We do not measure memory consumption for micro benchmarks at all.
That is correct.
There was a problem hiding this comment.
Then it's a non-issue, thanks.
sidke
force-pushed
the
features/configurable-window-size
branch
2 times, most recently
from
c015159
to
d82de5a
Compare
| let mut rng = XorShiftRng::from_seed([0x5dbe6259, 0x8d313d76, 0x3237db17, 0xe5bc0654]); | ||
|
|
||
| let cases = [(32, 689), (64, 1376)]; | ||
|
|
||
| for (bytes, constraints) in &cases { | ||
| let mut cs = TestConstraintSystem::<Bls12>::new(); | ||
| let data: Vec<u8> = (0..*bytes).map(|_| rng.gen()).collect(); | ||
| let params = &JubjubBls12::new(); | ||
| let params = &JubjubBls12::new_with_window_size(window_size); |
There was a problem hiding this comment.
instead of having the settings code + the new_with_window_size how about a single method in storage-proofs like new_jubjub_bls12_current_window_size, that does the work for us?
sidke
force-pushed
the
features/configurable-window-size
branch
from
d82de5a
to
26a3a2c
Compare
What's in this PR?
JubjubBls12resolves #736