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

Add Field::sum_of_products method #80

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

str4d
Copy link
Member

@str4d str4d commented Apr 26, 2022

Closes #79.

ff_derive/src/lib.rs Outdated Show resolved Hide resolved
@str4d
Copy link
Member Author

str4d commented Apr 27, 2022

Force-pushed to remove the ff_derive efficient override (which I don't have time to prove correct), and change the API to consume an iterator of pairs of references instead of two arrays (to make it more flexible).

/// above. Implementations of `Field` should override this to use more efficient
/// methods that take advantage of their internal representation, such as interleaving
/// or sharing modular reductions.
fn sum_of_products<'a, I: IntoIterator<Item = (&'a Self, &'a Self)> + Clone>(pairs: I) -> Self {
Copy link
Member Author

Choose a reason for hiding this comment

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

I originally made this an iterator of &(a, b), and I tested this against the array impl and saw little-to-no performance difference. I then changed it to an iterator of (&a, &b) because I thought it would be more flexible, but doing that significantly hurts performance (I presume because we're calling .clone() on an owned tuple that contains references, rather than on a reference).

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, even using nicer tricks for constructing the input arrays, having this API take (&a, &b) causes bls12_381 pairings to be over 5% slower compared to arrays or &(a, b).

Copy link
Member Author

Choose a reason for hiding this comment

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

Tried two iterators instead:

fn sum_of_products<'a>(
        a: impl IntoIterator<Item = &'a Self> + Clone,
        b: impl IntoIterator<Item = &'a Self> + Clone,
    ) -> Self

and then a.clone().into_iter().zip(b.clone().into_iter()); this is similarly slower than arrays.

@str4d
Copy link
Member Author

str4d commented Oct 29, 2022

Rebased on current main.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Field::sum_of_products
1 participant