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

Fix full segment bug #844

Merged
merged 6 commits into from
Sep 7, 2023
Merged

Fix full segment bug #844

merged 6 commits into from
Sep 7, 2023

Conversation

capossele
Copy link
Contributor

@capossele capossele commented Sep 6, 2023

This PR fixes a bug occurring when calling iteratively sha2, ending up calling the expand function. The fix increases the const SHA_CYCLES from 72 to 73.
It also adds a unit test for that case.

Fixes #820

@flaub
Copy link
Member

flaub commented Sep 6, 2023

This is interesting because I was planning to drop the expand function altogether since it was written before we had an executor which could compute ahead of time how big the prover matrix needs to be. So it feels like the executor is still miscalculating something.

@github-actions
Copy link

github-actions bot commented Sep 6, 2023

Benchmark for Linux-cuda e7f4930

Click to hide benchmark
Test Base PR %
fib/100/execute 5.1±0.16ms 5.1±0.13ms 0.00%
fib/100/prove 717.3±5.26ms 665.5±3.06ms -7.22%
fib/100/total 717.8±3.22ms 670.7±2.24ms -6.56%
fib/1000/execute 5.8±0.14ms 5.7±0.14ms -1.72%
fib/1000/prove 745.0±2.42ms 696.0±1.66ms -6.58%
fib/1000/total 753.2±3.84ms 702.0±1.46ms -6.80%
fib/10000/execute 11.6±0.13ms 11.6±0.11ms 0.00%
fib/10000/prove 2.8±0.01s 2.5±0.01s -10.71%
fib/10000/total 2.8±0.00s 2.5±0.01s -10.71%

Benchmark for Linux-default e7f4930

Click to hide benchmark
Test Base PR %
fib/100/execute 6.9±0.48ms 6.8±0.49ms -1.45%
fib/100/prove 5.0±0.03s 5.0±0.02s 0.00%
fib/100/total 5.0±0.02s 5.0±0.03s 0.00%
fib/1000/execute 8.0±0.65ms 7.8±0.51ms -2.50%
fib/1000/prove 5.1±0.02s 5.0±0.03s -1.96%
fib/1000/total 5.1±0.02s 5.1±0.02s 0.00%
fib/10000/execute 14.3±0.50ms 14.0±0.58ms -2.10%
fib/10000/prove 20.7±0.10s 20.7±0.05s 0.00%
fib/10000/total 20.7±0.05s 20.7±0.12s 0.00%

Benchmark for macOS-default e7f4930

Click to hide benchmark
Test Base PR %
fib/100/execute 2.8±0.13ms 2.7±0.09ms -3.57%
fib/100/prove 3.6±0.06s 3.6±0.05s 0.00%
fib/100/total 3.6±0.04s 3.6±0.07s 0.00%
fib/1000/execute 3.0±0.05ms 3.0±0.14ms 0.00%
fib/1000/prove 3.7±0.04s 3.6±0.07s -2.70%
fib/1000/total 3.7±0.04s 3.7±0.04s 0.00%
fib/10000/execute 6.0±0.15ms 5.8±0.09ms -3.33%
fib/10000/prove 15.1±0.13s 15.0±0.07s -0.66%
fib/10000/total 15.0±0.09s 15.0±0.10s 0.00%

Benchmark for macOS-metal e7f4930

Click to hide benchmark
Test Base PR %
fib/100/execute 2.8±0.09ms 2.8±0.16ms 0.00%
fib/100/prove 799.1±6.51ms 795.8±4.92ms -0.41%
fib/100/total 819.5±3.99ms 817.0±6.92ms -0.31%
fib/1000/execute 3.0±0.08ms 2.9±0.09ms -3.33%
fib/1000/prove 817.5±3.94ms 816.3±3.39ms -0.15%
fib/1000/total 840.5±8.01ms 837.8±6.32ms -0.32%
fib/10000/execute 5.8±0.05ms 5.8±0.13ms 0.00%
fib/10000/prove 3.1±0.01s 3.1±0.01s 0.00%
fib/10000/total 3.1±0.01s 3.1±0.01s 0.00%

@capossele capossele marked this pull request as draft September 7, 2023 13:52
@github-actions
Copy link

github-actions bot commented Sep 7, 2023

Benchmark for Linux-cuda

    <details open>
      <summary>Click to hide benchmark</summary>
      Benchmarks have changed between the two branches, unable to diff.
    </details>

Benchmark for Linux-default

    <details open>
      <summary>Click to hide benchmark</summary>
      Benchmarks have changed between the two branches, unable to diff.
    </details>

Benchmark for macOS-default 7e95ed5

Click to hide benchmark
Test Base PR %
fib/100/execute 2.8±0.11ms 2.7±0.11ms -3.57%
fib/100/prove 3.7±0.05s 3.7±0.03s 0.00%
fib/100/total 3.7±0.06s 3.7±0.08s 0.00%
fib/1000/execute 3.0±0.09ms 3.0±0.05ms 0.00%
fib/1000/prove 3.7±0.07s 3.6±0.07s -2.70%
fib/1000/total 3.7±0.04s 3.6±0.07s -2.70%
fib/10000/execute 5.9±0.10ms 5.8±0.09ms -1.69%
fib/10000/prove 15.1±0.10s 15.0±0.09s -0.66%
fib/10000/total 15.1±0.16s 15.1±0.07s 0.00%

Benchmark for macOS-metal 7e95ed5

Click to hide benchmark
Test Base PR %
fib/100/execute 2.8±0.08ms 2.7±0.11ms -3.57%
fib/100/prove 818.6±4.88ms 795.1±3.57ms -2.87%
fib/100/total 821.0±3.51ms 819.0±5.36ms -0.24%
fib/1000/execute 3.0±0.05ms 2.9±0.13ms -3.33%
fib/1000/prove 842.2±4.13ms 813.8±3.97ms -3.37%
fib/1000/total 843.2±2.29ms 843.0±8.70ms -0.02%
fib/10000/execute 5.9±0.12ms 5.8±0.03ms -1.69%
fib/10000/prove 3.1±0.01s 3.1±0.01s 0.00%
fib/10000/total 3.1±0.01s 3.1±0.01s 0.00%

@capossele capossele marked this pull request as ready for review September 7, 2023 18:26
@github-actions
Copy link

github-actions bot commented Sep 7, 2023

Benchmark for Linux-cuda 2dfc852

Click to hide benchmark
Test Base PR %
fib/100/execute 5.1±0.11ms 5.0±0.11ms -1.96%
fib/100/prove 721.9±3.03ms 696.2±5.42ms -3.56%
fib/100/total 725.1±1.63ms 700.8±4.33ms -3.35%
fib/1000/execute 5.7±0.12ms 5.6±0.11ms -1.75%
fib/1000/prove 750.5±2.64ms 727.9±2.15ms -3.01%
fib/1000/total 755.9±2.44ms 736.7±3.46ms -2.54%
fib/10000/execute 11.6±0.12ms 11.1±0.13ms -4.31%
fib/10000/prove 2.9±0.01s 2.5±0.00s -13.79%
fib/10000/total 2.9±0.01s 2.5±0.01s -13.79%

Benchmark for Linux-default

    <details open>
      <summary>Click to hide benchmark</summary>
      Benchmarks have changed between the two branches, unable to diff.
    </details>

Benchmark for macOS-default 2dfc852

Click to hide benchmark
Test Base PR %
fib/100/execute 2.9±0.15ms 2.7±0.06ms -6.90%
fib/100/prove 3.6±0.08s 3.6±0.07s 0.00%
fib/100/total 3.6±0.05s 3.6±0.06s 0.00%
fib/1000/execute 3.1±0.11ms 3.0±0.07ms -3.23%
fib/1000/prove 3.7±0.06s 3.6±0.07s -2.70%
fib/1000/total 3.7±0.06s 3.6±0.07s -2.70%
fib/10000/execute 6.0±0.10ms 5.7±0.06ms -5.00%
fib/10000/prove 15.0±0.18s 15.0±0.12s 0.00%
fib/10000/total 15.0±0.15s 15.0±0.11s 0.00%

Benchmark for macOS-metal 2dfc852

Click to hide benchmark
Test Base PR %
fib/100/execute 2.8±0.05ms 2.8±0.07ms 0.00%
fib/100/prove 800.8±8.03ms 793.6±5.60ms -0.90%
fib/100/total 832.0±6.27ms 821.8±7.75ms -1.23%
fib/1000/execute 3.0±0.05ms 3.0±0.15ms 0.00%
fib/1000/prove 823.2±8.40ms 815.1±3.94ms -0.98%
fib/1000/total 842.5±3.22ms 840.9±6.07ms -0.19%
fib/10000/execute 5.8±0.08ms 5.8±0.08ms 0.00%
fib/10000/prove 3.1±0.01s 3.1±0.01s 0.00%
fib/10000/total 3.1±0.01s 3.1±0.01s 0.00%

Copy link
Member

@flaub flaub left a comment

Choose a reason for hiding this comment

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

Seems reasonable 👍

@capossele capossele merged commit 3faa248 into main Sep 7, 2023
20 checks passed
@capossele capossele deleted the capossele/fix-full-segment branch September 7, 2023 20:55
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.

Failure to split a computation consisting in iterating calls to sha256
2 participants