-
Notifications
You must be signed in to change notification settings - Fork 310
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
fix: memory fragmentation fixes to cut UltraHonk memory usage by 26% #11895
Changes from all commits
7bb2457
c08599d
1290a37
a846f7a
d20a679
f0c2eac
cb0e4d9
2a96e92
f2a8816
c6047ba
6e97025
111c657
80d0631
a3491d9
a5e7bf9
c3e82b2
888b998
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -910,17 +910,23 @@ UltraProver_<Flavor> compute_valid_prover(const std::string& bytecodePath, | |
} else if constexpr (IsAnyOf<Flavor, UltraRollupFlavor>) { | ||
honk_recursion = 2; | ||
} | ||
const acir_format::ProgramMetadata metadata{ .recursive = recursive, .honk_recursion = honk_recursion }; | ||
|
||
acir_format::AcirProgram program{ get_constraint_system(bytecodePath, metadata.honk_recursion) }; | ||
if (!witnessPath.empty()) { | ||
program.witness = get_witness(witnessPath); | ||
} | ||
// TODO(https://github.com/AztecProtocol/barretenberg/issues/1180): Don't init grumpkin crs when unnecessary. | ||
init_grumpkin_crs(1 << CONST_ECCVM_LOG_N); | ||
|
||
auto builder = acir_format::create_circuit<Builder>(program, metadata); | ||
auto prover = Prover{ builder }; | ||
// Lambda function to ensure the builder gets freed before generating the vk. Vk generation requires initialing the | ||
// pippenger runtime state which leads to it being the peak, when its functionality is purely for debugging purposes | ||
// here. | ||
auto prover = [&] { | ||
const acir_format::ProgramMetadata metadata{ .recursive = recursive, .honk_recursion = honk_recursion }; | ||
acir_format::AcirProgram program{ get_constraint_system(bytecodePath, metadata.honk_recursion) }; | ||
if (!witnessPath.empty()) { | ||
program.witness = get_witness(witnessPath); | ||
} | ||
auto builder = acir_format::create_circuit<Builder>(program, metadata); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually I'm a little bit surprised that this helps - It seems like the Prover constructor will instantiate/construct most polynomials so the polys and builder will still coincide. Maybe that's true but it still decreases the peak because we don't simultaneously have builder + polys + sumcheck? I wonder if we'll need something more fine grained in the CIVC setting.. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So this compute_valid_prover function is already allowing us to free the builder before computing the proof. The problem I'm trying to solve here is that we generate the vk in this function, so we create a peak by holding the builder, prover polys, and the commitment key (including the pippenger runtime state). |
||
return Prover{ builder }; | ||
}(); | ||
|
||
size_t required_crs_size = prover.proving_key->proving_key.circuit_size; | ||
if constexpr (Flavor::HasZK) { | ||
// Ensure there are enough points to commit to the libra polynomials required for zero-knowledge sumcheck | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -278,13 +278,16 @@ TEST_F(ECCVMTranscriptTests, ProverManifestConsistency) | |
|
||
// Automatically generate a transcript manifest by constructing a proof | ||
ECCVMProver prover(builder); | ||
prover.transcript->enable_manifest(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. explicitly enable prover manifest for this test |
||
prover.ipa_transcript->enable_manifest(); | ||
ECCVMProof proof = prover.construct_proof(); | ||
|
||
// Check that the prover generated manifest agrees with the manifest hard coded in this suite | ||
auto manifest_expected = this->construct_eccvm_honk_manifest(); | ||
auto prover_manifest = prover.transcript->get_manifest(); | ||
|
||
// Note: a manifest can be printed using manifest.print() | ||
ASSERT(manifest_expected.size() > 0); | ||
for (size_t round = 0; round < manifest_expected.size(); ++round) { | ||
ASSERT_EQ(prover_manifest[round], manifest_expected[round]) << "Prover manifest discrepency in round " << round; | ||
} | ||
|
@@ -293,6 +296,7 @@ TEST_F(ECCVMTranscriptTests, ProverManifestConsistency) | |
auto prover_ipa_manifest = prover.ipa_transcript->get_manifest(); | ||
|
||
// Note: a manifest can be printed using manifest.print() | ||
ASSERT(ipa_manifest_expected.size() > 0); | ||
for (size_t round = 0; round < ipa_manifest_expected.size(); ++round) { | ||
ASSERT_EQ(prover_ipa_manifest[round], ipa_manifest_expected[round]) | ||
<< "IPA prover manifest discrepency in round " << round; | ||
|
@@ -311,6 +315,8 @@ TEST_F(ECCVMTranscriptTests, VerifierManifestConsistency) | |
|
||
// Automatically generate a transcript manifest in the prover by constructing a proof | ||
ECCVMProver prover(builder); | ||
prover.transcript->enable_manifest(); | ||
prover.ipa_transcript->enable_manifest(); | ||
ECCVMProof proof = prover.construct_proof(); | ||
|
||
// Automatically generate a transcript manifest in the verifier by verifying a proof | ||
|
@@ -325,10 +331,20 @@ TEST_F(ECCVMTranscriptTests, VerifierManifestConsistency) | |
// The last challenge generated by the ECCVM Prover is the translation univariate batching challenge and, on the | ||
// verifier side, is only generated in the translator verifier hence the ECCVM prover's manifest will have one extra | ||
// challenge | ||
ASSERT(prover_manifest.size() > 0); | ||
for (size_t round = 0; round < prover_manifest.size() - 1; ++round) { | ||
ASSERT_EQ(prover_manifest[round], verifier_manifest[round]) | ||
<< "Prover/Verifier manifest discrepency in round " << round; | ||
} | ||
|
||
// Check consistency of IPA transcripts | ||
auto prover_ipa_manifest = prover.ipa_transcript->get_manifest(); | ||
auto verifier_ipa_manifest = verifier.ipa_transcript->get_manifest(); | ||
ASSERT(prover_ipa_manifest.size() > 0); | ||
for (size_t round = 0; round < prover_ipa_manifest.size(); ++round) { | ||
ASSERT_EQ(prover_ipa_manifest[round], verifier_ipa_manifest[round]) | ||
<< "Prover/Verifier IPA manifest discrepency in round " << round; | ||
} | ||
} | ||
|
||
/** | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,6 +20,9 @@ bool ECCVMVerifier::verify_proof(const ECCVMProof& proof) | |
RelationParameters<FF> relation_parameters; | ||
transcript = std::make_shared<Transcript>(proof.pre_ipa_proof); | ||
ipa_transcript = std::make_shared<Transcript>(proof.ipa_proof); | ||
transcript->enable_manifest(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. just enable the manifests in the native/recursive verifiers since we shouldn't be having memory problems in those scenarios There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. its a little ugly that we have to add this though There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it doesnt seem too bad. I dont know, my sense was that the manifest was useful but if we find that it isnt maybe we'll eventually just kill it There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah its not horrible since we don't really use the manifest outside of the tests. If we ever want to test the manifest more, it shouldn't be too bad to add a few more enable_manifest()s since I'm pretty I'm missing a few in some verifiers where the manifest isn't tested. |
||
ipa_transcript->enable_manifest(); | ||
|
||
VerifierCommitments commitments{ key }; | ||
CommitmentLabels commitment_labels; | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -55,9 +55,11 @@ void TraceToPolynomials<Flavor>::add_memory_records_to_proving_key(TraceData& tr | |
ASSERT(proving_key.memory_read_records.empty() && proving_key.memory_write_records.empty()); | ||
|
||
// Update indices of RAM/ROM reads/writes based on where block containing these gates sits in the trace | ||
proving_key.memory_read_records.reserve(builder.memory_read_records.size()); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. reserve instead of just emplace_back since we know the size of this vector |
||
for (auto& index : builder.memory_read_records) { | ||
proving_key.memory_read_records.emplace_back(index + trace_data.ram_rom_offset); | ||
} | ||
proving_key.memory_write_records.reserve(builder.memory_write_records.size()); | ||
for (auto& index : builder.memory_write_records) { | ||
proving_key.memory_write_records.emplace_back(index + trace_data.ram_rom_offset); | ||
} | ||
|
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.
Can you add your comment about using a lambda to ensure the builder is freed to this usage? I think this is the only place its missed. Definitely good to have so someone doesn't undo it without realizing the effect