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

Cleanup memory pipeline for SuperNova #227

Merged
merged 3 commits into from
Jan 4, 2024
Merged

Conversation

winston-h-zhang
Copy link
Member

@winston-h-zhang winston-h-zhang commented Jan 3, 2024

Resolves the next step of #137.

This PR brings the changes done to the Nova memory pipeline in #139, #143, and #144 to SuperNova as well. Since the core logic was already established in the previous Nova PRs, this PR mostly just refactors and cleans up the SuperNova API.

Notes:

  • We don't remove the use of R1CSShape::multiply_vec, since it's still in (pp)snark.rs. The concatenation of z = W.W ++ U.u ++ U.X happens anyways, so we can keep the instances of S.multiply_vec(z) for readability.

@winston-h-zhang winston-h-zhang marked this pull request as ready for review January 4, 2024 01:42
@winston-h-zhang winston-h-zhang changed the title [wip] Cleanup memory pipeline for SuperNova Cleanup memory pipeline for SuperNova Jan 4, 2024
adr1anh
adr1anh previously approved these changes Jan 4, 2024
Copy link
Contributor

@adr1anh adr1anh left a comment

Choose a reason for hiding this comment

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

Small nit about initializing the ResourceBuffer but other than that it looks good to me!

Comment on lines 618 to 632
// find the largest length r1cs shape for the buffer size
let mut max_r1cs_primary = &pp[0].r1cs_shape;
pp.circuit_shapes.iter().for_each(|circuit| {
if circuit.r1cs_shape.num_cons > max_r1cs_primary.num_cons {
max_r1cs_primary = &circuit.r1cs_shape;
}
});

let buffer_primary = ResourceBuffer {
l_w: None,
l_u: None,
ABC_Z_1: R1CSResult::default(max_r1cs_primary),
ABC_Z_2: R1CSResult::default(max_r1cs_primary),
T: r1cs::default_T(max_r1cs_primary),
};
Copy link
Contributor

Choose a reason for hiding this comment

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

R1CSResult::default() only requires the num_cons value from the R1CSShape. If we change the function to take this value instead, then we can more simply compute

let max_num_cons = *pp.circuit_shapes.iter().map(|shape| shape.num_cons).max().unwrap();
let buffer_primary = ResourceBuffer {
      l_w: None,
      l_u: None,
      ABC_Z_1: R1CSResult::default(max_num_cons),
      ABC_Z_2: R1CSResult::default(max_num_cons),
      T: r1cs::default_T(max_num_cons),
    }; 

Copy link
Member

@huitseeker huitseeker left a comment

Choose a reason for hiding this comment

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

This looks great, thank you so much!

@winston-h-zhang winston-h-zhang added this pull request to the merge queue Jan 4, 2024
Merged via the queue into dev with commit 52a57d6 Jan 4, 2024
7 checks passed
@winston-h-zhang winston-h-zhang deleted the sn-spmvm-allocations branch January 4, 2024 21:29
@huitseeker huitseeker mentioned this pull request Jan 8, 2024
5 tasks
huitseeker added a commit to huitseeker/arecibo that referenced this pull request Jan 14, 2024
* Small perf items (argumentcomputer#12)

* Optimize generator creation in provider module

- Refactored the `from_label` function in pasta.rs removing redundant collect, flatten operations and Vec<usize> creation,

* fix: rename VanillaRO -> NativeRO

* refactor: Optimize circuit methods and enhance trait compatibility

- Replaced the `.map()` function with the `.map_or()` function in `r1cs.rs` for efficiency.
- Added `Eq` trait derivation to `NovaAugmentedCircuitParams` in `circuit.rs` to enable comparisons.
- Introduced `Eq` trait to `TrivialTestCircuit` struct in `traits/circuit.rs` without altering its input or output.
- Optimized performance by refactoring the use of iterator `into_iter()` to the iterator itself in `gadgets/r1cs.rs`.

* ci: Bring clippy standards lurk-rs -> arecibo (argumentcomputer#13)

* ci: Bring clippy standards lurk-rs -> arecibo

- Added a new .cargo/config file with a custom alias "xclippy" to execute Clippy lints for code quality assurance,
- Fixed code correspondingly when necessary,
- Added xclippy to CI

* chore: add dbg_macro to ci's clippy

- Implemented a new clippy lint for the dbg_macro in the cargo config file

* test: Extend tests to add missing curve variants (argumentcomputer#33)

- Enhanced testing for multiple modules, including bellpepper, spartan, and provider, focusing on the addition and multiplication of multi-linear polynomials and the Keccak transcript.
- Introduced usage of secp256k1::Scalar (and sometimes bn256_grumpkin::bn256::Scalar) into tests for improved coverage.
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.

3 participants