-
Notifications
You must be signed in to change notification settings - Fork 324
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
Conversation
…e native and recursive verifiers
barretenberg/ts/src/main.ts
Outdated
// TODO(https://github.com/AztecProtocol/barretenberg/issues/1126): use specific UltraHonk function | ||
const circuitSize = await getGatesUltra(bytecodePath, recursive, /*honkRecursion=*/ true, api); | ||
// const circuitSize = 864047; |
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 delete
@@ -59,10 +59,10 @@ template <IsUltraFlavor Flavor> void OinkProver<Flavor>::prove() | |||
// Generate relation separators alphas for sumcheck/combiner computation | |||
proving_key->alphas = generate_alphas_round(); | |||
|
|||
#ifndef __wasm__ | |||
// #ifndef __wasm__ |
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 delete
@@ -311,6 +311,15 @@ WASM_EXPORT void acir_verify_aztec_client(uint8_t const* proof_buf, uint8_t cons | |||
*result = ClientIVC::verify(proof, vk); | |||
} | |||
|
|||
template <typename Flavor> | |||
UltraProver_<Flavor> construct_prover(acir_format::AcirProgram& program, const acir_format::ProgramMetadata& metadata) |
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.
new function to facilitate builder deallocation before proving
@@ -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 comment
The reason will be displayed to describe this comment to others. Learn more.
explicitly enable prover manifest for this test
@@ -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 comment
The 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 comment
The 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 comment
The 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 comment
The 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.
@@ -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 comment
The 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
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.
LGTM. Minor requests
// 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 }; | ||
auto prover = [&] { |
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
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 comment
The 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 comment
The 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).
@@ -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 comment
The 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
🤖 I have created a release *beep* *boop* --- <details><summary>aztec-package: 0.76.3</summary> ## [0.76.3](aztec-package-v0.76.2...aztec-package-v0.76.3) (2025-02-12) ### Features * Add undici ([#11818](#11818)) ([8503c7a](8503c7a)) * Initial multi-proof test ([#11779](#11779)) ([f54db75](f54db75)) </details> <details><summary>barretenberg.js: 0.76.3</summary> ## [0.76.3](barretenberg.js-v0.76.2...barretenberg.js-v0.76.3) (2025-02-12) ### Features * **perf:** Speed up construction of bbjs Frs & cache zero hashes in ephemeral trees (redo) ([#11894](#11894)) ([e093acf](e093acf)) ### Bug Fixes * Memory fragmentation fixes to cut UltraHonk memory usage by 26% ([#11895](#11895)) ([b4e2264](b4e2264)) </details> <details><summary>aztec-packages: 0.76.3</summary> ## [0.76.3](aztec-packages-v0.76.2...aztec-packages-v0.76.3) (2025-02-12) ### Features * Add undici ([#11818](#11818)) ([8503c7a](8503c7a)) * **avm:** Sequential lookup resolution ([#11769](#11769)) ([3980f6c](3980f6c)) * Enable ws for reth on devnet ([#11922](#11922)) ([7124664](7124664)), closes [#11921](#11921) * Initial multi-proof test ([#11779](#11779)) ([f54db75](f54db75)) * Native world state now supports checkpointing ([#11739](#11739)) ([6464059](6464059)) * **perf:** Speed up construction of bbjs Frs & cache zero hashes in ephemeral trees (redo) ([#11894](#11894)) ([e093acf](e093acf)) ### Bug Fixes * Correctly configure batch queue ([#11934](#11934)) ([4908df8](4908df8)) * De-dup pubkey conversion of cli-wallet param ([#11948](#11948)) ([5529871](5529871)) * **docs:** Update the token bridge tutorial ([#11578](#11578)) ([aaf42a7](aaf42a7)) * Don't restart kind control plane automatically ([#11923](#11923)) ([c23c0f9](c23c0f9)) * Empty blocks can now be unwound ([#11920](#11920)) ([fdc2042](fdc2042)) * Gcloud logs ([#11944](#11944)) ([c53b1c5](c53b1c5)), closes [#11887](#11887) * Lmdb cmake race condition ([#11959](#11959)) ([031200d](031200d)) * Memory fragmentation fixes to cut UltraHonk memory usage by 26% ([#11895](#11895)) ([b4e2264](b4e2264)) * Npm packages noir resolution ([#11946](#11946)) ([d3e3f20](d3e3f20)) * Set resource limits on Loki pods ([#11940](#11940)) ([6999982](6999982)) ### Miscellaneous * Only take FF (and not Flavor) in compute_logderivative_inverse ([#11938](#11938)) ([bbbded3](bbbded3)) * Op queue cleanup ([#11925](#11925)) ([082ed66](082ed66)) * Renaming `pxe_db.nr` as `capsules.nr` ([#11905](#11905)) ([14d873c](14d873c)) * Replace relative paths to noir-protocol-circuits ([9ce401a](9ce401a)) * Use realistic size Client IVC proofs during simulations ([#11692](#11692)) ([90b9fbf](90b9fbf)) * Use RelationChecker in relation correctness tests and add Translator interleaving test ([#11878](#11878)) ([ed215e8](ed215e8)) </details> <details><summary>barretenberg: 0.76.3</summary> ## [0.76.3](barretenberg-v0.76.2...barretenberg-v0.76.3) (2025-02-12) ### Features * **avm:** Sequential lookup resolution ([#11769](#11769)) ([3980f6c](3980f6c)) * Native world state now supports checkpointing ([#11739](#11739)) ([6464059](6464059)) ### Bug Fixes * Empty blocks can now be unwound ([#11920](#11920)) ([fdc2042](fdc2042)) * Lmdb cmake race condition ([#11959](#11959)) ([031200d](031200d)) * Memory fragmentation fixes to cut UltraHonk memory usage by 26% ([#11895](#11895)) ([b4e2264](b4e2264)) ### Miscellaneous * Only take FF (and not Flavor) in compute_logderivative_inverse ([#11938](#11938)) ([bbbded3](bbbded3)) * Op queue cleanup ([#11925](#11925)) ([082ed66](082ed66)) * Use RelationChecker in relation correctness tests and add Translator interleaving test ([#11878](#11878)) ([ed215e8](ed215e8)) </details> --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
🤖 I have created a release *beep* *boop* --- <details><summary>aztec-package: 0.76.3</summary> ## [0.76.3](AztecProtocol/aztec-packages@aztec-package-v0.76.2...aztec-package-v0.76.3) (2025-02-12) ### Features * Add undici ([#11818](AztecProtocol/aztec-packages#11818)) ([8503c7a](AztecProtocol/aztec-packages@8503c7a)) * Initial multi-proof test ([#11779](AztecProtocol/aztec-packages#11779)) ([f54db75](AztecProtocol/aztec-packages@f54db75)) </details> <details><summary>barretenberg.js: 0.76.3</summary> ## [0.76.3](AztecProtocol/aztec-packages@barretenberg.js-v0.76.2...barretenberg.js-v0.76.3) (2025-02-12) ### Features * **perf:** Speed up construction of bbjs Frs & cache zero hashes in ephemeral trees (redo) ([#11894](AztecProtocol/aztec-packages#11894)) ([e093acf](AztecProtocol/aztec-packages@e093acf)) ### Bug Fixes * Memory fragmentation fixes to cut UltraHonk memory usage by 26% ([#11895](AztecProtocol/aztec-packages#11895)) ([b4e2264](AztecProtocol/aztec-packages@b4e2264)) </details> <details><summary>aztec-packages: 0.76.3</summary> ## [0.76.3](AztecProtocol/aztec-packages@aztec-packages-v0.76.2...aztec-packages-v0.76.3) (2025-02-12) ### Features * Add undici ([#11818](AztecProtocol/aztec-packages#11818)) ([8503c7a](AztecProtocol/aztec-packages@8503c7a)) * **avm:** Sequential lookup resolution ([#11769](AztecProtocol/aztec-packages#11769)) ([3980f6c](AztecProtocol/aztec-packages@3980f6c)) * Enable ws for reth on devnet ([#11922](AztecProtocol/aztec-packages#11922)) ([7124664](AztecProtocol/aztec-packages@7124664)), closes [#11921](AztecProtocol/aztec-packages#11921) * Initial multi-proof test ([#11779](AztecProtocol/aztec-packages#11779)) ([f54db75](AztecProtocol/aztec-packages@f54db75)) * Native world state now supports checkpointing ([#11739](AztecProtocol/aztec-packages#11739)) ([6464059](AztecProtocol/aztec-packages@6464059)) * **perf:** Speed up construction of bbjs Frs & cache zero hashes in ephemeral trees (redo) ([#11894](AztecProtocol/aztec-packages#11894)) ([e093acf](AztecProtocol/aztec-packages@e093acf)) ### Bug Fixes * Correctly configure batch queue ([#11934](AztecProtocol/aztec-packages#11934)) ([4908df8](AztecProtocol/aztec-packages@4908df8)) * De-dup pubkey conversion of cli-wallet param ([#11948](AztecProtocol/aztec-packages#11948)) ([5529871](AztecProtocol/aztec-packages@5529871)) * **docs:** Update the token bridge tutorial ([#11578](AztecProtocol/aztec-packages#11578)) ([aaf42a7](AztecProtocol/aztec-packages@aaf42a7)) * Don't restart kind control plane automatically ([#11923](AztecProtocol/aztec-packages#11923)) ([c23c0f9](AztecProtocol/aztec-packages@c23c0f9)) * Empty blocks can now be unwound ([#11920](AztecProtocol/aztec-packages#11920)) ([fdc2042](AztecProtocol/aztec-packages@fdc2042)) * Gcloud logs ([#11944](AztecProtocol/aztec-packages#11944)) ([c53b1c5](AztecProtocol/aztec-packages@c53b1c5)), closes [#11887](AztecProtocol/aztec-packages#11887) * Lmdb cmake race condition ([#11959](AztecProtocol/aztec-packages#11959)) ([031200d](AztecProtocol/aztec-packages@031200d)) * Memory fragmentation fixes to cut UltraHonk memory usage by 26% ([#11895](AztecProtocol/aztec-packages#11895)) ([b4e2264](AztecProtocol/aztec-packages@b4e2264)) * Npm packages noir resolution ([#11946](AztecProtocol/aztec-packages#11946)) ([d3e3f20](AztecProtocol/aztec-packages@d3e3f20)) * Set resource limits on Loki pods ([#11940](AztecProtocol/aztec-packages#11940)) ([6999982](AztecProtocol/aztec-packages@6999982)) ### Miscellaneous * Only take FF (and not Flavor) in compute_logderivative_inverse ([#11938](AztecProtocol/aztec-packages#11938)) ([bbbded3](AztecProtocol/aztec-packages@bbbded3)) * Op queue cleanup ([#11925](AztecProtocol/aztec-packages#11925)) ([082ed66](AztecProtocol/aztec-packages@082ed66)) * Renaming `pxe_db.nr` as `capsules.nr` ([#11905](AztecProtocol/aztec-packages#11905)) ([14d873c](AztecProtocol/aztec-packages@14d873c)) * Replace relative paths to noir-protocol-circuits ([9ce401a](AztecProtocol/aztec-packages@9ce401a)) * Use realistic size Client IVC proofs during simulations ([#11692](AztecProtocol/aztec-packages#11692)) ([90b9fbf](AztecProtocol/aztec-packages@90b9fbf)) * Use RelationChecker in relation correctness tests and add Translator interleaving test ([#11878](AztecProtocol/aztec-packages#11878)) ([ed215e8](AztecProtocol/aztec-packages@ed215e8)) </details> <details><summary>barretenberg: 0.76.3</summary> ## [0.76.3](AztecProtocol/aztec-packages@barretenberg-v0.76.2...barretenberg-v0.76.3) (2025-02-12) ### Features * **avm:** Sequential lookup resolution ([#11769](AztecProtocol/aztec-packages#11769)) ([3980f6c](AztecProtocol/aztec-packages@3980f6c)) * Native world state now supports checkpointing ([#11739](AztecProtocol/aztec-packages#11739)) ([6464059](AztecProtocol/aztec-packages@6464059)) ### Bug Fixes * Empty blocks can now be unwound ([#11920](AztecProtocol/aztec-packages#11920)) ([fdc2042](AztecProtocol/aztec-packages@fdc2042)) * Lmdb cmake race condition ([#11959](AztecProtocol/aztec-packages#11959)) ([031200d](AztecProtocol/aztec-packages@031200d)) * Memory fragmentation fixes to cut UltraHonk memory usage by 26% ([#11895](AztecProtocol/aztec-packages#11895)) ([b4e2264](AztecProtocol/aztec-packages@b4e2264)) ### Miscellaneous * Only take FF (and not Flavor) in compute_logderivative_inverse ([#11938](AztecProtocol/aztec-packages#11938)) ([bbbded3](AztecProtocol/aztec-packages@bbbded3)) * Op queue cleanup ([#11925](AztecProtocol/aztec-packages#11925)) ([082ed66](AztecProtocol/aztec-packages@082ed66)) * Use RelationChecker in relation correctness tests and add Translator interleaving test ([#11878](AztecProtocol/aztec-packages#11878)) ([ed215e8](AztecProtocol/aztec-packages@ed215e8)) </details> --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
prove_ultra_honk on the verify_honk_proof circuit goes from 3059.63MiB to 2251.31MiB, a decrease of 26%. This gets us close to the memory reported by tracy, 2081MiB, which doesn't account for any fragmentation issues.
The fix hinges on a couple key issues: we want to deallocate large objects when we don't need them anymore and we need to be careful with our vector usage.
First, we should deallocate the builder after the prover is constructed and before we call construct_proof, and we should also deallocate the commitment_key during sumcheck since we do not need to commit to any polynomials during that phase.
Second, this deallocation of the commitment key does not actually help memory that much, in large part due to fragmentation. I discovered that our usage of the manifest, which uses vectors for each round data, caused tiny vectors to be littered across memory, often breaking up what otherwise would be a large contiguous block of memory.
With this in mind, we now only use the prover manifest when we specify that we want the manifest specifically, i.e. for the manifest tests. The native and recursive verifier manifests will be enabled for now.