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

Add measurement logs #493

Merged
merged 1 commit into from
Apr 15, 2024
Merged

Conversation

spapinistarkware
Copy link
Contributor

@spapinistarkware spapinistarkware commented Mar 16, 2024

This change is Reviewable

@codecov-commenter
Copy link

codecov-commenter commented Mar 16, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.12%. Comparing base (c8552c4) to head (1b54a8c).

Additional details and impacted files
@@           Coverage Diff           @@
##              dev     #493   +/-   ##
=======================================
  Coverage   94.11%   94.12%           
=======================================
  Files          66       66           
  Lines        8379     8392   +13     
  Branches     8379     8392   +13     
=======================================
+ Hits         7886     7899   +13     
  Misses        428      428           
  Partials       65       65           

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

This was referenced Mar 27, 2024
@spapinistarkware spapinistarkware force-pushed the spapini/03-16-widefib_test_with_avx_backend branch from af26708 to 34a5a50 Compare March 28, 2024 09:44
@spapinistarkware spapinistarkware force-pushed the spapini/03-16-add_measurement_logs branch from f4b7c15 to 07a62f9 Compare March 28, 2024 09:44
@spapinistarkware spapinistarkware force-pushed the spapini/03-16-widefib_test_with_avx_backend branch from 34a5a50 to 28f7685 Compare April 3, 2024 06:22
@spapinistarkware spapinistarkware force-pushed the spapini/03-16-add_measurement_logs branch from 07a62f9 to 7240676 Compare April 3, 2024 06:22
@spapinistarkware spapinistarkware mentioned this pull request Apr 3, 2024
@spapinistarkware spapinistarkware force-pushed the spapini/03-16-widefib_test_with_avx_backend branch 5 times, most recently from b9036b8 to 19ded1d Compare April 4, 2024 09:52
@spapinistarkware spapinistarkware force-pushed the spapini/03-16-add_measurement_logs branch from 7240676 to 60bf0a0 Compare April 4, 2024 09:52
@spapinistarkware spapinistarkware force-pushed the spapini/03-16-widefib_test_with_avx_backend branch from 19ded1d to bcc3f59 Compare April 4, 2024 12:20
@spapinistarkware spapinistarkware force-pushed the spapini/03-16-add_measurement_logs branch from 60bf0a0 to c048950 Compare April 4, 2024 12:20
@spapinistarkware spapinistarkware force-pushed the spapini/03-16-widefib_test_with_avx_backend branch from bcc3f59 to 4b151b8 Compare April 4, 2024 12:26
@spapinistarkware spapinistarkware force-pushed the spapini/03-16-add_measurement_logs branch from c048950 to c61c35b Compare April 4, 2024 12:26
@spapinistarkware spapinistarkware force-pushed the spapini/03-16-add_measurement_logs branch from c61c35b to 82a86ae Compare April 15, 2024 08:56
@spapinistarkware spapinistarkware changed the base branch from spapini/03-16-widefib_test_with_avx_backend to dev April 15, 2024 08:56
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 9 of 9 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @spapinistarkware)


src/examples/wide_fibonacci/avx.rs line 39 at r1 (raw file):

    log_size: usize,
) -> ColumnVec<CircleEvaluation<AVX512Backend, BaseField, BitReversedOrder>> {
    let _span = span!(Level::INFO, "Trace generation").entered();

Can you move it to the caller?

let trace = self.get_trace();

Code quote:

  let _span = span!(Level::INFO, "Trace generation").entered();

@spapinistarkware spapinistarkware force-pushed the spapini/03-16-add_measurement_logs branch 2 times, most recently from eeb1ad0 to c79fbdb Compare April 15, 2024 11:01
Copy link
Contributor Author

@spapinistarkware spapinistarkware 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: 7 of 11 files reviewed, 1 unresolved discussion (waiting on @shaharsamocha7)


src/examples/wide_fibonacci/avx.rs line 39 at r1 (raw file):

Previously, shaharsamocha7 wrote…

Can you move it to the caller?

let trace = self.get_trace();

Done.

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 4 of 4 files at r2.
Reviewable status: 10 of 11 files reviewed, 3 unresolved discussions (waiting on @spapinistarkware)


src/core/commitment_scheme/prover.rs line 163 at r2 (raw file):

        span.exit();

        let _span = span!(Level::INFO, "Commitment merkle").entered();

Why do you need to bind it to a variable?

Code quote:

let _span = 

src/examples/wide_fibonacci/avx.rs line 74 at r2 (raw file):

        evaluation_accumulator: &mut DomainEvaluationAccumulator<AVX512Backend>,
    ) {
        let span = span!(Level::INFO, "Constraint eval extension").entered();

This is not just the extension here.

Code quote:

 let span = span!(Level::INFO, "Constraint eval extension").entered();

src/examples/wide_fibonacci/avx.rs line 170 at r3 (raw file):

    fn test_avx_wide_fib_prove() {
        // Note: To see time measurement, run test with
        //   RUST_LOG_SPAN_EVENTS=enter,close RUST_LOG=info RUST_BACKTRACE=1 RUSTFLAGS="-Awarnings

Why do you have warnings here?

Code quote:

RUSTFLAGS="-Awarnings

Copy link
Contributor Author

@spapinistarkware spapinistarkware 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: 10 of 11 files reviewed, 3 unresolved discussions (waiting on @shaharsamocha7)


src/core/commitment_scheme/prover.rs line 163 at r2 (raw file):

Previously, shaharsamocha7 wrote…

Why do you need to bind it to a variable?

I think it's more consistent with the let span= calls.


src/examples/wide_fibonacci/avx.rs line 74 at r2 (raw file):

Previously, shaharsamocha7 wrote…

This is not just the extension here.

Done.


src/examples/wide_fibonacci/avx.rs line 170 at r3 (raw file):

Previously, shaharsamocha7 wrote…

Why do you have warnings here?

Done.

@spapinistarkware spapinistarkware force-pushed the spapini/03-16-add_measurement_logs branch from c79fbdb to 1b54a8c Compare April 15, 2024 11:10
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.

:lgtm:

Reviewed 2 of 2 files at r4, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @spapinistarkware)

Copy link
Contributor Author

spapinistarkware commented Apr 15, 2024

Merge activity

@spapinistarkware spapinistarkware merged commit a8d128b into dev Apr 15, 2024
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.

3 participants