Support shared preproccessed columns.

[ilyalesokhin-starkware]

No description provided.

[reviewable-StarkWare]

This change is 

[andrewmilson]

Reviewable status: 0 of 9 files reviewed, 5 unresolved discussions (waiting on @ilyalesokhin-starkware, @ohad-starkware, and @shaharsamocha7)

---

crates/prover/src/constraint_framework/component.rs line 71 at r1 (raw file):

    }

    pub fn with_preproccessed_columnds(preprocessed_columns: &[PreprocessedColumn]) -> Self {

WDYT?

Suggestion:

new_with_preproccessed_columns

---

crates/prover/src/constraint_framework/component.rs line 167 at r1 (raw file):

            .preprocessed_column_indices
            .iter()
            .map(|idx| mask.0[PREPROCESSED_TRACE_IDX][*idx].clone())

Nit: it's deref

Suggestion:

mask[

---

crates/prover/src/constraint_framework/component.rs line 171 at r1 (raw file):

        let mut mask_points = mask.sub_tree(&self.trace_locations);
        mask_points.0[PREPROCESSED_TRACE_IDX] = preprocessed_mask.iter().collect_vec();

Suggestion:

mask_points[PREPROCESSED_TRACE_IDX] = preprocessed_mask;

---

crates/prover/src/core/air/components.rs line 35 at r1 (raw file):

        *preprocessed_mask_points = vec![vec![]; n_preprocessed_columns];

        for component in self.0.iter() {

Nit

Suggestion:

&self.0

---

crates/prover/src/core/vcs/verifier.rs line 61 at r1 (raw file):

        let max_log_size = self.column_log_sizes.iter().max().copied().unwrap_or(0);

        eprintln!("self.column_log_sizes: {:?}", self.column_log_sizes);

Should we use tracing logs here i.e. info!(...) since we use for similar logs in other spots like here

[andrewmilson]

Reviewable status: 0 of 9 files reviewed, 6 unresolved discussions (waiting on @ilyalesokhin-starkware, @ohad-starkware, and @shaharsamocha7)

---

crates/prover/src/examples/wide_fibonacci/mod.rs line 35 at r1 (raw file):

    fn evaluate<E: EvalAtRow>(&self, mut eval: E) -> E {
        // TODO(ilya): Remove the following once preproccessed columns are not mandatory.
        // let _ = eval.get_preprocessed_column(PreprocessedColumn::IsFirst(self.log_size()));

Remove?

[ilyalesokhin-starkware]

crates/prover/src/core/vcs/verifier.rs line 61 at r1 (raw file):

Previously, andrewmilson (Andrew Milson) wrote…

Should we use tracing logs here i.e. info!(...) since we use for similar logs in other spots like here

IT was here for debugging, I'll remove that.
Thanks.

[ilyalesokhin-starkware]

Reviewable status: 0 of 9 files reviewed, 4 unresolved discussions (waiting on @andrewmilson, @ohad-starkware, and @shaharsamocha7)

---

crates/prover/src/constraint_framework/component.rs line 167 at r1 (raw file):

Previously, andrewmilson (Andrew Milson) wrote…

Nit: it's deref

done

---

crates/prover/src/constraint_framework/component.rs line 171 at r1 (raw file):

        let mut mask_points = mask.sub_tree(&self.trace_locations);
        mask_points.0[PREPROCESSED_TRACE_IDX] = preprocessed_mask.iter().collect_vec();

done

---

crates/prover/src/core/air/components.rs line 35 at r1 (raw file):

Previously, andrewmilson (Andrew Milson) wrote…

Nit

done

[ilyalesokhin-starkware]

Reviewable status: 0 of 13 files reviewed, 4 unresolved discussions (waiting on @andrewmilson, @ohad-starkware, and @shaharsamocha7)

---

crates/prover/src/constraint_framework/component.rs line 71 at r1 (raw file):

Previously, andrewmilson (Andrew Milson) wrote…

WDYT?

Done.

[ilyalesokhin-starkware]

Reviewable status: 0 of 13 files reviewed, 4 unresolved discussions (waiting on @andrewmilson, @ohad-starkware, and @shaharsamocha7)

---

crates/prover/src/examples/wide_fibonacci/mod.rs line 35 at r1 (raw file):

Previously, andrewmilson (Andrew Milson) wrote…

Remove?

Done.

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 72.88136% with 32 lines in your changes missing coverage. Please review.

Project coverage is 91.73%. Comparing base (8385e54) to head (971c8cf).

Files with missing lines	Patch %	Lines
...rates/prover/src/constraint_framework/component.rs	74.66%	18 Missing and 1 partial ⚠️
crates/prover/src/core/air/components.rs	66.66%	11 Missing and 1 partial ⚠️
crates/prover/src/constraint_framework/info.rs	85.71%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##              dev     #869      +/-   ##
==========================================
- Coverage   91.92%   91.73%   -0.19%     
==========================================
  Files          92       92              
  Lines       12626    12731     +105     
  Branches    12626    12731     +105     
==========================================
+ Hits        11606    11679      +73     
- Misses        910      939      +29     
- Partials      110      113       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[github-actions]

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 2.

Benchmark suite	Current: 971c8cf	Previous: f6214d1	Ratio
merkle throughput/simd merkle	30108332 ns/iter (± 452138)	14690867 ns/iter (± 434150)	2.05

This comment was automatically generated by workflow using github-action-benchmark.

CC: @shaharsamocha7

[andrewmilson]

with comment

Reviewable status: 0 of 13 files reviewed, 6 unresolved discussions (waiting on @ilyalesokhin-starkware, @ohad-starkware, and @shaharsamocha7)

---

crates/prover/src/core/air/components.rs line 37 at r3 (raw file):

        for component in &self.0 {
            for idx in component.preproccessed_column_indices() {
                preprocessed_mask_points[idx].resize(1, point);

WDYT?

Suggestion:

preprocessed_mask_points[idx] = vec![point];

---

crates/prover/src/core/prover/mod.rs line 110 at r3 (raw file):

    let oods_point = CirclePoint::<SecureField>::get_random_point(channel);

    eprintln!(

Is this eprintln (and the others) also for debugging?

[shaharsamocha7]

Reviewed all commit messages.
Reviewable status: 0 of 13 files reviewed, 8 unresolved discussions (waiting on @andrewmilson, @ilyalesokhin-starkware, and @ohad-starkware)

---

crates/prover/src/constraint_framework/component.rs line 141 at r3 (raw file):

            .map(|tree_offsets| vec![self.eval.log_size(); tree_offsets.len()]);

        log_degree_bounds[0] = self

mask_offset [0] is empty vec before adding the preprocessed columns?
maybe we should put a dbg_assert?

Code quote:

log_degree_bounds[0] = self

---

crates/prover/src/core/prover/mod.rs line 117 at r3 (raw file):

            .column_log_sizes
            .len()
    );

Remove (or change to dbg!)

Code quote:

    eprintln!(
        "commitment_scheme.trees[PREPROCESSED_TRACE_IDX]
            .column_log_sizes
            .len()={}",
        commitment_scheme.trees[PREPROCESSED_TRACE_IDX]
            .column_log_sizes
            .len()
    );

[ilyalesokhin-starkware]

crates/prover/src/constraint_framework/component.rs line 141 at r3 (raw file):

Previously, shaharsamocha7 wrote…

mask_offset [0] is empty vec before adding the preprocessed columns?
maybe we should put a dbg_assert?

yes

[ilyalesokhin-starkware]

crates/prover/src/constraint_framework/component.rs line 71 at r1 (raw file):

Previously, ilyalesokhin-starkware wrote…

Done.

done

[ilyalesokhin-starkware]

crates/prover/src/core/air/components.rs line 37 at r3 (raw file):

Previously, andrewmilson (Andrew Milson) wrote…

WDYT?

I think it is slightly less efficient, but I don't mind if you think it is better.

[ilyalesokhin-starkware]

Reviewable status: 0 of 13 files reviewed, 8 unresolved discussions (waiting on @Alon-Ti, @andrewmilson, @ohad-starkware, and @shaharsamocha7)

---

crates/prover/src/core/prover/mod.rs line 110 at r3 (raw file):

Previously, andrewmilson (Andrew Milson) wrote…

Is this eprintln (and the others) also for debugging?

removed

---

crates/prover/src/core/prover/mod.rs line 117 at r3 (raw file):

Previously, shaharsamocha7 wrote…

Remove (or change to dbg!)

Done.

[ilyalesokhin-starkware]

Reviewable status: 0 of 13 files reviewed, 6 unresolved discussions (waiting on @Alon-Ti, @andrewmilson, and @ohad-starkware)

---

crates/prover/src/examples/blake/air.rs line 93 at r6 (raw file):

            }),
        )
        .collect_vec();

I'm not sure this is the right place to put it.

Code quote:

        log_sizes[PREPROCESSED_TRACE_IDX] = chain!(
            [log_size],
            ROUND_LOG_SPLIT.iter().map(|l| log_size + l),
            PREPROCESSED_XOR_COLUMNS.map(|column| match column {
                PreprocessedColumn::XorTable(elem_bits, expand_bits, _) =>
                    2 * (elem_bits - expand_bits),
                PreprocessedColumn::IsFirst(log_size) => log_size,
                _ => panic!("Unexpected column"),
            }),
        )
        .collect_vec();

[andrewmilson]

Reviewable status: 0 of 13 files reviewed, 9 unresolved discussions (waiting on @Alon-Ti, @ilyalesokhin-starkware, and @ohad-starkware)

---

crates/prover/src/core/air/components.rs line 37 at r3 (raw file):

Previously, ilyalesokhin-starkware wrote…

I think it is slightly less efficient, but I don't mind if you think it is better.

Agree I just thought it was more readable. This function isn't performance critical.

---

crates/prover/src/examples/blake/air.rs line 93 at r6 (raw file):

Previously, ilyalesokhin-starkware wrote…

I'm not sure this is the right place to put it.

Seems ok to me for now.
Was there somewhere you wanted to put it?

---

crates/prover/src/examples/blake/air.rs line 85 at r6 (raw file):

        log_sizes[PREPROCESSED_TRACE_IDX] = chain!(
            [log_size],
            ROUND_LOG_SPLIT.iter().map(|l| log_size + l),

What are these first two for? Can it be documented plz?

Code quote:

            [log_size],
            ROUND_LOG_SPLIT.iter().map(|l| log_size + l),

---

crates/prover/src/core/air/components.rs line 68 at r6 (raw file):

    pub fn column_log_sizes(&self) -> TreeVec<ColumnVec<u32>> {
        let mut preprocessed_columns = vec![0; self.n_preprocessed_columns];
        let mut updated_columns = vec![false; self.n_preprocessed_columns];

Optional: could just do one storing Option.

Code quote:

        let mut preprocessed_columns = vec![0; self.n_preprocessed_columns];
        let mut updated_columns = vec![false; self.n_preprocessed_columns];

---

crates/prover/src/core/air/components.rs line 77 at r6 (raw file):

                .into_iter()
                .zip(std::mem::take(&mut component_trace_log_sizes[0]))
            {

This isn't performance critical. WDYT?
I think offset should be renamed to index because column offset I usually associate with a column's mask offset.

Suggestion:

            let mut preprocessed_trace_log_sizes = &component.trace_log_degree_bounds()[0];

            for (i, &log_size) in zip(component.preproccessed_column_indices(), preprocessed_trace_log_sizes) {

[ilyalesokhin-starkware]

crates/prover/src/examples/blake/air.rs line 93 at r6 (raw file):

Previously, andrewmilson (Andrew Milson) wrote…

Seems ok to me for now.
Was there somewhere you wanted to put it?

maybe in verify_blake? as the verifier is supposed to know the structure of the preproccess trace.

The problem is that I do want the assert here:
https://github.com/starkware-industries/stwo/blob/b36d75f09347977062a1eb077db6c11069996980/crates/prover/src/examples/blake/air.rs#L439

[ilyalesokhin-starkware]

crates/prover/src/core/air/components.rs line 77 at r6 (raw file):

Previously, andrewmilson (Andrew Milson) wrote…

This isn't performance critical. WDYT?
I think offset should be renamed to index because column offset I usually associate with a column's mask offset.

I return component_trace_log_sizes at the end of the map.

[ilyalesokhin-starkware]

crates/prover/src/core/air/components.rs line 68 at r6 (raw file):

Previously, andrewmilson (Andrew Milson) wrote…

Optional: could just do one storing Option.

I would have to convert it to a vector of non-options at the end.

[ilyalesokhin-starkware]

Reviewable status: 0 of 13 files reviewed, 9 unresolved discussions (waiting on @Alon-Ti, @andrewmilson, and @ohad-starkware)

---

crates/prover/src/core/air/components.rs line 37 at r3 (raw file):

Previously, andrewmilson (Andrew Milson) wrote…

Agree I just thought it was more readable. This function isn't performance critical.

Done.

---

crates/prover/src/examples/blake/air.rs line 85 at r6 (raw file):

Previously, andrewmilson (Andrew Milson) wrote…

What are these first two for? Can it be documented plz?

Done.

[andrewmilson]

Reviewable status: 0 of 13 files reviewed, 9 unresolved discussions (waiting on @Alon-Ti and @ohad-starkware)

[ilyalesokhin-starkware]

crates/prover/src/constraint_framework/component.rs line 134 at r8 (raw file):

                        );
                        next_column
                    })

Note that this removes duplicated columns.

Code quote:

                    .entry(*col)
                    .or_insert({
                        assert_matches!(
                            location_allocator.preprocessed_columns_allocation_mode,
                            PreprocessedColumnsAllocationMode::Dynamic,
                        );
                        next_column
                    })