feat: move busread and lookup block construction at the top of the trace #10707
Conversation
maramihali
force-pushed
the
mm/refine-active-ranges
branch
from
0e9e0df to
5656dac
Compare
maramihali
changed the title
barretenberg/cpp/src/barretenberg/commitment_schemes/commitment_key.hpp
Outdated
Show resolved
Hide resolved
| @@ -25,7 +25,10 @@ void construct_lookup_table_polynomials(const RefArray<typename Flavor::Polynomi | |||
| const size_t tables_size = circuit.get_tables_size(); | |||
| ASSERT(tables_size <= MAX_LOOKUP_TABLES_SIZE); // if false, may need to increase constant | |||
| ASSERT(dyadic_circuit_size > tables_size + additional_offset); | |||
| size_t offset = dyadic_circuit_size - tables_size - additional_offset; | |||
| size_t offset = circuit.blocks.lookup.trace_offset; | |||
| if constexpr (IsPlonkFlavor<Flavor>) { | |||
There was a problem hiding this comment.
this is the right thing to do but ugh. Plonk.
There was a problem hiding this comment.
getting closer and closer to removing it is what one would hope
| @@ -55,7 +55,7 @@ TEST_F(ComposerLibTests, LookupReadCounts) | |||
|
|
|||
| // The table polys are constructed at the bottom of the trace, thus so to are the counts/tags | |||
| // TODO(https://github.com/AztecProtocol/barretenberg/issues/1033): construct tables and counts at top of trace | |||
| size_t offset = circuit_size - builder.get_tables_size(); | |||
| size_t offset = builder.blocks.lookup.trace_offset; | |||
There was a problem hiding this comment.
I wonder if this offset should be codified into the flavor or something..
There was a problem hiding this comment.
it's a bit risky it will cause someone some annoying bugs :/ the composer lib stuff can be integrated in the decider proving key once we get rid of plonk so it would not look as odd to access trace_offsets there
| @@ -84,25 +84,23 @@ struct ExecutionTraceUsageTracker { | |||
| for (auto [max_size, fixed_block] : zip_view(max_sizes.get(), fixed_sizes.get())) { | |||
| size_t start_idx = fixed_block.trace_offset; | |||
| size_t end_idx = start_idx + max_size; | |||
| active_ranges.push_back(Range{ start_idx, end_idx }); | |||
| active_ranges.emplace_back(start_idx, end_idx); | |||
There was a problem hiding this comment.
I think this will not break the mac build since Range is just a std::pair which probably has an explicit constructor but you might want to make sure that's right. What you have is obviously better form but I'm suspicious as to why it was using the push_back syntax previously.. may have just been because the Range struct used to be something custom.
There was a problem hiding this comment.
To be clear the mac build will break if you try to use emplace_back(<constructor_inputs>) for a struct which may be trivially constructible but does not have an explicitly defined constructor. For example it would not work if you had
struct Range {
size_t start;
size_t end;
};
There was a problem hiding this comment.
ohh interesting, i will double check this is ok
...erg/cpp/src/barretenberg/plonk_honk_shared/execution_trace/execution_trace_usage_tracker.hpp
Show resolved
Hide resolved
...erg/cpp/src/barretenberg/plonk_honk_shared/execution_trace/execution_trace_usage_tracker.hpp
Outdated
Show resolved
Hide resolved
barretenberg/cpp/src/barretenberg/ultra_honk/decider_proving_key.hpp
Outdated
Show resolved
Hide resolved
…red trace also works appropriately
…nges' into mm/refine-active-ranges
maramihali
changed the title
maramihali
changed the title
| @@ -11,7 +11,7 @@ namespace bb { | |||
| template <typename Flavor> | |||
| void construct_lookup_table_polynomials(const RefArray<typename Flavor::Polynomial, 4>& table_polynomials, | |||
| const typename Flavor::CircuitBuilder& circuit, | |||
| const size_t dyadic_circuit_size, | |||
| [[maybe_unused]] const size_t dyadic_circuit_size, | |||
| @@ -33,16 +33,16 @@ struct ExecutionTraceUsageTracker { | |||
| size_t max_tables_size = 0; | |||
|
|
|||
| // For printing only. Must match the order of the members in the arithmetization | |||
| static constexpr std::array<std::string_view, 13> block_labels{ "ecc_op", | |||
| static constexpr std::array<std::string_view, 13> block_labels{ "busread", | |||
There was a problem hiding this comment.
these are wrong no? ecc_op should still be first
…nges' into mm/refine-active-ranges
Changes to circuit sizes
🧾 Summary (100% most significant diffs)
Full diff report 👇
|
Constructing the lookup block and lookup table data at the top of the trace removes the dependence of active ranges on the dyadic circuit size which was causing problems for the overflow scenario and also reduces the number of active rows to be close to the real size (modulo AztecProtocol/barretenberg#1152 which still needs investigation).