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

Remove duplicate poly definitions and parallelize evaluation #13

Merged
merged 6 commits into from
Jun 27, 2024

Conversation

Cesar199999
Copy link
Contributor

Closes #9. Removes duplicated EqPolynomial and MultilinearPolynomial definitions by removing the
polynomial.rs file. Both types are now only in the eq.rs. No other changes were required in
the codebase as the types in polynomial.rs were not used anywhere else.

In addition, this PR parallelizes the EqPolynomial::evaluate function using Rayon's into_par_iter
method, and adds benchmarks for the function. The benchmarks (on my HP EliteBook 840 G7) show that
the parallelized version of the function performs better for large instances:

evaluate_incremental/2^1
                       time:   [13.666 µs 13.717 µs 13.768 µs]
                       change: [+6812.6% +6937.9% +7055.6%] (p = 0.00 < 0.05)
                       Performance has regressed.
Found 6 outliers among 100 measurements (6.00%)
 3 (3.00%) low mild
 2 (2.00%) high mild
 1 (1.00%) high severe
evaluate_incremental/2^3
                       time:   [14.974 µs 15.108 µs 15.262 µs]
                       change: [+1966.1% +2019.2% +2074.8%] (p = 0.00 < 0.05)
                       Performance has regressed.
Found 11 outliers among 100 measurements (11.00%)
 4 (4.00%) high mild
 7 (7.00%) high severe
evaluate_incremental/2^5
                       time:   [18.359 µs 18.562 µs 18.785 µs]
                       change: [+545.64% +552.71% +559.60%] (p = 0.00 < 0.05)
                       Performance has regressed.
Found 3 outliers among 100 measurements (3.00%)
 3 (3.00%) high mild
evaluate_incremental/2^7
                       time:   [28.191 µs 28.402 µs 28.628 µs]
                       change: [+145.80% +148.43% +151.12%] (p = 0.00 < 0.05)
                       Performance has regressed.
Found 4 outliers among 100 measurements (4.00%)
 3 (3.00%) high mild
 1 (1.00%) high severe
evaluate_incremental/2^9
                       time:   [55.980 µs 57.917 µs 60.159 µs]
                       change: [+21.740% +23.970% +26.544%] (p = 0.00 < 0.05)
                       Performance has regressed.
Found 8 outliers among 100 measurements (8.00%)
 4 (4.00%) high mild
 4 (4.00%) high severe
evaluate_incremental/2^11
                       time:   [112.90 µs 114.63 µs 116.48 µs]
                       change: [-38.754% -37.528% -35.992%] (p = 0.00 < 0.05)
                       Performance has improved.
Found 2 outliers among 100 measurements (2.00%)
 2 (2.00%) high mild
evaluate_incremental/2^13
                       time:   [277.95 µs 282.42 µs 287.06 µs]
                       change: [-62.248% -61.689% -61.053%] (p = 0.00 < 0.05)
                       Performance has improved.
Found 2 outliers among 100 measurements (2.00%)
 1 (1.00%) high mild
 1 (1.00%) high severe
evaluate_incremental/2^15
                       time:   [953.95 µs 986.72 µs 1.0333 ms]
                       change: [-69.195% -68.426% -67.543%] (p = 0.00 < 0.05)
                       Performance has improved.
Found 3 outliers among 100 measurements (3.00%)
 2 (2.00%) high mild
 1 (1.00%) high severe
evaluate_incremental/2^17
                       time:   [3.4105 ms 3.4747 ms 3.5640 ms]
                       change: [-71.229% -70.680% -69.911%] (p = 0.00 < 0.05)
                       Performance has improved.
Found 5 outliers among 100 measurements (5.00%)
 2 (2.00%) high mild
 3 (3.00%) high severe
evaluate_incremental/2^19
                       time:   [11.854 ms 12.042 ms 12.241 ms]
                       change: [-75.035% -74.632% -74.186%] (p = 0.00 < 0.05)
                       Performance has improved.
Found 1 outliers among 100 measurements (1.00%)
 1 (1.00%) high mild

@Cesar199999 Cesar199999 marked this pull request as ready for review March 15, 2024 14:50
@srinathsetty
Copy link
Contributor

Thanks for the PR! This improvement might apply here as well: https://github.com/microsoft/Nova/blob/4f8f3e782b172e98d6d741b29e5bc671ab5b93a6/src/spartan/polys/eq.rs#L36

@Cesar199999
Copy link
Contributor Author

Thanks for the PR! This improvement might apply here as well: https://github.com/microsoft/Nova/blob/4f8f3e782b172e98d6d741b29e5bc671ab5b93a6/src/spartan/polys/eq.rs#L36

Thank you. Sure, I'll create a PR there too.

@Cesar199999
Copy link
Contributor Author

Hey @srinathsetty, is it possible to merge this?

@srinathsetty srinathsetty merged commit afbf72b into microsoft:main Jun 27, 2024
2 checks passed
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.

Duplicate poly definitions
3 participants