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 str4d force-pushed the 79-sum-of-products branch from 9d9b11f to 8ef6be8 Compare April 27, 2022 23:00
@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).

@str4d str4d force-pushed the 79-sum-of-products branch from 8ef6be8 to be49a7c Compare April 27, 2022 23:05
/// 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 str4d force-pushed the 79-sum-of-products branch from be49a7c to 77fb7f0 Compare October 29, 2022 03:18
@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