Neptune-Crypto / twenty-first

Collection of mathematics routines and cryptography for the twenty-first century
GNU General Public License v2.0
74 stars 22 forks source link

Optionally borrow polynomial coefficients #237

Closed jan-ferdinand closed 3 weeks ago

jan-ferdinand commented 1 month ago

Refactor polynomials to (a) make their innards private and (b) turn their innards into a clone-on-write slice, reducing the number of allocations needed for read-only operations like polynomial evaluation.

Pros

Cons

A lot of lines have changed only because of re-grouping to avoid multiple impl blocks with the same trait & lifetime bounds. Commit 65914d5a736cce472080b12a168b4ef5d22820f only makes the polynomial's innards private to simplify review.

Performance

The benchmark suite indicates some improvements, some regressions, and I can't seem to identify clear patterns. I will try to make sense of the results and report back in a separate comment.

[^exceptions]: Methods taking &mut self, like scalar_mul_mut() or normalize(), are unfortunate exceptions if called on a Polynomial::new_borrowed(). I thought about making those methods consuming, and return a Polynomial<'static, FF>, but have decided against that for now. Let me know if you disagree with that decision.

jan-ferdinand commented 1 month ago

@aszepieniec, @Sword-Smith, I have requested both of you for review only because I'm not sure who this is better suited for. :relieved:

coveralls commented 1 month ago

Coverage Status

coverage: 97.747% (-0.06%) from 97.81% when pulling 65914d5a736cce472080b12a168b4ef5d22820fc on poly_cow into 4af2ffa374d3be18ae6617f702ec889c92c8d920 on master.

Sword-Smith commented 1 month ago

Let me know if/when you need me to run benchmarks.

jan-ferdinand commented 3 weeks ago

Performance

In my eyes, this PR is ready to be merged.