feat: permutation argument optimizations

[ledwards2225]

A handful of optimizations for the large ambient trace setting, mostly to do with the grand product argument. Total savings is about 1s on the "17 in 20" benchmark.

• Only perform computation for the grand product on active rows of the trace. This means (1) only setting the values of sigma/id on the active rows (they remain zero elsewhere since those values don't contribute to the grand product anyway). And (2) only compute the grand product at active rows then populate the constant regions as a final step. These are both facilitated by constructing a vector active_row_idxs which explicitly contains the indices of the active rows. This makes it easier to multithread and is much more efficient than looping over the entire domain and using something like check_is_active() which itself has low overhead but results in huge disparities in the distribution of actual work across threads.
• Replace a default initialized std::vector in PG with a Polynomial simply to take advantage of the optimized constructor

Branch "17 in 20" benchmark

ClientIVCBench/Full/6 20075 ms 17763 ms

Master "17 in 20" benchmark

ClientIVCBench/Full/6 21054 ms 18395 ms

The conventional benchmark ("19 in 19") shows a very minor improvement, as expected:

Branch:

ClientIVCBench/Full/6 22231 ms 19857 ms

Master:

ClientIVCBench/Full/6 22505 ms 19536 ms

[ledwards2225]

Using Polynomial here instead of vector simply because it has a much more efficient constructor for initializing all of its elements to zero

[ledwards2225]

This loop now iterates over only the active domain instead of the entire poly domain. Prior to this change, the sigma/id polynomials took non-zero values across the entire domain. Now, they are non-zero only in the active regions of the trace and 0 elsewhere (previously we had sigma_i == id_i in these regions). These values don't contribute to the computation of the grand product anyway so there's no reason to compute them.

[ledwards2225]

this didn't need to change, I just wanted to make sure that tampering was a bit more robust. In theory setting a wire value at a row where no gates/copy constraints are active could still (correctly) lead to a verifiable proof

[maramihali]

would be nice to have the timings for what happens in a 2^19 trace as well :)

[maramihali]

Looks good, small comments/suggestions and a question

[maramihali]

don't these ranges need to be non-overlapping and in increasing order? maybe there should be a comment of some sort of check

[ledwards2225]

its a good point. I could add a check on add_range that the input has start >= the previous end. To be safe I suppose I'd also want to make the members private and add getters

[maramihali]

why is this cast needed?

[maramihali]

also this can be a const and the one above too

[ledwards2225]

The cast is needed since row_idx has type std::shared_ptr<uint32_t[]> which can only be indexed with a ptrdiff_t. (Note this isn't a change introduced in this PR)

[maramihali]

has_active_regions?

[ledwards2225]

hah I started with that but thought it was misleading because if false it implies that there are NO active regions when really its just that they are implicit and haven't been specified. You're probably right tho that has_active_regions is more clear

[maramihali]

if constexpr (!IsPlonkFlavor<Flavor>) would make this more readable I think

[ledwards2225]

That's not quite the same thing though because this code is also used by the ECCVM/Translator which need to be excluded. I think this just comes down to the fact that we need better concepts. Probably isUltraOrMegaHonk

[ledwards2225]

I suppose I could just add methods to the ECCVM/Trans flavors that just call get_row from get_row_for_permutation_arg. Not sure what's better

[maramihali]

const size_t

[maramihali]

This comment seems incomplete

[ledwards2225]

haha its not but I do see what you mean. I reordered to make it sound more natural