Skip to content

Conversation

@jeanmon
Copy link
Contributor

@jeanmon jeanmon commented Feb 3, 2026

This PR performs the following changes (kudo to @fcarreiro for suggesting this):

  • precomputed.clk is non-zero only on the active portion of the precomputed subtrace
  • Introduce a clk column in execution sub-trace (counter on active rows). For each execution dispatching lookup using precomputed.clk, we replace precomputed.clk by execution.clk.
  • Rename precomputed.clk by precomputed.idx

Performance improvements

  • PCS: goes from 3s to ~1.5s on 32 cores.
  • For bulk this is a small improvement.
  • But for token transfers it is a 25% improvement.

@jeanmon jeanmon marked this pull request as ready for review February 3, 2026 17:59
auto index_of_max_end_index = [](const auto& polys) {
size_t max_idx = 0;
size_t max_end_idx = polys[0].end_index();
for (size_t idx = 0; const auto& poly : polys) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@fcarreiro Maybe this way of iterating is a bit dangerous. Maybe we do not have the guarantee that the iteration is following the indices of the span.

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 you can use these functions

// this assumes non-empty, returns an iterator
auto it = std::max_element(nums.begin(), nums.end(), [](int a, int b) {
    return a < b; // Your custom comparator logic
});

// retrieves the index
size_t index = std::distance(nums.begin(), it);

Otherwise I would do a good old for loop. Spans have size(), and they are contiguous.

Copy link
Contributor

@fcarreiro fcarreiro left a comment

Choose a reason for hiding this comment

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

I have some doubts about the PIL changes. I think that only lookups into precomputed or PIs should use idx. Shouldn't the rest use the execution clk? In particular think of memory. In simulation the clk manager (or whatever the name was) links it to execution.

auto index_of_max_end_index = [](const auto& polys) {
size_t max_idx = 0;
size_t max_end_idx = polys[0].end_index();
for (size_t idx = 0; const auto& poly : polys) {
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 you can use these functions

// this assumes non-empty, returns an iterator
auto it = std::max_element(nums.begin(), nums.end(), [](int a, int b) {
    return a < b; // Your custom comparator logic
});

// retrieves the index
size_t index = std::distance(nums.begin(), it);

Otherwise I would do a good old for loop. Spans have size(), and they are contiguous.

@jeanmon
Copy link
Contributor Author

jeanmon commented Feb 3, 2026

I have some doubts about the PIL changes. I think that only lookups into precomputed or PIs should use idx. Shouldn't the rest use the execution clk? In particular think of memory. In simulation the clk manager (or whatever the name was) links it to execution.

@fcarreiro You are absolutely right. I think I overlooked the virtual trace of execution. I will review all of these changes more carefully.

@jeanmon jeanmon force-pushed the jm/smaller-precomputed-trace branch from 18b28ac to 82d36b4 Compare February 3, 2026 22:59
@jeanmon jeanmon requested a review from fcarreiro February 3, 2026 23:11
@jeanmon jeanmon force-pushed the jm/smaller-precomputed-trace branch from 82d36b4 to 8d4560e Compare February 3, 2026 23:16
@jeanmon jeanmon requested review from federicobarbacovi and iakovenkos and removed request for IlyasRidhuan and Maddiaa0 February 3, 2026 23:17
@jeanmon
Copy link
Contributor Author

jeanmon commented Feb 3, 2026

@federicobarbacovi @iakovenkos Could you please have a look at the changes in the files: prover.cpp, verifier.cpp, recursive_verifier.cpp?

idx++;
}

PolynomialBatcher polynomial_batcher(ProvingKey::circuit_size);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@iakovenkos Is this correct that we should not change the size of polynomial_batcher ?

polynomial_batcher.set_unshifted(RefVector{ batched_unshifted });
polynomial_batcher.set_to_be_shifted_by_one(RefVector{ batched_shifted });

const size_t circuit_dyadic_size = numeric::round_up_power_2(batched_unshifted.end_index());
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@iakovenkos Is this what you expected?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants