refactor(bb): use std::span in pippenger for scalars by ludamad · Pull Request #8269 · AztecProtocol/aztec-packages

ludamad · 2024-08-29T14:44:38Z

Refactoring stepping stone. Behaves identically

Next step would be to use this to allow accessing power of 2 quantities above the std::span size() (with a different wrapper class) so that non-powers-of-2 can be passed directly to pippenger

We recently anted to save memory on polynomials. The idea is that instead of rounding up to a power of 2 to make pippenger fast (at cost of memory), we will make a wrapper class that happily pretends it has T{} (i.e. zeroes) anywhere form 0 to nearest rounded up power of 2. For starters this just introduces a std::span, which should behave identically

maramihali

LGTM, added two requests for comments just to aid readability. Also maybe it would be good to attach relevant benchmarks in order to keep track.

maramihali · 2024-08-29T16:06:13Z

barretenberg/cpp/src/barretenberg/ecc/scalar_multiplication/scalar_multiplication.cpp

                                           pippenger_runtime_state<Curve>& state,
                                           bool handle_edge_cases)
 {
+    size_t num_initial_points_power_2 = 1 << numeric::get_msb(num_initial_points);


Can you please add a sente commenting these three lines

These shouldn't be here actually right now, will remove

maramihali · 2024-08-29T16:10:46Z

barretenberg/cpp/src/barretenberg/benchmark/pippenger_bench/main.cpp

    std::chrono::steady_clock::time_point time_start = std::chrono::steady_clock::now();
    g1::element result = scalar_multiplication::pippenger_unsafe<curve::BN254>(
-        &scalars[0], reference_string->get_monomial_points(), NUM_POINTS, state);
+        { &scalars[0], NUM_POINTS }, reference_string->get_monomial_points(), NUM_POINTS, state);


does this syntax construct a span from the localation of scalars[0] up to NUM_POINTS? I assume so. It'd be good if you could document this in whatever place you see best

…nger

…use-spans-pippenger

ludamad · 2024-08-29T16:30:55Z

The change to benchmarks is negligible, within noise (e.g. you might be lead to believe this speeds up things on some comparisons)

🤖 I have created a release *beep* *boop* --- <details><summary>aztec-package: 0.51.1</summary> ## [0.51.1](aztec-package-v0.51.0...aztec-package-v0.51.1) (2024-08-29) ### Features * Add status check to prover agent ([#8248](#8248)) ([7b3006a](7b3006a)) * Faster L1 deployment ([#8234](#8234)) ([51d6699](51d6699)) * Spartan token transfer ([#8163](#8163)) ([38f0157](38f0157)) </details> <details><summary>barretenberg.js: 0.51.1</summary> ## [0.51.1](barretenberg.js-v0.51.0...barretenberg.js-v0.51.1) (2024-08-29) ### Miscellaneous * **barretenberg.js:** Synchronize aztec-packages versions </details> <details><summary>aztec-packages: 0.51.1</summary> ## [0.51.1](aztec-packages-v0.51.0...aztec-packages-v0.51.1) (2024-08-29) ### Features * Add CLI command for gathering proving metrics ([#8221](#8221)) ([5929a42](5929a42)) * Add status check to prover agent ([#8248](#8248)) ([7b3006a](7b3006a)) * **avm:** 1-slot sload/sstore (nr, ts) ([#8264](#8264)) ([bdd9b06](bdd9b06)) * **avm:** Range check gadget ([#7967](#7967)) ([0dd954e](0dd954e)) * **docs:** Add partial notes doc ([#8192](#8192)) ([4299bbd](4299bbd)) * Faster L1 deployment ([#8234](#8234)) ([51d6699](51d6699)) * Initial validator set ([#8133](#8133)) ([6d31ad2](6d31ad2)) * L1-publisher cleanup ([#8148](#8148)) ([6ae2535](6ae2535)) * Proof surgery class ([#8236](#8236)) ([10d7edd](10d7edd)) * Request specific transactions through the p2p layer ([#8185](#8185)) ([54e1cc7](54e1cc7)) * Slot duration flexibility ([#8122](#8122)) ([708e4e5](708e4e5)) * Spartan token transfer ([#8163](#8163)) ([38f0157](38f0157)) ### Bug Fixes * Attempt to fix nightly test ([#8222](#8222)) ([477eec5](477eec5)) * **avm-simulator:** Await avm bytecode check ([#8268](#8268)) ([4410eb3](4410eb3)) * **bb-prover:** Create structure for AVM vk ([#8233](#8233)) ([55b6ba2](55b6ba2)) * **bb:** Mac build ([#8255](#8255)) ([ac54f5c](ac54f5c)) * **ci:** Spot-runner-action was not built ([#8274](#8274)) ([c1509c1](c1509c1)) * **ci:** Try fix brotli edge-case ([#8256](#8256)) ([e03ea0b](e03ea0b)) * Docker containers healthchecks ([#8228](#8228)) ([19edbbb](19edbbb)) * **docs:** Update entrypoint details on accounts page ([#8184](#8184)) ([8453ec7](8453ec7)) * Export brillig names in contract functions ([#8212](#8212)) ([4745741](4745741)) * Fixes for the nightly test run against Sepolia ([#8229](#8229)) ([cfc65c6](cfc65c6)) * Handle constant output for sha256 ([#8251](#8251)) ([0653ba5](0653ba5)) * Log public vm errors as warn in prover-agent ([#8247](#8247)) ([9f4ea9f](9f4ea9f)) * Remove devnet ARM builds for now ([#8202](#8202)) ([81ef715](81ef715)) * Remove fundFpc step from bootstrap ([#8245](#8245)) ([a742531](a742531)) * Ts codegen ([#8267](#8267)) ([cb58800](cb58800)) ### Miscellaneous * Add check to just release images to devnet-deploys ([#8242](#8242)) ([aa6791d](aa6791d)) * Add partial note support for value note ([#8141](#8141)) ([daa57cc](daa57cc)) * Always run `build-check` step in `publish-bb.yml` ([#8240](#8240)) ([5e9749f](5e9749f)) * **avm:** Replace range and cmp with gadgets ([#8164](#8164)) ([cc12558](cc12558)) * Basic network matrix ([#8257](#8257)) ([2a76b1a](2a76b1a)), closes [#8001](#8001) * **bb:** Use std::span in pippenger for scalars ([#8269](#8269)) ([2323cd5](2323cd5)) * Configure interval mining for anvil ([#8211](#8211)) ([eba57b4](eba57b4)) * Create external-ci-approved.yml ([#8235](#8235)) ([24b059b](24b059b)) * Disallow prune in devnet + add onlyOwners ([#8134](#8134)) ([c736f96](c736f96)) * Fix various warnings in noir code ([#8258](#8258)) ([1c6b478](1c6b478)) * Less noisy AVM failures in proving ([#8227](#8227)) ([03bcd62](03bcd62)) * Open an issue if publishing bb fails ([#8223](#8223)) ([2d7a775](2d7a775)) * Reinstate l1-contracts package ([#8250](#8250)) ([263a912](263a912)) * Remove unused generic parameters ([#8249](#8249)) ([00ed045](00ed045)) * Replace relative paths to noir-protocol-circuits ([1783c80](1783c80)) * Replace relative paths to noir-protocol-circuits ([ffe1f35](ffe1f35)) * Report prover metrics ([#8155](#8155)) ([dc7bcdf](dc7bcdf)), closes [#7675](#7675) * Rework balances map ([#8127](#8127)) ([1cac3dd](1cac3dd)), closes [#8104](#8104) * Run CI after merges to provernet ([#8244](#8244)) ([97e5e25](97e5e25)) ### Documentation * Minor fixes ([#8273](#8273)) ([2b8af9e](2b8af9e)) </details> <details><summary>barretenberg: 0.51.1</summary> ## [0.51.1](barretenberg-v0.51.0...barretenberg-v0.51.1) (2024-08-29) ### Features * **avm:** 1-slot sload/sstore (nr, ts) ([#8264](#8264)) ([bdd9b06](bdd9b06)) * **avm:** Range check gadget ([#7967](#7967)) ([0dd954e](0dd954e)) * Proof surgery class ([#8236](#8236)) ([10d7edd](10d7edd)) ### Bug Fixes * **bb-prover:** Create structure for AVM vk ([#8233](#8233)) ([55b6ba2](55b6ba2)) * **bb:** Mac build ([#8255](#8255)) ([ac54f5c](ac54f5c)) * Handle constant output for sha256 ([#8251](#8251)) ([0653ba5](0653ba5)) ### Miscellaneous * **avm:** Replace range and cmp with gadgets ([#8164](#8164)) ([cc12558](cc12558)) * **bb:** Use std::span in pippenger for scalars ([#8269](#8269)) ([2323cd5](2323cd5)) </details> --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).

ludamad added 2 commits August 29, 2024 14:44

use spans in pippenger for scalars

a4d8d8d

partial revert

f31de4d

maramihali approved these changes Aug 29, 2024

View reviewed changes

ludamad and others added 6 commits August 29, 2024 16:22

revert

b7a9ddd

Update main.cpp

56fec15

Merge github.com:AztecProtocol/aztec-packages into ad/use-spans-pippe…

5db113d

…nger

add size comment

a9b9344

Merge remote-tracking branch 'origin/ad/use-spans-pippenger' into ad/…

e72db15

…use-spans-pippenger

revert

7b5d95e

ludamad enabled auto-merge (squash) August 29, 2024 16:30

ludamad merged commit 2323cd5 into master Aug 29, 2024

ludamad deleted the ad/use-spans-pippenger branch August 29, 2024 17:01

AztecBot mentioned this pull request Aug 29, 2024

chore(master): Release 0.51.1 #8218

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(bb): use std::span in pippenger for scalars#8269

refactor(bb): use std::span in pippenger for scalars#8269
ludamad merged 8 commits intomasterfrom
ad/use-spans-pippenger

ludamad commented Aug 29, 2024 •

edited

Loading

Uh oh!

maramihali left a comment

Uh oh!

maramihali Aug 29, 2024

Uh oh!

ludamad Aug 29, 2024

Uh oh!

maramihali Aug 29, 2024

Uh oh!

ludamad Aug 29, 2024

Uh oh!

ludamad commented Aug 29, 2024 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

ludamad commented Aug 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maramihali left a comment

Choose a reason for hiding this comment

Uh oh!

maramihali Aug 29, 2024

Choose a reason for hiding this comment

Uh oh!

ludamad Aug 29, 2024

Choose a reason for hiding this comment

Uh oh!

maramihali Aug 29, 2024

Choose a reason for hiding this comment

Uh oh!

ludamad Aug 29, 2024

Choose a reason for hiding this comment

Uh oh!

ludamad commented Aug 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ludamad commented Aug 29, 2024 •

edited

Loading

ludamad commented Aug 29, 2024 •

edited

Loading