feat: Faster randomness sampling for field elements#9627
Conversation
| numeric::RNG& engine = numeric::get_randomness(); | ||
| for (auto _ : state) { | ||
| state.PauseTiming(); | ||
| size_t num_cycles = 1 << static_cast<size_t>(state.range(0)); |
There was a problem hiding this comment.
minor nit: numeric literals like 1 are usually interpreted as int types. Might make the code ever so slightly more readable by making this 1UL (32 bit unsigned long). Up to you though, this isa very petty comment.
| BENCHMARK(uint_multiplication)->Unit(kMicrosecond)->DenseRange(12, 27); | ||
| BENCHMARK(uint_extended_multiplication)->Unit(kMicrosecond)->DenseRange(12, 27); | ||
| BENCHMARK(pippenger)->Unit(kMicrosecond)->DenseRange(16, 20)->Setup(DoPippengerSetup)->Iterations(5); | ||
| BENCHMARK(bn254fr_random)->Unit(kMicrosecond)->DenseRange(10, 16); |
There was a problem hiding this comment.
when making a zk proof do we not need randomness equal to the circuit size? It might be useful to extend this to 20 to validate something odd doesn't happen for larger circuits
There was a problem hiding this comment.
I assume we'll do it in parallel. And right now 2^20 is very slow
| } | ||
|
|
||
| // This test shows that ((lo|hi)% modulus) in uint512_t is equivalent to (lo + 2^256 * hi) in field elements so we | ||
| // don't have to use the slow API |
There was a problem hiding this comment.
what is the "slow API"? might be useful to make this more explicit so that this comment does not have implied context that future readers might not have
| uint512_t q(modulus); | ||
| uint512_t reduced = source % q; | ||
| return field(reduced.lo); | ||
| constexpr field pow_2_256 = field(uint256_t(1) << 128).sqr(); |
There was a problem hiding this comment.
How sensitive are we to performance here? If we wanted to maximize performance, I think the following algorithm would be fastest:
- generate random u512
t - feed
tdirectly into a Barrett reduction algorithm, producing outputt*2^{-256} mod p - multiply the output by 2^{512} mod p to return
t * 2^{256} mod p(which ist mod pconverted into Montgomery form, which is what the underlyingfieldtype expects)
The above requires 2 modular reductions. The current method performs 3, due to converting two random u256 values into field elements (along with additional arithmetic operations)
Very minor point, I don't know how performance sensitive we are to this algorithm
There was a problem hiding this comment.
Our old modulo algorithm was super slow, so this is a significant improvement. I tried checking what kind of an improvement would not converting to montgomery for 256-bit chunks make and it was minimal. The problem is that now that the division algorithm is not extremely slow, the bottleneck is sampling the uint512_t.
There was a problem hiding this comment.
Also we don't do 2^256 montgomery in wasm. It's different(
There was a problem hiding this comment.
thanks for the context about the wasm. Replace 2^{256} with whatever the montgomery R parameter is and I think the above all applies.
But yes I appreciate it doesn't affect performance too much, it's the difference between 3 reductions and 2. It makes sense that generating the random bytes would be the limiting factor after your changes.
zac-williamson
left a comment
zac-williamson
left a comment
There was a problem hiding this comment.
I left some comments about minor details, as well as a potential improvement. But any changes are optional so I have approved. good work!
2 participants
Changes the algorithm for converting uint512_ts to fields. The result is equivalent (we can make an even faster version, but without the equivalence). 1.5x faster on the mainframe, 5x in wasm
Before. Native
After. Native:
Before. Wasm:
After. Wasm:
