Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat(bb): consider polynomial end_index when constructing partially evaluated multivariates #12530

Merged
merged 5 commits into from
Mar 10, 2025
Merged

feat(bb): consider polynomial end_index when constructing partially evaluated multivariates #12530

merged 5 commits into from
Mar 10, 2025

Conversation

fcarreiro
Copy link
Contributor

@fcarreiro fcarreiro commented Mar 6, 2025 •
edited
Loading

This fixes the sumcheck memory peak.

Takes AVM memory usage for a trivial transaction from 33GB (projected 100GB on full VM) to 5GB.

Hopefully also useful for other flavors, when the trace is not full.

This also improves sumcheck performance (thanks @jeanmon and @lucasxia01 for the suggestions on this)

  • AVM (16 cores, short trace) - Before: 28s
  • AVM (16 cores, short trace) - After: 16s

@fcarreiro Graphite App
Copy link
Contributor Author

This stack of pull requests is managed by Graphite. Learn more about stacking.

@fcarreiro fcarreiro force-pushed the fc/bb-sumcheck-memory branch from 164245b to 77cd95d Compare March 6, 2025 15:03
@fcarreiro fcarreiro force-pushed the fc/bb-sumcheck-memory branch from 77cd95d to caf6579 Compare March 6, 2025 15:05
fcarreiro
fcarreiro commented Mar 6, 2025
View reviewed changes
barretenberg/cpp/src/barretenberg/eccvm/eccvm_flavor.hpp
@@ -714,6 +698,30 @@ class ECCVMFlavor {
}
};

/**
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just moved because this class refers to ProverPolynomials.

@fcarreiro fcarreiro force-pushed the fc/bb-sumcheck-memory branch from caf6579 to 90bed9f Compare March 6, 2025 15:49
@fcarreiro fcarreiro force-pushed the fc/bb-sumcheck-memory branch from 90bed9f to 4c353b0 Compare March 6, 2025 16:35
lucasxia01
lucasxia01 reviewed Mar 6, 2025
View reviewed changes
barretenberg/cpp/src/barretenberg/sumcheck/partial_evaluation.test.cpp
@@ -65,6 +65,7 @@ TYPED_TEST(PartialEvaluationTests, TwoRoundsSpecial)
FF expected_lo = v00 * (FF(1) - round_challenge_0) + v10 * round_challenge_0;
FF expected_hi = v01 * (FF(1) - round_challenge_0) + v11 * round_challenge_0;

sumcheck.partially_evaluated_polynomials = typename Flavor::PartiallyEvaluatedMultivariates(multivariate_n);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why reassign? should probably just change the constructor in sumcheck itself

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh I see, that constructor was just removed and moved to the prove() function

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah this test itself is not very nice. It tests "partially_evaluate" on itself, but that's not how the usual flow goes.

lucasxia01
lucasxia01 reviewed Mar 6, 2025
View reviewed changes
Copy link
Contributor

@lucasxia01 lucasxia01 left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for taking this on; the memory savings are fantastic. There's still a little room for optimization I believe. You actually allocate up to double the memory necessary. I also realize that a lot of these changes were made with the assumption that the polynomials are nonzero starting from 0, rather than some arbitrary start_index. That's fine though, we can keep it like that for now since that's the main use case for this optimization.

@fcarreiro fcarreiro requested a review from lucasxia01 March 6, 2025 17:54
@fcarreiro fcarreiro force-pushed the fc/bb-sumcheck-memory branch from 946e1f3 to f8f25c7 Compare March 6, 2025 17:55
lucasxia01
lucasxia01 reviewed Mar 6, 2025
View reviewed changes
barretenberg/cpp/src/barretenberg/eccvm/eccvm_flavor.hpp Outdated
for (auto [poly, full_poly] : zip_view(get_all(), full_polynomials.get_all())) {
size_t desired_size = std::min(full_poly.end_index(), halved_circuit_size);
poly = Polynomial(desired_size, halved_circuit_size);
poly = Polynomial(full_poly.end_index() / 2, circuit_size / 2);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looking at this again, I would be wary of off-by-issues. I think if the end_index was odd like it was 7, then, we would actually want the poly size to be 4, not 3.

@fcarreiro fcarreiro force-pushed the fc/bb-sumcheck-memory branch 3 times, most recently from 20bf171 to 5d74c12 Compare March 6, 2025 18:23
@fcarreiro fcarreiro requested a review from lucasxia01 March 6, 2025 18:23
lucasxia01
lucasxia01 reviewed Mar 6, 2025
View reviewed changes
barretenberg/cpp/src/barretenberg/sumcheck/sumcheck.hpp Outdated
@@ -351,7 +354,7 @@ template <typename Flavor> class SumcheckProver {
transcript->template get_challenge<FF>("Sumcheck:u_" + std::to_string(round_idx));
multivariate_challenge.emplace_back(round_challenge);
// Prepare sumcheck book-keeping table for the next round
partially_evaluate(partially_evaluated_polynomials, round.round_size, round_challenge);
partially_evaluate(partially_evaluated_polynomials, round_idx, round_challenge);
Copy link
Contributor

@lucasxia01 lucasxia01 Mar 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there's confusingly 2 prove functions (and many other functions) - one for normal, one with zk. So both need to be updated similarly

lucasxia01
lucasxia01 reviewed Mar 6, 2025
View reviewed changes
barretenberg/cpp/src/barretenberg/sumcheck/sumcheck.hpp Outdated
pep_view[j].at(i >> 1) =
polynomials[j][i] + round_challenge * (polynomials[j][i + 1] - polynomials[j][i]);
const auto& poly = polynomials[j];
size_t actual_end_index = (poly.end_index() + 1) / (1 << (round_index - 1));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is the wrong partially_evaluate to update? No idea what PolynomialT is and where this is used...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this only used in the partially evaluation test? if so, maybe we can just get rid of this function...

lucasxia01
lucasxia01 reviewed Mar 6, 2025
View reviewed changes
barretenberg/cpp/src/barretenberg/numeric/bitop/division.hpp Outdated
@@ -0,0 +1,10 @@
#pragma once
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: not really a bitop but its fine

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

he yes I know, but didn't want to create a whole new directory. Give me a name for it and I'll create it :)

Copy link
Contributor

@jeanmon jeanmon Mar 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we create a function for this, I would like it to be "overflow/underflow" safe.
If we restrict such a function to work on "unsigned types" then one elegant solution is:

q = x/y + (x % y != 0); (Credit to https://stackoverflow.com/a/14878734)

Or for any signed type integer:

int div = a / b;
if (((a ^ b) >= 0) && (a % b != 0))
    div++;

(Credit to https://stackoverflow.com/a/924160)

@fcarreiro fcarreiro requested a review from lucasxia01 March 6, 2025 23:08
@fcarreiro fcarreiro force-pushed the fc/bb-sumcheck-memory branch from d947ebf to ab1c419 Compare March 7, 2025 17:09
@fcarreiro fcarreiro force-pushed the fc/bb-sumcheck-memory branch from ab1c419 to 89964cc Compare March 7, 2025 17:55
@fcarreiro fcarreiro changed the title feat(bb): consider structured polynomials when constructing partially evaluated multivariates feat(bb): consider polynomial end_index when constructing partially evaluated multivariates Mar 8, 2025
jeanmon
jeanmon approved these changes Mar 10, 2025
View reviewed changes
Copy link
Contributor

@jeanmon jeanmon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM and great improvement!

lucasxia01
lucasxia01 approved these changes Mar 10, 2025
View reviewed changes
Copy link
Contributor

@lucasxia01 lucasxia01 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Agree with Jean that the optimizations we were suggesting should be possible, but also agree that we can just get this in first.

barretenberg/cpp/src/barretenberg/sumcheck/sumcheck.hpp
// Prepare sumcheck book-keeping table for the next round
partially_evaluate(partially_evaluated_polynomials, round.round_size, round_challenge);
// Prepare sumcheck book-keeping table for the next round.
partially_evaluate(partially_evaluated_polynomials, multivariate_n >> round_idx, round_challenge);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why this change? to make it more clear what this is?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, it is kind of a leftover of many changes that I had and rolled back. However now that you mention it, it is kind of nice to use multivariate_n in round 0 and multivariate_n >> round_idx here. I'll keep it just because CI is green but feel free to change!

@lucasxia01
Copy link
Contributor

Partially handles AztecProtocol/barretenberg#1120, but maybe a followup will fully close it

@fcarreiro fcarreiro merged commit abd22cd into master Mar 10, 2025
8 checks passed
@fcarreiro fcarreiro deleted the fc/bb-sumcheck-memory branch March 10, 2025 16:03
@AztecBot AztecBot mentioned this pull request Mar 10, 2025
Merged
sklppy88 pushed a commit that referenced this pull request Mar 11, 2025
@AztecBot
chore(master): release 0.79.0 (#12578)
cb5d35f 
🤖 I have created a new Aztec Packages release
---


##
[0.79.0](v0.78.1...v0.79.0)
(2025-03-11)


### ⚠ BREAKING CHANGES

* aggregate data for batch calls
([#12562](#12562))

### Features

* add extra attributes to target_info
([#12583](#12583))
([c296422](c296422))
* add optional oracle resolver url in `acvm_cli`
(noir-lang/noir#7630)
([cc6cdbb](cc6cdbb))
* allow to pay via sponsored fpc from cli
([#12598](#12598))
([877de5c](877de5c))
* array concat method (noir-lang/noir#7199)
([cc6cdbb](cc6cdbb))
* **avm:** ToRadix gadget
([#12528](#12528))
([02a7171](02a7171))
* aztec-up -v flag
([#12590](#12590))
([6a41565](6a41565))
* **bb:** consider polynomial end_index when constructing partially
evaluated multivariates
([#12530](#12530))
([abd22cd](abd22cd))
* **config:** add fallbacks
([#12593](#12593))
([f2f9ef3](f2f9ef3))
* **p2p:** add trusted peers mechanics
([#12447](#12447))
([d67f7e8](d67f7e8))
* **p2p:** peer manager peer count metrics
([#12575](#12575))
([b4891c1](b4891c1))
* provision alerts
([#12561](#12561))
([2ea1767](2ea1767))
* Resolve callstacks in protocol circuit errors on wasm
([#12573](#12573))
([657299b](657299b))


### Bug Fixes

* aggregate data for batch calls
([#12562](#12562))
([bd0b3b6](bd0b3b6))
* broken kind transfer test
([#12611](#12611))
([6e91934](6e91934))
* Cl/release fixes 2
([#12595](#12595))
([fc597f4](fc597f4))
* Cl/release noir refs
([#12597](#12597))
([fdcfcaf](fdcfcaf))
* demote log
([#12626](#12626))
([bec8953](bec8953))
* deploy method test
([#12609](#12609))
([f2c06c2](f2c06c2))
* Do not report epoch as complete until blocks have synced
([#12638](#12638))
([2ddfa76](2ddfa76)),
closes
[#12625](#12625)
* Error on infinitely recursive types
(noir-lang/noir#7579)
([cc6cdbb](cc6cdbb))
* get L1 tx utils config from env
([#12620](#12620))
([d930c01](d930c01))
* Log overflow handling in reset
([#12579](#12579))
([283b624](283b624))
* metrics update
([#12571](#12571))
([80a5df2](80a5df2))
* **sandbox:** query release please manifest for version if in a docker
container
([#12591](#12591))
([db8ebc6](db8ebc6))
* **spartan:** setup needs kubectl
([#12580](#12580))
([753cb33](753cb33))
* update dead partial notes link
([#12629](#12629))
([5a1dc4c](5a1dc4c))
* update error message to display 128 bits as valid bit size
(noir-lang/noir#7626)
([cc6cdbb](cc6cdbb))
* update fallback transport
([#12470](#12470))
([88f0711](88f0711))


### Miscellaneous

* bump external pinned commits
(noir-lang/noir#7640)
([cc6cdbb](cc6cdbb))
* **ci3:** add helper for uncached test introspection
([#12618](#12618))
([9ac518b](9ac518b))
* **ci3:** better memsuspend_limit comment
([#12622](#12622))
([de84187](de84187))
* clean up upgrade test and other small things
([#12558](#12558))
([c28abe1](c28abe1))
* cleanup eth artifacts + misc aztec.js reorg
([#12563](#12563))
([6623244](6623244))
* **docs:** Updated accounts page
([#12019](#12019))
([d45dac9](d45dac9))
* Fix mac build
([#12610](#12610))
([adceed6](adceed6))
* gemini soundness regression test
([#12570](#12570))
([c654106](c654106))
* more sane e2e_prover/full timeout
([#12619](#12619))
([add9d35](add9d35))
* reactivate acir_test for `regression_5045`
([#12548](#12548))
([c89f89c](c89f89c))
* remove unnecessary trait bounds
(noir-lang/noir#7635)
([cc6cdbb](cc6cdbb))
* Rename `StructDefinition` to `TypeDefinition`
(noir-lang/noir#7614)
([cc6cdbb](cc6cdbb))
* replace relative paths to noir-protocol-circuits
([4f7f5c3](4f7f5c3))
* replace relative paths to noir-protocol-circuits
([0f68d11](0f68d11))
* replace relative paths to noir-protocol-circuits
([8f593ce](8f593ce))
* replace relative paths to noir-protocol-circuits
([251ae38](251ae38))
* rollup library cleanup
([#12621](#12621))
([361fc59](361fc59))
* **sandbox:** drop cheat-codes log level
([#12586](#12586))
([24f04c7](24f04c7))
* **sandbox:** expose anvil port
([#12599](#12599))
([955f1b0](955f1b0))
* **testnet:** updating script for ignition, change naming
([#12566](#12566))
([2d7b69d](2d7b69d))
* turn on masking in eccvm
([#12467](#12467))
([aacb91a](aacb91a))
* Update Bb line counting script
([#12350](#12350))
([7a41843](7a41843))
* update docs to reflect u128 type
(noir-lang/noir#7623)
([cc6cdbb](cc6cdbb))
* Validate blobs posted to sink belong to our L2
([#12587](#12587))
([9578f1e](9578f1e)),
closes
[#12497](#12497)


### Documentation

* update cli-wallet commands in profiler doc
([#12568](#12568))
([239a4fb](239a4fb))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lucasxia01 lucasxia01 lucasxia01 approved these changes

@jeanmon jeanmon jeanmon approved these changes

@ledwards2225 ledwards2225 Awaiting requested review from ledwards2225

Assignees

@fcarreiro fcarreiro

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@fcarreiro @lucasxia01 @jeanmon