Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Lagrange evaluation performance improvements #1409

Merged
merged 3 commits into from
Nov 12, 2024
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 29 additions & 22 deletions ipa-core/src/protocol/ipa_prf/malicious_security/lagrange.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use std::{borrow::Borrow, fmt::Debug};
use std::fmt::Debug;

use typenum::Unsigned;

Expand Down Expand Up @@ -79,8 +79,7 @@

impl<F, const N: usize> LagrangeTable<F, N, 1>
where
F: Field + TryFrom<u128>,
<F as TryFrom<u128>>::Error: Debug,
F: PrimeField,
{
/// generates a `CanonicalLagrangeTable` from `CanoncialLagrangeDenominators` for a single output point
/// The "x coordinate" of the output point is `x_output`.
Expand All @@ -95,31 +94,16 @@

impl<F, const N: usize, const M: usize> LagrangeTable<F, N, M>
where
F: Field,
F: PrimeField,
{
/// This function uses the `LagrangeTable` to evaluate `polynomial` on the _output_ "x coordinates"
/// that were used to generate this table.
/// It is assumed that the `y_coordinates` provided to this function correspond the values of the _input_ "x coordinates"
/// that were used to generate this table.
pub fn eval<I>(&self, y_coordinates: I) -> [F; M]
where
I: IntoIterator + Copy,
I::IntoIter: ExactSizeIterator,
I::Item: Borrow<F>,
{
debug_assert_eq!(y_coordinates.into_iter().len(), N);

pub fn eval(&self, y_coordinates: &[F; N]) -> [F; M] {
self.table
.iter()
.map(|table_row| {
table_row
.iter()
.zip(y_coordinates)
.fold(F::ZERO, |acc, (&base, y)| acc + base * (*y.borrow()))
})
.collect::<Vec<F>>()
.try_into()
.unwrap()
.each_ref()
.map(|row| dot_product(row, y_coordinates))
}

/// helper function to compute a single row of `LagrangeTable`
Expand Down Expand Up @@ -176,6 +160,29 @@
}
}

/// Computes the dot product of two arrays of the same size.
/// It is isolated from Lagrange because there could be potential SIMD optimizations used
fn dot_product<F: PrimeField, const N: usize>(a: &[F; N], b: &[F; N]) -> F {
// Staying in integers allows rustc to optimize this code properly
// with any reasonable N, we won't run into overflow with dot product.
// (N can be as large as 2^32 and still no chance of overflow for 61 bit prime fields)
debug_assert!(
F::PRIME.into() < (1 << 64),
"The prime {} is too large for this dot product implementation",
F::PRIME.into()

Check warning on line 172 in ipa-core/src/protocol/ipa_prf/malicious_security/lagrange.rs

View check run for this annotation

Codecov / codecov/patch

ipa-core/src/protocol/ipa_prf/malicious_security/lagrange.rs#L171-L172

Added lines #L171 - L172 were not covered by tests
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are multiplying two field values and then summing. I'd say that the conservative requirement is 2 * F::BITS + N.next_power_of_two().ilog2() <= u128::BITS.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's right, I think this assertion just assumes N being reasonable and the second argument of this expression negligible compared to F::BITS. I don't mind changing it, the reasoning behind this was just to optimize for readers

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think what I said was correct here - I did go over the math again and we can't sustain $2^{32}$ additions for sure w/o risk of an overflow. @andyleiserson suggestion was correct

);

let mut sum = 0;

// I am cautious about using zip in hot code
// https://github.com/rust-lang/rust/issues/103555
for i in 0..N {
sum += a[i].as_u128() * b[i].as_u128();
}

F::truncate_from(sum)
}

#[cfg(all(test, unit_test))]
mod test {
use std::{borrow::Borrow, fmt::Debug};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -159,7 +159,7 @@ where
last_array[1..last_u_or_v_values.len()].copy_from_slice(&last_u_or_v_values[1..]);

// compute and output p_or_q
tables.last().unwrap().eval(last_array)[0]
tables.last().unwrap().eval(&last_array)[0]
}

#[cfg(all(test, unit_test))]
Expand Down