Improve the interpolation performance and avoid unnecessary state clones

[zhenfeizhang]

Description

This PR

• improve the internal API to avoid unnecessary clones of prover and verifier states during IOP
• improve the verifier complexity from O(num_vars^3) to O(num_vars^2)

with this PR

Verify/ML/10            time:   [143.88 us 144.17 us 144.48 us]

Verify/ML/11            time:   [156.66 us 157.11 us 157.58 us]

with 3d5c532 (prior to this PR)

Verify/ML/10            time:   [261.08 us 261.52 us 262.06 us]
                        change: [+78.989% +80.218% +82.102%] (p = 0.00 < 0.05)

Verify/ML/11            time:   [290.51 us 291.10 us 291.71 us]
                        change: [+83.756% +85.006% +86.476%] (p = 0.00 < 0.05)

---

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

• Targeted PR against correct branch (master)
• Linked to Github issue with discussion and accepted design OR have an explanation in the PR that describes this work.
• Wrote unit tests
• Updated relevant documentation in the code
• Added a relevant changelog entry to the Pending section in CHANGELOG.md
• Re-reviewed Files changed in the Github PR explorer

[weikengchen]

I updated the code a little bit.

I would not say that the complexity reduces, but we know that the concrete efficiency generally improves. I adjusted the comments for this.

[zhenfeizhang]

I would not say that the complexity reduces, but we know that the concrete efficiency generally improves. I adjusted the comments for this.

Is it because get_divisor is still a quadratic function?
We can actually use a similar method to compute divisors in linear time -- at a cost of 2 field divisions (where as the current code only has one division) per iteration. But it appears that O(n^2) primitive type multiplications is still faster than 1 field division.

Anyway, thanks for the review, and I am okay with the changes.

[weikengchen]

Do you have the code for that? I am thinking about including a general-purpose case, even if it can be slightly slow (note that this interpolation usually is not the bottleneck).

[zhenfeizhang]

I just pushed the code for that, and I managed to avoid the extra field division I thought that I needed.
Here are the latest benchmark result

Verify/ML/10            time:   [127.30 us 128.11 us 128.94 us]  
Verify/ML/11            time:   [147.69 us 148.81 us 150.05 us]              

Wait -- the code is currently buggy: the numerator for denominator may explode for pi.len = 20. Let me fix this.
fixed

[weikengchen]

Should field_factorial also use field elements in the middle of the computation?

[zhenfeizhang]

Should field_factorial also use field elements in the middle of the computation?

I think not. i64 already allows for 2^60+ number of elements in a single commit, or a zk circuit with 2^60+ variables. i would argue that one will never need to use a large commitment that will overflow i64.

[tsunrise]

Looks good to me! I will make a new benchmark diagram later, and we can merge it then.

[weikengchen]

I will need to look at the field_factorial again. Please wait for me :)

[tsunrise]

I will need to look at the field_factorial again. Please wait for me :)

sure, no rush (temporarily un-approve this for now in case someone merges this accidentally)

temporarily unapprove this because we have more things to do: #55 (comment)

[zhenfeizhang]

field_factorial

My bad. Misunderstood your point @weikengchen . fixed with e6d4b9d