Initialize logups automatically when first used.

[Alon-Ti]

No description provided.

[reviewable-StarkWare]

This change is 

[Alon-Ti]

• Initialize logups automatically when first used. #879 : 2 dependent PRs (#883 , #887 ) 👈 (View in Graphite)
• Abstracted eval.write_fracs to eval.add_to_relations. #876
• Added LogupAtRow functionality to EvalAtRow. #875
• dev

This stack of pull requests is managed by Graphite. Learn more about stacking.

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 91.83673% with 8 lines in your changes missing coverage. Please review.

Project coverage is 91.81%. Comparing base (e6cd98e) to head (0c64f92).

Files with missing lines	Patch %	Lines
crates/prover/src/constraint_framework/expr.rs	0.00%	7 Missing ⚠️
crates/prover/src/constraint_framework/mod.rs	97.36%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@                             Coverage Diff                             @@
##           alont/generalize-gen-interaction-trace1     #879      +/-   ##
===========================================================================
+ Coverage                                    91.79%   91.81%   +0.02%     
===========================================================================
  Files                                           92       92              
  Lines                                        12714    12721       +7     
  Branches                                     12714    12721       +7     
===========================================================================
+ Hits                                         11671    11680       +9     
+ Misses                                         932      930       -2     
  Partials                                       111      111              

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

[shaharsamocha7]

Reviewable status: 0 of 23 files reviewed, 2 unresolved discussions (waiting on @Alon-Ti)

---

a discussion (no related file):
Can you split this pr in any way?
It is both move a lot of code and do non-trivial changes such that write_frac also initialize the logup

---

a discussion (no related file):
I also think that we could extract some function from write_frac code macro to make it more clear

[ohad-starkware]

Reviewable status: 0 of 23 files reviewed, 6 unresolved discussions (waiting on @Alon-Ti and @shaharsamocha7)

---

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

    info: InfoEvaluator,
    total_sum: SecureField,
    claimed_sum: Option<ClaimedPrefixSum>,

use logup sums

Code quote:

    total_sum: SecureField,
    claimed_sum: Option<ClaimedPrefixSum>,

---

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

        let trace_domain = CanonicCoset::new(self.eval.log_size());

        println!("n polys: {}", trace.polys[0].len());

---

crates/prover/src/constraint_framework/logup.rs line 69 at r1 (raw file):

            prev_col_cumsum: E::EF::zero(),
            cur_frac: None,
            is_finalized: true,

?

---

crates/prover/src/constraint_framework/logup.rs line 94 at r1 (raw file):

impl<E: EvalAtRow> Drop for LogupAtRow<E> {
    fn drop(&mut self) {
        // assert!(self.is_finalized, "LogupAtRow was not finalized");

why?

[Alon-Ti]

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

---

a discussion (no related file):

Previously, shaharsamocha7 wrote…

Can you split this pr in any way?
It is both move a lot of code and do non-trivial changes such that write_frac also initialize the logup

I don't think so, making components initialize the logup means you have to pass all the params to all of them.

---

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

Previously, ohad-starkware (Ohad) wrote…

use logup sums

Done.

---

crates/prover/src/constraint_framework/logup.rs line 69 at r1 (raw file):

Previously, ohad-starkware (Ohad) wrote…

?

A logup starts finalized until it's used, when it becomes not finalized until finalize_logup is called. This is required for component that don't have logups.

---

crates/prover/src/constraint_framework/logup.rs line 94 at r1 (raw file):

Previously, ohad-starkware (Ohad) wrote…

why?

Done.

---

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

        let trace_domain = CanonicCoset::new(self.eval.log_size());

        println!("n polys: {}", trace.polys[0].len());

Done.

[ohad-starkware]

Reviewable status: 0 of 25 files reviewed, 2 unresolved discussions (waiting on @Alon-Ti and @shaharsamocha7)

[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: 92fe59a	Previous: f6214d1	Ratio
iffts/simd ifft/21	6793284 ns/iter (± 171971)	3272897 ns/iter (± 88576)	2.08
iffts/simd ifft/22	14412912 ns/iter (± 444177)	6919803 ns/iter (± 229254)	2.08
iffts/simd ifft/26	289384225 ns/iter (± 4786646)	138289295 ns/iter (± 2389141)	2.09
iffts/simd ifft/27	631584378 ns/iter (± 21418987)	312325137 ns/iter (± 5094310)	2.02
iffts/simd ifft/28	1401620822 ns/iter (± 45218845)	647142081 ns/iter (± 16823986)	2.17
merkle throughput/simd merkle	33993531 ns/iter (± 918431)	14690867 ns/iter (± 434150)	2.31

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

CC: @shaharsamocha7

[shaharsamocha7]

Reviewed 5 of 23 files at r1, 17 of 20 files at r2, 1 of 1 files at r3, 2 of 2 files at r4, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @Alon-Ti)

---

crates/prover/src/constraint_framework/info.rs line 26 at r4 (raw file):

    pub fn new(
        log_size: u32,
        preprocessed_columns: Vec<PreprocessedColumn>,

Why InfoEvaluator gets preprocessed_columns at all?
Shouldn't it be always empty at construction?

Code quote:

        preprocessed_columns: Vec<PreprocessedColumn>,

---

crates/prover/src/constraint_framework/logup.rs line 27 at r4 (raw file):

/// Should be used to eliminate padded rows for the logup sum.
pub type ClaimedPrefixSum = (SecureField, usize);
// (total_sum, claimed_sum)

Claimed_sum should be set to none if equal to total_sum

Code quote:

// (total_sum, claimed_sum)

---

crates/prover/src/constraint_framework/logup.rs line 69 at r4 (raw file):

            prev_col_cumsum: E::EF::zero(),
            cur_frac: None,
            is_finalized: true,

Write a comment that this is turned to false only at first use

Code quote:

is_finalized: true,

---

crates/prover/src/examples/blake/xor_table/gen.rs line 168 at r4 (raw file):

        })
        .to_vec();
    constant_trace.push(gen_is_first(column_bits::<ELEM_BITS, EXPAND_BITS>()));

I don't understand this

Code quote:

constant_trace.push(gen_is_first(column_bits::<ELEM_BITS, EXPAND_BITS>()));

---

crates/prover/src/constraint_framework/assert.rs line 32 at r4 (raw file):

            col_index: TreeVec::new(vec![0; trace.len()]),
            row,
            logup: LogupAtRow::new(INTERACTION_TRACE_IDX, logup_sums.0, logup_sums.1, log_size),

Should we change LogupAtRow to get LogupSums?
Can be done in another pr

Code quote:

LogupAtRow::new(INTERACTION_TRACE_IDX, logup_sums.0, logup_sums.1, log_size)

[Alon-Ti]

Merge activity

• Nov 18, 2:41 AM EST: A user merged this pull request with Graphite.