Skip to content

refactor(bb): small cleanup in protogalaxy prover #8072

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

Merged
ludamad merged 10 commits into from
Aug 19, 2024
Merged

refactor(bb): small cleanup in protogalaxy prover #8072

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
f2b8a36
chore: does parallel for nest right now
ludamad Aug 19, 2024
579ab70
protogalaxy splitting
ludamad Aug 19, 2024
1c43402
proto splitting part 2
ludamad Aug 19, 2024
49bdfa6
split proto
ludamad Aug 19, 2024
a8240d8
ensure ci
ludamad Aug 19, 2024
f58a558
only check for parallel nesting for in debug mode
ludamad Aug 19, 2024
5bca86c
try simple
ludamad Aug 19, 2024
47d99b5
Merge remote-tracking branch 'origin/master' into ad/try-see-parallel…
ludamad Aug 19, 2024
3c30f5c
reinstate
ludamad Aug 19, 2024
dd5e687
comment
ludamad Aug 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion barretenberg/cpp/src/barretenberg/common/parallel_for_mutex_pool.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
#include "barretenberg/common/throw_or_abort.hpp"
#ifndef NO_MULTITHREADING
#include "log.hpp"
#include "thread.hpp"
Expand Down Expand Up @@ -124,10 +125,18 @@ namespace bb {
void parallel_for_mutex_pool(size_t num_iterations, const std::function<void(size_t)>& func)
{
static ThreadPool pool(get_num_cpus() - 1);

// Note that if this is used safely, we don't need the std::atomic_bool (can use bool), but if we are catching the
// mess up case of nesting parallel_for this should be atomic
static std::atomic_bool nested = false;
Copy link
Collaborator Author

@ludamad ludamad Aug 19, 2024

Choose a reason for hiding this comment

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

note that if this is used safely, we don't need the std::atomic, but if we are catching the mess up case of nesting parallel_for this should be atomic

Copy link
Collaborator Author

@ludamad ludamad Aug 19, 2024

Choose a reason for hiding this comment

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

gonna just make that a comment

// Check if we are already in a nested parallel_for_mutex_pool call
bool expected = false;
if (!nested.compare_exchange_strong(expected, true)) {
throw_or_abort("Error: Nested parallel_for_mutex_pool calls are not allowed.");
}
// info("starting job with iterations: ", num_iterations);
pool.start_tasks(num_iterations, func);
// info("done");
nested = false;
}
} // namespace bb
#endif
103 changes: 53 additions & 50 deletions barretenberg/cpp/src/barretenberg/protogalaxy/combiner.test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -327,53 +327,56 @@ TEST(Protogalaxy, CombinerOptimizationConsistency)
run_test(false);
};

TEST(Protogalaxy, CombinerOn4Instances)
{
constexpr size_t NUM_INSTANCES = 4;
using ProverInstance = ProverInstance_<Flavor>;
using ProverInstances = ProverInstances_<Flavor, NUM_INSTANCES>;
using ProtoGalaxyProver = ProtoGalaxyProver_<ProverInstances>;

const auto zero_all_selectors = [](auto& polys) {
std::fill(polys.q_arith.begin(), polys.q_arith.end(), 0);
std::fill(polys.q_delta_range.begin(), polys.q_delta_range.end(), 0);
std::fill(polys.q_elliptic.begin(), polys.q_elliptic.end(), 0);
std::fill(polys.q_aux.begin(), polys.q_aux.end(), 0);
std::fill(polys.q_lookup.begin(), polys.q_lookup.end(), 0);
std::fill(polys.q_4.begin(), polys.q_4.end(), 0);
std::fill(polys.w_4.begin(), polys.w_4.end(), 0);
std::fill(polys.w_4_shift.begin(), polys.w_4_shift.end(), 0);
};

auto run_test = [&]() {
std::vector<std::shared_ptr<ProverInstance>> instance_data(NUM_INSTANCES);
ProtoGalaxyProver prover;

for (size_t idx = 0; idx < NUM_INSTANCES; idx++) {
auto instance = std::make_shared<ProverInstance>();
auto prover_polynomials = get_zero_prover_polynomials<Flavor>(
/*log_circuit_size=*/1);
instance->proving_key.polynomials = std::move(prover_polynomials);
instance->proving_key.circuit_size = 2;
instance_data[idx] = instance;
}

ProverInstances instances{ instance_data };
instances.alphas.fill(bb::Univariate<FF, 40>(FF(0))); // focus on the arithmetic relation only

zero_all_selectors(instances[0]->proving_key.polynomials);
zero_all_selectors(instances[1]->proving_key.polynomials);
zero_all_selectors(instances[2]->proving_key.polynomials);
zero_all_selectors(instances[3]->proving_key.polynomials);

auto pow_polynomial = PowPolynomial(std::vector<FF>{ 2 });
auto result = prover.compute_combiner</*OptimisationEnabled=*/false>(instances, pow_polynomial);
auto optimised_result = prover.compute_combiner(instances, pow_polynomial);
std::array<FF, 40> zeroes;
std::fill(zeroes.begin(), zeroes.end(), 0);
auto expected_result = Univariate<FF, 40>(zeroes);
EXPECT_EQ(result, expected_result);
EXPECT_EQ(optimised_result, expected_result);
};
run_test();
};
// Tests a combiner on 4 instances, note currently we don't plan
Copy link
Contributor

@ledwards2225 ledwards2225 Aug 19, 2024

Choose a reason for hiding this comment

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

commented out for debugging?

Copy link
Collaborator Author

@ludamad ludamad Aug 19, 2024

Choose a reason for hiding this comment

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

As discussed, we just don't plan k>2 folding. This is commented out and not removed just in case, but perhaps it should just be removed

// to fold with num instances > 2, this would require an additional explicit instantiation in
// protogalaxy_prover_ultra.cpp. Currently, we rather save the compile time.
// TEST(Protogalaxy, CombinerOn4Instances)
// {
// constexpr size_t NUM_INSTANCES = 4;
// using ProverInstance = ProverInstance_<Flavor>;
// using ProverInstances = ProverInstances_<Flavor, NUM_INSTANCES>;
// using ProtoGalaxyProver = ProtoGalaxyProver_<ProverInstances>;

// const auto zero_all_selectors = [](auto& polys) {
// std::fill(polys.q_arith.begin(), polys.q_arith.end(), 0);
// std::fill(polys.q_delta_range.begin(), polys.q_delta_range.end(), 0);
// std::fill(polys.q_elliptic.begin(), polys.q_elliptic.end(), 0);
// std::fill(polys.q_aux.begin(), polys.q_aux.end(), 0);
// std::fill(polys.q_lookup.begin(), polys.q_lookup.end(), 0);
// std::fill(polys.q_4.begin(), polys.q_4.end(), 0);
// std::fill(polys.w_4.begin(), polys.w_4.end(), 0);
// std::fill(polys.w_4_shift.begin(), polys.w_4_shift.end(), 0);
// };

// auto run_test = [&]() {
// std::vector<std::shared_ptr<ProverInstance>> instance_data(NUM_INSTANCES);
// ProtoGalaxyProver prover;

// for (size_t idx = 0; idx < NUM_INSTANCES; idx++) {
// auto instance = std::make_shared<ProverInstance>();
// auto prover_polynomials = get_zero_prover_polynomials<Flavor>(
// /*log_circuit_size=*/1);
// instance->proving_key.polynomials = std::move(prover_polynomials);
// instance->proving_key.circuit_size = 2;
// instance_data[idx] = instance;
// }

// ProverInstances instances{ instance_data };
// instances.alphas.fill(bb::Univariate<FF, 40>(FF(0))); // focus on the arithmetic relation only

// zero_all_selectors(instances[0]->proving_key.polynomials);
// zero_all_selectors(instances[1]->proving_key.polynomials);
// zero_all_selectors(instances[2]->proving_key.polynomials);
// zero_all_selectors(instances[3]->proving_key.polynomials);

// auto pow_polynomial = PowPolynomial(std::vector<FF>{ 2 });
// auto result = prover.compute_combiner</*OptimisationEnabled=*/false>(instances, pow_polynomial);
// auto optimised_result = prover.compute_combiner(instances, pow_polynomial);
// std::array<FF, 40> zeroes;
// std::fill(zeroes.begin(), zeroes.end(), 0);
// auto expected_result = Univariate<FF, 40>(zeroes);
// EXPECT_EQ(result, expected_result);
// EXPECT_EQ(optimised_result, expected_result);
// };
// run_test();
// };
Loading