-
Notifications
You must be signed in to change notification settings - Fork 34
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 large vector allocations #144
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These allocations were really nagging me, I'm glad you're taking care of this! The changes look fine to me, and I think this is a good atomic change to get in soon.
src/r1cs/mod.rs
Outdated
let u_1_cdot_CZ_2 = U1.u * CZ_2[i]; | ||
AZ_1_circ_BZ_2 + AZ_2_circ_BZ_1 - u_1_cdot_CZ_2 - CZ_1[i] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very small nit: subtraction is ever so slightly more expensive than addition. Computing
u_1_cdot_Cz_2_plus_Cz_1 = U1.u * CZ_2[I] + CZ_1[i]
and returning
AZ_1_circ_BZ_2 + AZ_2_circ_BZ_1 - u_1_cdot_Cz_2_plus_Cz_1
would replace one subtraction by an addition.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is a good change. I would also like to see (in a PR, or in separate commits attached to this PR) showing a slightly more extensive variant of that where we delete (instead of adapting, as you do here with multiply_witness_unchecked
) the allocating methods that this would make obsolete.
If we're going to go for fine-grained allocation control, let's do it fully and not maintain "shadow" allocating methods that would be left unused.
src/lib.rs
Outdated
// save the inputs before proceeding to the `i+1`th step | ||
let r_U_primary_i = self.r_U_primary.clone(); | ||
let r_U_secondary_i = self.r_U_secondary.clone(); | ||
// let l_u_primary_i = self.l_u_primary.clone(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this intentional?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@winston-h-zhang ping
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, one second
@huitseeker The allocating methods are still required by SuperNova, and I wanted to avoid the PR from spilling over. My plan was to wait once this is merged in, then open the same set of PRs for SuperNova, and everything should be cleaned up. |
cda7c6f
to
74a44a0
Compare
a300f84
to
d599a53
Compare
d599a53
to
8f7f8e6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉
* remove large vector allocations * add suggestions
Addresses #137.
This PR addresses the cloning/allocation of large vectors used in the intermediary steps of each Nova
prove_step
computation. In particular, we focus onNIFS::prove
, which allocates new vectors to compute the fold step, when technically we could be reusing and overwriting the already allocated vectors from the previous step. Concretely, we add the functionsNIFS::prove_mut
andR1CSRelaxedWitness/Instance::prove_mut
. We also add aResourceSink
with preallocated buffers forABC_Z{1,2}
andT
s to help as stratch memory for thecommit_T_into
operation.