Skip to content

Conversation

@leo-starkware
Copy link
Contributor

No description provided.

@reviewable-StarkWare
Copy link

This change is Reviewable

Copy link
Contributor Author

leo-starkware commented Dec 21, 2025

@codecov-commenter
Copy link

codecov-commenter commented Dec 21, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 91.97%. Comparing base (47186dd) to head (5221f96).

Additional details and impacted files
@@                 Coverage Diff                  @@
##           leo/fix_nonparallel    #1288   +/-   ##
====================================================
  Coverage                91.97%   91.97%           
====================================================
  Files                      119      119           
  Lines                    14958    14966    +8     
  Branches                 14958    14966    +8     
====================================================
+ Hits                     13757    13765    +8     
  Misses                    1111     1111           
  Partials                    90       90           

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Contributor

@Gali-StarkWare Gali-StarkWare left a comment

Choose a reason for hiding this comment

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

@Gali-StarkWare reviewed 2 files and all commit messages, and made 2 comments.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @leo-starkware).


crates/stwo/src/prover/pcs/mod.rs line 128 at r1 (raw file):

                // TODO(Leo): the computation `point.repeated_double(max_log_size - log_size)` is
                // likely repeated a bunch of times in a typical flat air. Consider moving it
                // outside the loop.

I guess it doesn't matter in the parallel case, right?

Code quote:

                // TODO(Leo): the computation `point.repeated_double(max_log_size - log_size)` is
                // likely repeated a bunch of times in a typical flat air. Consider moving it
                // outside the loop.

crates/stwo/src/prover/pcs/quotient_ops.rs line 221 at r1 (raw file):

    }

    fn prove_and_verify_pcs<B: BackendForChannel<Blake2sMerkleChannel>, const STORE_COEFF: bool>(

Suggestion:

STORE_COEFFS

Copy link
Contributor Author

@leo-starkware leo-starkware left a comment

Choose a reason for hiding this comment

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

@leo-starkware made 2 comments.
Reviewable status: 1 of 2 files reviewed, 1 unresolved discussion (waiting on @Gali-StarkWare).


crates/stwo/src/prover/pcs/mod.rs line 128 at r1 (raw file):

Previously, Gali-StarkWare wrote…

I guess it doesn't matter in the parallel case, right?

it also matters in parallel, in the sense that some threads could save this computation. But to do it one would need a map that is shareable among threads, not sure if worth the effort.


crates/stwo/src/prover/pcs/quotient_ops.rs line 221 at r1 (raw file):

    }

    fn prove_and_verify_pcs<B: BackendForChannel<Blake2sMerkleChannel>, const STORE_COEFF: bool>(

done

Copy link
Contributor

@Gali-StarkWare Gali-StarkWare left a comment

Choose a reason for hiding this comment

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

@Gali-StarkWare reviewed 1 file and all commit messages, made 1 comment, and resolved 1 discussion.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @leo-starkware).


crates/stwo/src/prover/pcs/mod.rs line 128 at r1 (raw file):

Previously, leo-starkware wrote…

it also matters in parallel, in the sense that some threads could save this computation. But to do it one would need a map that is shareable among threads, not sure if worth the effort.

Yes, but the total time would not change even if we had another dashmap..
The non-parallel case is much less efficient, but I'm not sure we care about it (@gilbens-starkware wdyt?)

Copy link
Contributor

@Gali-StarkWare Gali-StarkWare left a comment

Choose a reason for hiding this comment

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

:lgtm:

@Gali-StarkWare made 1 comment.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @leo-starkware).

@leo-starkware leo-starkware force-pushed the leo/modify_build_weights_0 branch from 1b3066c to 5221f96 Compare December 21, 2025 12:55
@leo-starkware leo-starkware changed the base branch from leo/fix_nonparallel to lifted December 21, 2025 14:01
@leo-starkware leo-starkware force-pushed the leo/modify_build_weights_0 branch from 5221f96 to 55445cb Compare December 21, 2025 14:02
@leo-starkware leo-starkware merged commit 1ec0705 into lifted Dec 21, 2025
22 checks passed
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