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

Parallelize s-column generation #326

Merged

Conversation

Al-Kindi-0
Copy link
Contributor

This brings about 60% improvement in s-column generation times.
Fixes also a bug in the construction of the MLEs.

Copy link
Contributor

@plafer plafer left a comment

Choose a reason for hiding this comment

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

LGTM!

// note that `deltas[0]` is set `0` and thus `deltas` satisfies the conditions for invoking
// the function
let mut cumulative_sum = deltas;
cumulative_sum[0] = E::ZERO;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: can we set deltas[0] - E::ZERO on line 391 so that it's clear before we get to the cumulative_sum part that all elements on deltas were properly initialized?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense, updated

Comment on lines 392 to 403
batch_iter_mut!(&mut deltas[1..], 1024, |batch: &mut [E], batch_offset: usize| {
let mut query = vec![E::BaseField::ZERO; num_oracles];
let mut main_frame = EvaluationFrame::<E::BaseField>::new(num_cols);

evaluator.build_query(&main_frame, &mut query);
let cur_value = last_value - mean + gkr_data.compute_batched_query(&query) * *item;
for (i, v) in batch.iter_mut().enumerate() {
trace.read_main_frame(i + batch_offset, &mut main_frame);

result.push(cur_value);
last_value = cur_value;
}

result
evaluator.build_query(&main_frame, &mut query);
*v = gkr_data.compute_batched_query(&query) * lagrange_kernel_col[i + batch_offset]
- mean;
}
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you benchmark the serial version of this approach vs the old? In the case where this version turns out to be slower, we could keep the old version for the serial case, and this one for the parallel case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point! Updated to include the old implementation in the serial case as it a bit faster than the other one and also since I updated the implementation for the parallel case to take the number of threads into account

Copy link
Collaborator

@irakliyk irakliyk left a comment

Choose a reason for hiding this comment

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

Looks good! Thank you! I left a couple of comments inline - but we can also address them in a follow-up PR.

There also seem to be some merge conflicts.

prover/src/logup_gkr/prover.rs Show resolved Hide resolved
prover/src/logup_gkr/mod.rs Show resolved Hide resolved
@Al-Kindi-0 Al-Kindi-0 force-pushed the al-parallelize-s-col-generation branch from 6ddec5f to 74b5a5c Compare September 25, 2024 09:09
@Al-Kindi-0 Al-Kindi-0 force-pushed the al-parallelize-s-col-generation branch from 74b5a5c to b3eb72a Compare September 26, 2024 19:36
Copy link
Collaborator

@irakliyk irakliyk left a comment

Choose a reason for hiding this comment

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

Looks good! Thank you! I left one small comment inline.

air/src/air/context.rs Outdated Show resolved Hide resolved
@irakliyk irakliyk merged commit a4e383e into facebook:logup-gkr Sep 27, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants