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

Cache FFT roots #261

Merged
merged 1 commit into from
Sep 22, 2021
Merged

Cache FFT roots #261

merged 1 commit into from
Sep 22, 2021

Conversation

nbgl
Copy link
Contributor

@nbgl nbgl commented Sep 22, 2021

This unfortunately results in the root table being passed around a bit as part of ProverOnlyCircuitData, but it does speed up PolynomialBatchCommitment::lde_values by 10%

@nbgl nbgl requested a review from dlubarov September 22, 2021 00:24
src/field/fft.rs Outdated
@@ -42,6 +42,13 @@ fn fft_classic_root_table<F: Field>(n: usize) -> FftRootTable<F> {
root_table
}

pub fn fft_default_strategy_root_table<F: Field>(n: usize) -> FftRootTable<F> {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can get rid of this once we merge #258.

) -> Vec<F> {
let n = input.len();
let computed_root_table = if let Some(_) = root_table {
Copy link
Contributor

Choose a reason for hiding this comment

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

Argh that's annoying that we can't use unwrap_or_else (in a reasonable way). I don't see a better way though.

Copy link
Contributor

@dlubarov dlubarov left a comment

Choose a reason for hiding this comment

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

LGTM

@nbgl nbgl merged commit 7360391 into main Sep 22, 2021
@nbgl nbgl deleted the jakub/cache-fft-roots branch September 22, 2021 17:56
@nbgl nbgl mentioned this pull request Sep 22, 2021
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.

2 participants