-
Notifications
You must be signed in to change notification settings - Fork 268
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
chore(avm): Allocate memory for unshifted polynomials according to their trace col size #9345
Changes from all commits
c6cc532
4887ee9
b6d0b44
7ce4084
6d36387
4d77abc
02daeca
cce6613
52c890a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,6 +2,8 @@ | |
#include "barretenberg/vm/avm/generated/circuit_builder.hpp" | ||
|
||
#include <mutex> | ||
#include <set> | ||
#include <unordered_map> | ||
|
||
#include "barretenberg/common/constexpr_utils.hpp" | ||
#include "barretenberg/common/thread.hpp" | ||
|
@@ -22,6 +24,23 @@ AvmCircuitBuilder::ProverPolynomials AvmCircuitBuilder::compute_polynomials() co | |
ASSERT(num_rows <= circuit_subgroup_size); | ||
ProverPolynomials polys; | ||
|
||
// We create a mapping between the polynomial index and the corresponding column index when row | ||
// is expressed as a vector, i.e., column of the trace matrix. | ||
std::unordered_map<std::string, size_t> names_to_col_idx; | ||
const auto names = Row::names(); | ||
for (size_t i = 0; i < names.size(); i++) { | ||
names_to_col_idx[names[i]] = i; | ||
} | ||
|
||
const auto labels = polys.get_unshifted_labels(); | ||
const size_t num_unshifted = labels.size(); | ||
|
||
// Mapping | ||
std::vector<size_t> polys_to_cols_unshifted_idx(num_unshifted); | ||
for (size_t i = 0; i < num_unshifted; i++) { | ||
polys_to_cols_unshifted_idx[i] = names_to_col_idx.at(labels[i]); | ||
} | ||
|
||
// Allocate mem for each column | ||
AVM_TRACK_TIME("circuit_builder/init_polys_to_be_shifted", ({ | ||
for (auto& poly : polys.get_to_be_shifted()) { | ||
|
@@ -30,15 +49,48 @@ AvmCircuitBuilder::ProverPolynomials AvmCircuitBuilder::compute_polynomials() co | |
/*make shiftable with offset*/ 1 }; | ||
} | ||
})); | ||
|
||
// catch-all with fully formed polynomials | ||
AVM_TRACK_TIME( | ||
"circuit_builder/init_polys_unshifted", ({ | ||
auto unshifted = polys.get_unshifted(); | ||
bb::parallel_for(unshifted.size(), [&](size_t i) { | ||
|
||
// An array which stores for each column of the trace the smallest size of the | ||
// truncated column containing all non-zero elements. | ||
// It is used to allocate the polynomials without memory overhead for the tail of zeros. | ||
std::array<size_t, Row::SIZE> col_nonzero_size{}; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You don't need the {} for size_t I think. But let's leave it just in case. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually you need them otherwise it will be uninitialized like a C array. |
||
|
||
// Computation of size of columns. | ||
// Non-parallel version takes 0.5 second for a trace size of 200k rows. | ||
// A parallel version might be considered in the future. | ||
for (size_t i = 0; i < num_rows; i++) { | ||
const auto row = rows[i].as_vector(); | ||
for (size_t col = 0; col < Row::SIZE; col++) { | ||
if (!row[col].is_zero()) { | ||
col_nonzero_size[col] = i + 1; | ||
} | ||
} | ||
} | ||
|
||
// Set of the labels for derived/inverse polynomials. | ||
const auto derived_labels = polys.get_derived_labels(); | ||
std::set<std::string> derived_labels_set(derived_labels.begin(), derived_labels.end()); | ||
|
||
bb::parallel_for(num_unshifted, [&](size_t i) { | ||
auto& poly = unshifted[i]; | ||
const auto col_idx = polys_to_cols_unshifted_idx[i]; | ||
size_t col_size = 0; | ||
|
||
// We fully allocate the inverse polynomials. We leave this potential memory optimization for later. | ||
if (derived_labels_set.contains(labels[i])) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. An idea (please double check): if you initialize You do still need the poly_to_cols, unfortunatelly. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Then, I would allocate a whole column for those which are full of zeros. I think the main issue to distinguish the inverse columns from the standard ones but full of zeros. |
||
col_size = num_rows; | ||
} else { | ||
col_size = col_nonzero_size[col_idx]; | ||
} | ||
|
||
if (poly.is_empty()) { | ||
// Not set above | ||
poly = Polynomial{ /*memory size*/ num_rows, /*largest possible index*/ circuit_subgroup_size }; | ||
poly = Polynomial{ /*memory size*/ col_size, /*largest possible index*/ circuit_subgroup_size }; | ||
} | ||
}); | ||
})); | ||
|
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.
Ideally this could be in an anonymous namespace helper