feat(avm): make ProverPolynomials::get_row return references #7419
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
fcarreiro
requested review from
jeanmon,
IlyasRidhuan and
Maddiaa0
as code owners
|
This stack of pull requests is managed by Graphite. Learn more about stacking. Join @fcarreiro and the rest of your teammates on |
fcarreiro
requested review from
ledwards2225 and
ludamad
and removed request for
jeanmon and
Maddiaa0
Changes to circuit sizes
🧾 Summary (100% most significant diffs)
Full diff report 👇
|
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. Proof generationEach column represents the number of threads used in proof generation.
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 8 txs.
Circuits statsStats on running time and I/O sizes collected for every kernel circuit run across all benchmarks.
Stats on running time collected for app circuits
AVM SimulationTime to simulate various public functions in the AVM.
Public DB AccessTime to access various public DBs.
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 | Metric | | |
Maddiaa0
approved these changes
Jul 10, 2024
Merge activity
|
rahul-kothari
pushed a commit
that referenced
this pull request
Jul 11, 2024
🤖 I have created a release *beep* *boop* --- <details><summary>aztec-package: 0.46.3</summary> ## [0.46.3](aztec-package-v0.46.2...aztec-package-v0.46.3) (2024-07-11) ### Miscellaneous * **aztec-package:** Synchronize aztec-packages versions </details> <details><summary>barretenberg.js: 0.46.3</summary> ## [0.46.3](barretenberg.js-v0.46.2...barretenberg.js-v0.46.3) (2024-07-11) ### Miscellaneous * **barretenberg.js:** Synchronize aztec-packages versions </details> <details><summary>aztec-packages: 0.46.3</summary> ## [0.46.3](aztec-packages-v0.46.2...aztec-packages-v0.46.3) (2024-07-11) ### Features * Add CLI argument for debugging comptime blocks (noir-lang/noir#5192) ([97ecff5](97ecff5)) * Add reset tiny and optimize tail ([#7422](#7422)) ([399917b](399917b)) * **avm:** Calldatacopy and return gadget ([#7415](#7415)) ([ec39e4e](ec39e4e)), closes [#7381](#7381) [#7211](#7211) * **avm:** Make ProverPolynomials::get_row return references ([#7419](#7419)) ([108fc5f](108fc5f)) * Integrate new proving systems in e2e ([#6971](#6971)) ([723a0c1](723a0c1)) * Lsp rename/find-all-references for struct members (noir-lang/noir#5443) ([97ecff5](97ecff5)) * MSM sorting ([#7351](#7351)) ([5cbdc54](5cbdc54)) * **optimization:** Deduplicate more instructions (noir-lang/noir#5457) ([97ecff5](97ecff5)) * Prefix operator overload trait dispatch (noir-lang/noir#5423) ([97ecff5](97ecff5)) * Remove proof from L1 Rollup process ([#7347](#7347)) ([2645eab](2645eab)), closes [#7346](#7346) * Remove ram tables in note_getter ([#7434](#7434)) ([fd67da3](fd67da3)) * Sync from aztec-packages (noir-lang/noir#5467) ([97ecff5](97ecff5)) * Typing return values of embedded_curve_ops ([#7413](#7413)) ([db96077](db96077)) ### Bug Fixes * **avm:** Fixes AVM full tests and decrease timeout to 35 minutes ([#7438](#7438)) ([2a7494b](2a7494b)) * Memory init with no other ops gate counting ([#7427](#7427)) ([e7177ba](e7177ba)) * Pass secrets to ci-arm.yml ([#7436](#7436)) ([619501d](619501d)) * Remove compile-time error for invalid indices (noir-lang/noir#5466) ([97ecff5](97ecff5)) * Using different generators in private refund ([#7414](#7414)) ([59b92ca](59b92ca)), closes [#7320](#7320) ### Miscellaneous * **bb:** Fix double increment ([#7428](#7428)) ([7870a58](7870a58)) * **boxes:** Adding an init command for an empty project ([#7398](#7398)) ([a6a605d](a6a605d)) * Bump bb to 0.45.1 (noir-lang/noir#5469) ([97ecff5](97ecff5)) * Disable flaky cheat code test ([7b8c2ba](7b8c2ba)) * Document EmbeddedCurvePoint (noir-lang/noir#5468) ([97ecff5](97ecff5)) * Minimize usage of get_row in inverse computation ([#7431](#7431)) ([f177887](f177887)) * Private refund cleanup ([#7403](#7403)) ([ebec8ff](ebec8ff)) * Replace relative paths to noir-protocol-circuits ([842f6d1](842f6d1)) * Unbundle `check_array_is_initialized` (noir-lang/noir#5451) ([97ecff5](97ecff5)) </details> <details><summary>barretenberg: 0.46.3</summary> ## [0.46.3](barretenberg-v0.46.2...barretenberg-v0.46.3) (2024-07-11) ### Features * **avm:** Calldatacopy and return gadget ([#7415](#7415)) ([ec39e4e](ec39e4e)), closes [#7381](#7381) [#7211](#7211) * **avm:** Make ProverPolynomials::get_row return references ([#7419](#7419)) ([108fc5f](108fc5f)) * Integrate new proving systems in e2e ([#6971](#6971)) ([723a0c1](723a0c1)) * MSM sorting ([#7351](#7351)) ([5cbdc54](5cbdc54)) ### Bug Fixes * **avm:** Fixes AVM full tests and decrease timeout to 35 minutes ([#7438](#7438)) ([2a7494b](2a7494b)) * Memory init with no other ops gate counting ([#7427](#7427)) ([e7177ba](e7177ba)) ### Miscellaneous * **bb:** Fix double increment ([#7428](#7428)) ([7870a58](7870a58)) * Minimize usage of get_row in inverse computation ([#7431](#7431)) ([f177887](f177887)) </details> --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
AztecBot
added a commit
to AztecProtocol/barretenberg
that referenced
this pull request
Jul 12, 2024
🤖 I have created a release *beep* *boop* --- <details><summary>aztec-package: 0.46.3</summary> ## [0.46.3](AztecProtocol/aztec-packages@aztec-package-v0.46.2...aztec-package-v0.46.3) (2024-07-11) ### Miscellaneous * **aztec-package:** Synchronize aztec-packages versions </details> <details><summary>barretenberg.js: 0.46.3</summary> ## [0.46.3](AztecProtocol/aztec-packages@barretenberg.js-v0.46.2...barretenberg.js-v0.46.3) (2024-07-11) ### Miscellaneous * **barretenberg.js:** Synchronize aztec-packages versions </details> <details><summary>aztec-packages: 0.46.3</summary> ## [0.46.3](AztecProtocol/aztec-packages@aztec-packages-v0.46.2...aztec-packages-v0.46.3) (2024-07-11) ### Features * Add CLI argument for debugging comptime blocks (noir-lang/noir#5192) ([97ecff5](AztecProtocol/aztec-packages@97ecff5)) * Add reset tiny and optimize tail ([#7422](AztecProtocol/aztec-packages#7422)) ([399917b](AztecProtocol/aztec-packages@399917b)) * **avm:** Calldatacopy and return gadget ([#7415](AztecProtocol/aztec-packages#7415)) ([ec39e4e](AztecProtocol/aztec-packages@ec39e4e)), closes [#7381](AztecProtocol/aztec-packages#7381) [#7211](AztecProtocol/aztec-packages#7211) * **avm:** Make ProverPolynomials::get_row return references ([#7419](AztecProtocol/aztec-packages#7419)) ([108fc5f](AztecProtocol/aztec-packages@108fc5f)) * Integrate new proving systems in e2e ([#6971](AztecProtocol/aztec-packages#6971)) ([723a0c1](AztecProtocol/aztec-packages@723a0c1)) * Lsp rename/find-all-references for struct members (noir-lang/noir#5443) ([97ecff5](AztecProtocol/aztec-packages@97ecff5)) * MSM sorting ([#7351](AztecProtocol/aztec-packages#7351)) ([5cbdc54](AztecProtocol/aztec-packages@5cbdc54)) * **optimization:** Deduplicate more instructions (noir-lang/noir#5457) ([97ecff5](AztecProtocol/aztec-packages@97ecff5)) * Prefix operator overload trait dispatch (noir-lang/noir#5423) ([97ecff5](AztecProtocol/aztec-packages@97ecff5)) * Remove proof from L1 Rollup process ([#7347](AztecProtocol/aztec-packages#7347)) ([2645eab](AztecProtocol/aztec-packages@2645eab)), closes [#7346](AztecProtocol/aztec-packages#7346) * Remove ram tables in note_getter ([#7434](AztecProtocol/aztec-packages#7434)) ([fd67da3](AztecProtocol/aztec-packages@fd67da3)) * Sync from aztec-packages (noir-lang/noir#5467) ([97ecff5](AztecProtocol/aztec-packages@97ecff5)) * Typing return values of embedded_curve_ops ([#7413](AztecProtocol/aztec-packages#7413)) ([db96077](AztecProtocol/aztec-packages@db96077)) ### Bug Fixes * **avm:** Fixes AVM full tests and decrease timeout to 35 minutes ([#7438](AztecProtocol/aztec-packages#7438)) ([2a7494b](AztecProtocol/aztec-packages@2a7494b)) * Memory init with no other ops gate counting ([#7427](AztecProtocol/aztec-packages#7427)) ([e7177ba](AztecProtocol/aztec-packages@e7177ba)) * Pass secrets to ci-arm.yml ([#7436](AztecProtocol/aztec-packages#7436)) ([619501d](AztecProtocol/aztec-packages@619501d)) * Remove compile-time error for invalid indices (noir-lang/noir#5466) ([97ecff5](AztecProtocol/aztec-packages@97ecff5)) * Using different generators in private refund ([#7414](AztecProtocol/aztec-packages#7414)) ([59b92ca](AztecProtocol/aztec-packages@59b92ca)), closes [#7320](AztecProtocol/aztec-packages#7320) ### Miscellaneous * **bb:** Fix double increment ([#7428](AztecProtocol/aztec-packages#7428)) ([7870a58](AztecProtocol/aztec-packages@7870a58)) * **boxes:** Adding an init command for an empty project ([#7398](AztecProtocol/aztec-packages#7398)) ([a6a605d](AztecProtocol/aztec-packages@a6a605d)) * Bump bb to 0.45.1 (noir-lang/noir#5469) ([97ecff5](AztecProtocol/aztec-packages@97ecff5)) * Disable flaky cheat code test ([7b8c2ba](AztecProtocol/aztec-packages@7b8c2ba)) * Document EmbeddedCurvePoint (noir-lang/noir#5468) ([97ecff5](AztecProtocol/aztec-packages@97ecff5)) * Minimize usage of get_row in inverse computation ([#7431](AztecProtocol/aztec-packages#7431)) ([f177887](AztecProtocol/aztec-packages@f177887)) * Private refund cleanup ([#7403](AztecProtocol/aztec-packages#7403)) ([ebec8ff](AztecProtocol/aztec-packages@ebec8ff)) * Replace relative paths to noir-protocol-circuits ([842f6d1](AztecProtocol/aztec-packages@842f6d1)) * Unbundle `check_array_is_initialized` (noir-lang/noir#5451) ([97ecff5](AztecProtocol/aztec-packages@97ecff5)) </details> <details><summary>barretenberg: 0.46.3</summary> ## [0.46.3](AztecProtocol/aztec-packages@barretenberg-v0.46.2...barretenberg-v0.46.3) (2024-07-11) ### Features * **avm:** Calldatacopy and return gadget ([#7415](AztecProtocol/aztec-packages#7415)) ([ec39e4e](AztecProtocol/aztec-packages@ec39e4e)), closes [#7381](AztecProtocol/aztec-packages#7381) [#7211](AztecProtocol/aztec-packages#7211) * **avm:** Make ProverPolynomials::get_row return references ([#7419](AztecProtocol/aztec-packages#7419)) ([108fc5f](AztecProtocol/aztec-packages@108fc5f)) * Integrate new proving systems in e2e ([#6971](AztecProtocol/aztec-packages#6971)) ([723a0c1](AztecProtocol/aztec-packages@723a0c1)) * MSM sorting ([#7351](AztecProtocol/aztec-packages#7351)) ([5cbdc54](AztecProtocol/aztec-packages@5cbdc54)) ### Bug Fixes * **avm:** Fixes AVM full tests and decrease timeout to 35 minutes ([#7438](AztecProtocol/aztec-packages#7438)) ([2a7494b](AztecProtocol/aztec-packages@2a7494b)) * Memory init with no other ops gate counting ([#7427](AztecProtocol/aztec-packages#7427)) ([e7177ba](AztecProtocol/aztec-packages@e7177ba)) ### Miscellaneous * **bb:** Fix double increment ([#7428](AztecProtocol/aztec-packages#7428)) ([7870a58](AztecProtocol/aztec-packages@7870a58)) * Minimize usage of get_row in inverse computation ([#7431](AztecProtocol/aztec-packages#7431)) ([f177887](AztecProtocol/aztec-packages@f177887)) </details> --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
fcarreiro
added a commit
that referenced
this pull request
Jan 29, 2025
Proving times (VM1) on 16 cores, 850+ columns, dozens of lookups, bulk_test. ``` ** Before ** prove/all_ms: 92606 prove/execute_log_derivative_inverse_round_ms: 21544 ** After ** prove/all_ms: 73404 prove/execute_log_derivative_inverse_round_ms: 839 ``` No change in sumcheck time. For reviewing, you can focus on the templates. An explanation follows (with history). --- This PR is about the `get_row()` method on the prover polynomials of a given flavor. This method is used by the [logderivative library](https://github.com/AztecProtocol/aztec-packages/blob/master/barretenberg/cpp/src/barretenberg/honk/proof_system/logderivative_library.hpp#L36) to compute logderivative inverses. Originally, `get_row()` was supposed to be debug only but it ended up used in the library above. To be fair, the reason is as follows: the `accumulate` function of relations (including lookups and perms), takes in a row (or something that looks like it!). However, by the time that you have to compute inverses, you don't have your row-based trace anymore, you only have the prover polynomials which are column-based. So, you need to extract a row from columns. The following sections explore a way to make things run faster, without completely breaking the `get_row()` expectations from the caller. That is, that it behaves like a row (you can do `.column` and it will return the field for it). # Phase 1: `AllEntities<FF>` So far so good. Normal [BB flavors](https://github.com/AztecProtocol/aztec-packages/blob/master/barretenberg/cpp/src/barretenberg/stdlib_circuit_builders/mega_flavor.hpp#L366) make `get_row()` return `AllEntities<FF>` which is literally a row with as many fields copied as columns you have. Note that the copy is done even for the columns that may not get used later in the accumulation of a relation, or in the computation of inverses. This might be ok if you have 10 columns and a handful of lookups, but in our case we have dozens of lookups and 850+ columns (we estimate 3500 by completion of the AVM). # Phase 2: something like `AllEntities<const FF&>` As a quick fix you might think you can copy references instead and use `AllEntities<const FF&>`. Well you can't, at least not the way you would use `AllEntities<FF>`. Since the class would have members that are references, you need to define a constructor that initializes them all, maybe from a `RefArray` of sorts. The problem is because the class `AllEntities` is defined as inheriting from other classes, instead of being "flat". This, for us, added an immense amount of codegen. See `AllConstRefValues` [here](https://github.com/AztecProtocol/aztec-packages/blob/2f05dc02fe7b147c7cd6fc235134279dbf332c08/barretenberg/cpp/src/barretenberg/vm/avm/generated/flavor.cpp). This improvement was introduced in [this PR](#7419) and it gave a **20x** speed improvement over `AllEntities<FF>`. The code itself was then improved in [this PR](#11504) by using a flat class and some fold expressions. # Phase 3: Getters Ideally what we'd want is for `get_row()` to return something like this: ``` template <typename Polynomials> class PolynomialEntitiesAtFixedRow { public: PolynomialEntitiesAtFixedRow(const size_t row_idx, const Polynomials& pp) : row_idx(row_idx) , pp(pp) {} // what here? private: const size_t row_idx; const Polynomials& pp; }; ``` such that if you do `row.column` it would secretly do `pp.column[row_idx]` instead. Unfortunately, you cannot override the `.` operator, and certainly not like this. Instead, we compromise. I added a macro to generate getters `_column()` for every column, which do exactly that. Then I changed the lookups and permutation codegen to use that (i.e., `in._column()` instead of `in.column`). Note that we _only_ use these getters in lookups and perm, not in the main relations. However, we are not done. The perms and lookups code that we changed is also called by `accumulate` when doing sumcheck, and `AllEntities` does not provide those getters so it will not compile. Well, we add them, and we are done. This results in a **25x** time improvement in calculating logderiv inverses, amounting to a total of **500x** better than baseline. # Conclusion Some thing in BB are not thought for a VM :) I wonder if theres any such improvement lurking in sumcheck? :)
AztecBot
pushed a commit
to AztecProtocol/barretenberg
that referenced
this pull request
Jan 30, 2025
Proving times (VM1) on 16 cores, 850+ columns, dozens of lookups, bulk_test. ``` ** Before ** prove/all_ms: 92606 prove/execute_log_derivative_inverse_round_ms: 21544 ** After ** prove/all_ms: 73404 prove/execute_log_derivative_inverse_round_ms: 839 ``` No change in sumcheck time. For reviewing, you can focus on the templates. An explanation follows (with history). --- This PR is about the `get_row()` method on the prover polynomials of a given flavor. This method is used by the [logderivative library](https://github.com/AztecProtocol/aztec-packages/blob/master/barretenberg/cpp/src/barretenberg/honk/proof_system/logderivative_library.hpp#L36) to compute logderivative inverses. Originally, `get_row()` was supposed to be debug only but it ended up used in the library above. To be fair, the reason is as follows: the `accumulate` function of relations (including lookups and perms), takes in a row (or something that looks like it!). However, by the time that you have to compute inverses, you don't have your row-based trace anymore, you only have the prover polynomials which are column-based. So, you need to extract a row from columns. The following sections explore a way to make things run faster, without completely breaking the `get_row()` expectations from the caller. That is, that it behaves like a row (you can do `.column` and it will return the field for it). # Phase 1: `AllEntities<FF>` So far so good. Normal [BB flavors](https://github.com/AztecProtocol/aztec-packages/blob/master/barretenberg/cpp/src/barretenberg/stdlib_circuit_builders/mega_flavor.hpp#L366) make `get_row()` return `AllEntities<FF>` which is literally a row with as many fields copied as columns you have. Note that the copy is done even for the columns that may not get used later in the accumulation of a relation, or in the computation of inverses. This might be ok if you have 10 columns and a handful of lookups, but in our case we have dozens of lookups and 850+ columns (we estimate 3500 by completion of the AVM). # Phase 2: something like `AllEntities<const FF&>` As a quick fix you might think you can copy references instead and use `AllEntities<const FF&>`. Well you can't, at least not the way you would use `AllEntities<FF>`. Since the class would have members that are references, you need to define a constructor that initializes them all, maybe from a `RefArray` of sorts. The problem is because the class `AllEntities` is defined as inheriting from other classes, instead of being "flat". This, for us, added an immense amount of codegen. See `AllConstRefValues` [here](https://github.com/AztecProtocol/aztec-packages/blob/2f05dc02fe7b147c7cd6fc235134279dbf332c08/barretenberg/cpp/src/barretenberg/vm/avm/generated/flavor.cpp). This improvement was introduced in [this PR](AztecProtocol/aztec-packages#7419) and it gave a **20x** speed improvement over `AllEntities<FF>`. The code itself was then improved in [this PR](AztecProtocol/aztec-packages#11504) by using a flat class and some fold expressions. # Phase 3: Getters Ideally what we'd want is for `get_row()` to return something like this: ``` template <typename Polynomials> class PolynomialEntitiesAtFixedRow { public: PolynomialEntitiesAtFixedRow(const size_t row_idx, const Polynomials& pp) : row_idx(row_idx) , pp(pp) {} // what here? private: const size_t row_idx; const Polynomials& pp; }; ``` such that if you do `row.column` it would secretly do `pp.column[row_idx]` instead. Unfortunately, you cannot override the `.` operator, and certainly not like this. Instead, we compromise. I added a macro to generate getters `_column()` for every column, which do exactly that. Then I changed the lookups and permutation codegen to use that (i.e., `in._column()` instead of `in.column`). Note that we _only_ use these getters in lookups and perm, not in the main relations. However, we are not done. The perms and lookups code that we changed is also called by `accumulate` when doing sumcheck, and `AllEntities` does not provide those getters so it will not compile. Well, we add them, and we are done. This results in a **25x** time improvement in calculating logderiv inverses, amounting to a total of **500x** better than baseline. # Conclusion Some thing in BB are not thought for a VM :) I wonder if theres any such improvement lurking in sumcheck? :)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This makes the computation of logderivatives ~20x faster, and proving a public token transfer goes from
60sto14s.The cost is verbosity.