Skip to content
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

Support preproccessed columns #869

Draft
wants to merge 2 commits into
base: ilya/prepro
Choose a base branch
from

Conversation

ilyalesokhin-starkware
Copy link
Collaborator

No description provided.

@reviewable-StarkWare
Copy link

This change is Reviewable

Copy link
Contributor

@andrewmilson andrewmilson left a comment

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@andrewmilson andrewmilson left a comment

Choose a reason for hiding this comment

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

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
Copy link
Collaborator Author

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.

Copy link
Collaborator Author

@ilyalesokhin-starkware ilyalesokhin-starkware left a comment

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

@ilyalesokhin-starkware ilyalesokhin-starkware left a comment

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

@ilyalesokhin-starkware ilyalesokhin-starkware left a comment

Choose a reason for hiding this comment

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

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
Copy link

Codecov Report

Attention: Patch coverage is 97.77778% with 2 lines in your changes missing coverage. Please review.

Project coverage is 91.94%. Comparing base (e37819b) to head (8c9f706).

Files with missing lines Patch % Lines
crates/prover/src/constraint_framework/info.rs 85.71% 1 Missing ⚠️
crates/prover/src/core/vcs/verifier.rs 50.00% 1 Missing ⚠️
Additional details and impacted files
@@               Coverage Diff               @@
##           ilya/prepro     #869      +/-   ##
===============================================
+ Coverage        91.90%   91.94%   +0.03%     
===============================================
  Files               92       92              
  Lines            12604    12686      +82     
  Branches         12604    12686      +82     
===============================================
+ Hits             11584    11664      +80     
- Misses             910      912       +2     
  Partials           110      110              

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

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

⚠️ 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: 2520abf Previous: f6214d1 Ratio
merkle throughput/simd merkle 30115036 ns/iter (± 464441) 14690867 ns/iter (± 434150) 2.05

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

CC: @shaharsamocha7

Copy link
Contributor

@andrewmilson andrewmilson left a comment

Choose a reason for hiding this comment

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

:lgtm: 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?

Copy link
Collaborator

@shaharsamocha7 shaharsamocha7 left a comment

Choose a reason for hiding this comment

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

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()
    );

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.

5 participants