-
Notifications
You must be signed in to change notification settings - Fork 297
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
refactor: bye bye shared ptrs for ultra/goblin ultra proving_keys :) #5407
Conversation
…packages into lx/clean-shared-ptr
Benchmark resultsMetrics with a significant change:
Detailed resultsAll benchmarks are run on txs on the This benchmark source data is available in JSON format on S3 here. Values are compared against data from master at commit L2 block published to L1Each column represents the number of txs on an L2 block published to L1.
L2 chain processingEach column represents the number of blocks on the L2 chain where each block has 16 txs.
Circuits statsStats on running time and I/O sizes collected for every circuit run across all benchmarks.
Tree insertion statsThe duration to insert a fixed batch of leaves into each tree type.
MiscellaneousTransaction sizes based on how many contract classes are registered in the tx.
Transaction size based on fee payment method
Transaction processing duration by data writes.
|
@@ -165,19 +165,6 @@ class VerificationKey_ : public PrecomputedCommitments { | |||
this->log_circuit_size = numeric::get_msb(circuit_size); | |||
this->num_public_inputs = num_public_inputs; | |||
}; | |||
|
|||
template <typename ProvingKeyPtr> VerificationKey_(const ProvingKeyPtr& proving_key) |
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.
this function with a provingkeyptr template is pretty bad
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.
Instead, I put them in the specific flavor classes
@@ -452,6 +471,7 @@ class GoblinUltraFlavor { | |||
*/ | |||
class ProverPolynomials : public AllEntities<Polynomial> { | |||
public: | |||
// TODO(https://github.com/AztecProtocol/barretenberg/issues/925), proving_key could be const ref |
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.
extra todo added from prev PR
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.
Approved conditional on responding to the comments, in particular to the one relating to reference type data member.
@@ -54,7 +54,7 @@ template <class Flavor> class ExecutionTrace_ { | |||
*/ | |||
static void add_wires_and_selectors_to_proving_key(TraceData& trace_data, | |||
Builder& builder, | |||
const std::shared_ptr<typename Flavor::ProvingKey>& proving_key); | |||
typename Flavor::ProvingKey& proving_key); |
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.
You can use the alias here rather than this heavier notation.
@@ -41,7 +41,7 @@ std::shared_ptr<plonk::proving_key> StandardComposer::compute_proving_key(Circui | |||
subgroup_size, circuit_constructor.public_inputs.size(), crs, CircuitType::STANDARD); | |||
|
|||
// Construct and add to proving key the wire, selector and copy constraint polynomials | |||
Trace::populate(circuit_constructor, circuit_proving_key); | |||
Trace::populate(circuit_constructor, *circuit_proving_key); |
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 guess it would have spiraled here to handle not through a pointer here? Seems reasonable to leave as a pointer in that case since Plonk is on its way out.
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.
indeed
@@ -43,7 +43,7 @@ template <IsUltraFlavor Flavor> class OinkProver { | |||
using FF = typename Flavor::FF; | |||
|
|||
public: | |||
std::shared_ptr<ProvingKey> proving_key; | |||
ProvingKey& proving_key; |
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.
Reference memmbers are the devil! Was 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.
Please change or try to convince me this is the way.
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.
you're right that this is not great. I think changing to unique_ptr might be better? I used a reference here because OinkProver's lifetime is basically in the call to prove().
@@ -26,6 +26,7 @@ template <IsUltraFlavor Flavor> OinkProverOutput<Flavor> OinkProver<Flavor>::pro | |||
execute_grand_product_computation_round(); | |||
|
|||
return OinkProverOutput<Flavor>{ | |||
.proving_key = std::move(proving_key), |
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.
not sure if this one is necessary too. Do we need to do 2 std::moves() in order to avoid copying when returning?
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.
Hm, my sense is it's better to have these though perhaps the compiler would already move? So yeah, also not sure.
🤖 I have created a release *beep* *boop* --- <details><summary>aztec-package: 0.32.1</summary> ## [0.32.1](aztec-package-v0.32.0...aztec-package-v0.32.1) (2024-04-02) ### Miscellaneous * Explicit type imports ([#5519](#5519)) ([2a217de](2a217de)) </details> <details><summary>barretenberg.js: 0.32.1</summary> ## [0.32.1](barretenberg.js-v0.32.0...barretenberg.js-v0.32.1) (2024-04-02) ### Features * Earthly split runners, structure reverts ([#5524](#5524)) ([fcb8787](fcb8787)) ### Bug Fixes * **ci:** Turn on earthly for everyone ([#5423](#5423)) ([bea3fcb](bea3fcb)) </details> <details><summary>aztec-cli: 0.32.1</summary> ## [0.32.1](aztec-cli-v0.32.0...aztec-cli-v0.32.1) (2024-04-02) ### Miscellaneous * Explicit type imports ([#5519](#5519)) ([2a217de](2a217de)) </details> <details><summary>aztec-packages: 0.32.1</summary> ## [0.32.1](aztec-packages-v0.32.0...aztec-packages-v0.32.1) (2024-04-02) ### Features * **acir_gen:** Fold attribute at compile-time and initial non inlined ACIR ([#5341](#5341)) ([a979150](a979150)) * **acvm:** Execute multiple circuits ([#5380](#5380)) ([bb71920](bb71920)) * Dont double check num bits in brillig vm ([#5489](#5489)) ([a18288d](a18288d)) * Earthly split runners, structure reverts ([#5524](#5524)) ([fcb8787](fcb8787)) * Parallel gtest ([#5498](#5498)) ([349ea59](349ea59)) ### Bug Fixes * **ci:** Turn on earthly for everyone ([#5423](#5423)) ([bea3fcb](bea3fcb)) * Cpp cache and add other e2e ([#5512](#5512)) ([4118bcd](4118bcd)) * Require noir-packages-test to finish ([#5505](#5505)) ([191f0df](191f0df)) * Univariate evals not set in ECCVM prover ([#5529](#5529)) ([f9a2b7c](f9a2b7c)) ### Miscellaneous * Add goblin ops in add_gates_to_ensure_all_polys_are_non_zero ([#5468](#5468)) ([b9041e4](b9041e4)) * **avm:** Add 15 additional 16-bit registers in ALU trace of AVM circuit ([#5503](#5503)) ([8725c39](8725c39)) * **avm:** Migrate memory data structure in AVM circuit to unordered map ([#5506](#5506)) ([ccd09aa](ccd09aa)) * Build contracts and protocol circuits sequentially if not enough ram ([#5499](#5499)) ([ea072b6](ea072b6)) * Bye bye shared ptrs for ultra/goblin ultra proving_keys :) ([#5407](#5407)) ([b94d0db](b94d0db)) * Clean up compute_next_accumulator ([#5516](#5516)) ([f9be2f2](f9be2f2)) * Explicit type imports ([#5519](#5519)) ([2a217de](2a217de)) * Improve caching in noir Earthfile ([#5513](#5513)) ([5d1fb44](5d1fb44)) * Inject fetcher instead of using global ([#5502](#5502)) ([a066544](a066544)) * Make get notes return all notes at beginning of array [#4991](#4991) ([#5321](#5321)) ([5c5b627](5c5b627)) * Move alphas generation to oink ([#5515](#5515)) ([3b964f3](3b964f3)) * Replace relative paths to noir-protocol-circuits ([a689e4e](a689e4e)) * Replace relative paths to noir-protocol-circuits ([db1bab5](db1bab5)) * Replace relative paths to noir-protocol-circuits ([b2ab64b](b2ab64b)) * Replace relative paths to noir-protocol-circuits ([1f468db](1f468db)) * Run nargo format for noir-projects ([#5483](#5483)) ([277168f](277168f)) </details> <details><summary>barretenberg: 0.32.1</summary> ## [0.32.1](barretenberg-v0.32.0...barretenberg-v0.32.1) (2024-04-02) ### Features * **acvm:** Execute multiple circuits ([#5380](#5380)) ([bb71920](bb71920)) * Earthly split runners, structure reverts ([#5524](#5524)) ([fcb8787](fcb8787)) * Parallel gtest ([#5498](#5498)) ([349ea59](349ea59)) ### Bug Fixes * **ci:** Turn on earthly for everyone ([#5423](#5423)) ([bea3fcb](bea3fcb)) * Cpp cache and add other e2e ([#5512](#5512)) ([4118bcd](4118bcd)) * Univariate evals not set in ECCVM prover ([#5529](#5529)) ([f9a2b7c](f9a2b7c)) ### Miscellaneous * Add goblin ops in add_gates_to_ensure_all_polys_are_non_zero ([#5468](#5468)) ([b9041e4](b9041e4)) * **avm:** Add 15 additional 16-bit registers in ALU trace of AVM circuit ([#5503](#5503)) ([8725c39](8725c39)) * **avm:** Migrate memory data structure in AVM circuit to unordered map ([#5506](#5506)) ([ccd09aa](ccd09aa)) * Bye bye shared ptrs for ultra/goblin ultra proving_keys :) ([#5407](#5407)) ([b94d0db](b94d0db)) * Clean up compute_next_accumulator ([#5516](#5516)) ([f9be2f2](f9be2f2)) * Move alphas generation to oink ([#5515](#5515)) ([3b964f3](3b964f3)) </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.32.1</summary> ## [0.32.1](AztecProtocol/aztec-packages@aztec-package-v0.32.0...aztec-package-v0.32.1) (2024-04-02) ### Miscellaneous * Explicit type imports ([#5519](AztecProtocol/aztec-packages#5519)) ([2a217de](AztecProtocol/aztec-packages@2a217de)) </details> <details><summary>barretenberg.js: 0.32.1</summary> ## [0.32.1](AztecProtocol/aztec-packages@barretenberg.js-v0.32.0...barretenberg.js-v0.32.1) (2024-04-02) ### Features * Earthly split runners, structure reverts ([#5524](AztecProtocol/aztec-packages#5524)) ([fcb8787](AztecProtocol/aztec-packages@fcb8787)) ### Bug Fixes * **ci:** Turn on earthly for everyone ([#5423](AztecProtocol/aztec-packages#5423)) ([bea3fcb](AztecProtocol/aztec-packages@bea3fcb)) </details> <details><summary>aztec-cli: 0.32.1</summary> ## [0.32.1](AztecProtocol/aztec-packages@aztec-cli-v0.32.0...aztec-cli-v0.32.1) (2024-04-02) ### Miscellaneous * Explicit type imports ([#5519](AztecProtocol/aztec-packages#5519)) ([2a217de](AztecProtocol/aztec-packages@2a217de)) </details> <details><summary>aztec-packages: 0.32.1</summary> ## [0.32.1](AztecProtocol/aztec-packages@aztec-packages-v0.32.0...aztec-packages-v0.32.1) (2024-04-02) ### Features * **acir_gen:** Fold attribute at compile-time and initial non inlined ACIR ([#5341](AztecProtocol/aztec-packages#5341)) ([a979150](AztecProtocol/aztec-packages@a979150)) * **acvm:** Execute multiple circuits ([#5380](AztecProtocol/aztec-packages#5380)) ([bb71920](AztecProtocol/aztec-packages@bb71920)) * Dont double check num bits in brillig vm ([#5489](AztecProtocol/aztec-packages#5489)) ([a18288d](AztecProtocol/aztec-packages@a18288d)) * Earthly split runners, structure reverts ([#5524](AztecProtocol/aztec-packages#5524)) ([fcb8787](AztecProtocol/aztec-packages@fcb8787)) * Parallel gtest ([#5498](AztecProtocol/aztec-packages#5498)) ([349ea59](AztecProtocol/aztec-packages@349ea59)) ### Bug Fixes * **ci:** Turn on earthly for everyone ([#5423](AztecProtocol/aztec-packages#5423)) ([bea3fcb](AztecProtocol/aztec-packages@bea3fcb)) * Cpp cache and add other e2e ([#5512](AztecProtocol/aztec-packages#5512)) ([4118bcd](AztecProtocol/aztec-packages@4118bcd)) * Require noir-packages-test to finish ([#5505](AztecProtocol/aztec-packages#5505)) ([191f0df](AztecProtocol/aztec-packages@191f0df)) * Univariate evals not set in ECCVM prover ([#5529](AztecProtocol/aztec-packages#5529)) ([f9a2b7c](AztecProtocol/aztec-packages@f9a2b7c)) ### Miscellaneous * Add goblin ops in add_gates_to_ensure_all_polys_are_non_zero ([#5468](AztecProtocol/aztec-packages#5468)) ([b9041e4](AztecProtocol/aztec-packages@b9041e4)) * **avm:** Add 15 additional 16-bit registers in ALU trace of AVM circuit ([#5503](AztecProtocol/aztec-packages#5503)) ([8725c39](AztecProtocol/aztec-packages@8725c39)) * **avm:** Migrate memory data structure in AVM circuit to unordered map ([#5506](AztecProtocol/aztec-packages#5506)) ([ccd09aa](AztecProtocol/aztec-packages@ccd09aa)) * Build contracts and protocol circuits sequentially if not enough ram ([#5499](AztecProtocol/aztec-packages#5499)) ([ea072b6](AztecProtocol/aztec-packages@ea072b6)) * Bye bye shared ptrs for ultra/goblin ultra proving_keys :) ([#5407](AztecProtocol/aztec-packages#5407)) ([b94d0db](AztecProtocol/aztec-packages@b94d0db)) * Clean up compute_next_accumulator ([#5516](AztecProtocol/aztec-packages#5516)) ([f9be2f2](AztecProtocol/aztec-packages@f9be2f2)) * Explicit type imports ([#5519](AztecProtocol/aztec-packages#5519)) ([2a217de](AztecProtocol/aztec-packages@2a217de)) * Improve caching in noir Earthfile ([#5513](AztecProtocol/aztec-packages#5513)) ([5d1fb44](AztecProtocol/aztec-packages@5d1fb44)) * Inject fetcher instead of using global ([#5502](AztecProtocol/aztec-packages#5502)) ([a066544](AztecProtocol/aztec-packages@a066544)) * Make get notes return all notes at beginning of array [#4991](AztecProtocol/aztec-packages#4991) ([#5321](AztecProtocol/aztec-packages#5321)) ([5c5b627](AztecProtocol/aztec-packages@5c5b627)) * Move alphas generation to oink ([#5515](AztecProtocol/aztec-packages#5515)) ([3b964f3](AztecProtocol/aztec-packages@3b964f3)) * Replace relative paths to noir-protocol-circuits ([a689e4e](AztecProtocol/aztec-packages@a689e4e)) * Replace relative paths to noir-protocol-circuits ([db1bab5](AztecProtocol/aztec-packages@db1bab5)) * Replace relative paths to noir-protocol-circuits ([b2ab64b](AztecProtocol/aztec-packages@b2ab64b)) * Replace relative paths to noir-protocol-circuits ([1f468db](AztecProtocol/aztec-packages@1f468db)) * Run nargo format for noir-projects ([#5483](AztecProtocol/aztec-packages#5483)) ([277168f](AztecProtocol/aztec-packages@277168f)) </details> <details><summary>barretenberg: 0.32.1</summary> ## [0.32.1](AztecProtocol/aztec-packages@barretenberg-v0.32.0...barretenberg-v0.32.1) (2024-04-02) ### Features * **acvm:** Execute multiple circuits ([#5380](AztecProtocol/aztec-packages#5380)) ([bb71920](AztecProtocol/aztec-packages@bb71920)) * Earthly split runners, structure reverts ([#5524](AztecProtocol/aztec-packages#5524)) ([fcb8787](AztecProtocol/aztec-packages@fcb8787)) * Parallel gtest ([#5498](AztecProtocol/aztec-packages#5498)) ([349ea59](AztecProtocol/aztec-packages@349ea59)) ### Bug Fixes * **ci:** Turn on earthly for everyone ([#5423](AztecProtocol/aztec-packages#5423)) ([bea3fcb](AztecProtocol/aztec-packages@bea3fcb)) * Cpp cache and add other e2e ([#5512](AztecProtocol/aztec-packages#5512)) ([4118bcd](AztecProtocol/aztec-packages@4118bcd)) * Univariate evals not set in ECCVM prover ([#5529](AztecProtocol/aztec-packages#5529)) ([f9a2b7c](AztecProtocol/aztec-packages@f9a2b7c)) ### Miscellaneous * Add goblin ops in add_gates_to_ensure_all_polys_are_non_zero ([#5468](AztecProtocol/aztec-packages#5468)) ([b9041e4](AztecProtocol/aztec-packages@b9041e4)) * **avm:** Add 15 additional 16-bit registers in ALU trace of AVM circuit ([#5503](AztecProtocol/aztec-packages#5503)) ([8725c39](AztecProtocol/aztec-packages@8725c39)) * **avm:** Migrate memory data structure in AVM circuit to unordered map ([#5506](AztecProtocol/aztec-packages#5506)) ([ccd09aa](AztecProtocol/aztec-packages@ccd09aa)) * Bye bye shared ptrs for ultra/goblin ultra proving_keys :) ([#5407](AztecProtocol/aztec-packages#5407)) ([b94d0db](AztecProtocol/aztec-packages@b94d0db)) * Clean up compute_next_accumulator ([#5516](AztecProtocol/aztec-packages#5516)) ([f9be2f2](AztecProtocol/aztec-packages@f9be2f2)) * Move alphas generation to oink ([#5515](AztecProtocol/aztec-packages#5515)) ([3b964f3](AztecProtocol/aztec-packages@3b964f3)) </details> --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
Partially addresses #4885.
Removes shared_ptr usage for ultra/goblin_ultra proving key.
Shouldn't affect the benchmarks much, but for some reason it seems to have decreased slightly.
Please read contributing guidelines and remove this line.