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

Remove thiserror from risc0-zkp #704

Merged
merged 11 commits into from
Jul 17, 2023
Merged

Remove thiserror from risc0-zkp #704

merged 11 commits into from
Jul 17, 2023

Conversation

flaub
Copy link
Member

@flaub flaub commented Jul 16, 2023

Fix for #694

@flaub flaub requested review from mothran and SchmErik July 16, 2023 09:34
@flaub flaub self-assigned this Jul 16, 2023
@github-actions
Copy link

Benchmark for Linux-cuda cc9b84f

Click to hide benchmark
Test Base PR %
fib/100/execute 5.0±0.10ms 5.0±0.08ms 0.00%
fib/100/prove 1752.4±23.24ms 938.8±19.03ms -46.43%
fib/100/total 1656.2±84.88ms 932.5±26.46ms -43.70%
fib/1000/execute 5.5±0.11ms 5.5±0.07ms 0.00%
fib/1000/prove 1808.1±117.61ms 978.4±27.88ms -45.89%
fib/1000/total 1690.8±49.98ms 993.1±32.50ms -41.26%
fib/10000/execute 9.8±0.10ms 9.8±0.18ms 0.00%
fib/10000/prove 6.0±0.06s 4.0±0.02s -33.33%
fib/10000/total 5.6±0.04s 4.1±0.05s -26.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 cc9b84f

Click to hide benchmark
Test Base PR %
fib/100/execute 2.8±0.12ms 2.7±0.08ms -3.57%
fib/100/prove 3.7±0.06s 3.7±0.06s 0.00%
fib/100/total 3.7±0.04s 3.7±0.05s 0.00%
fib/1000/execute 3.0±0.08ms 2.9±0.11ms -3.33%
fib/1000/prove 3.7±0.06s 3.7±0.06s 0.00%
fib/1000/total 3.7±0.06s 3.7±0.05s 0.00%
fib/10000/execute 5.1±0.10ms 5.1±0.11ms 0.00%
fib/10000/prove 15.2±0.10s 15.2±0.08s 0.00%
fib/10000/total 15.3±0.18s 15.3±0.05s 0.00%

Benchmark for macOS-metal

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

@flaub flaub enabled auto-merge (squash) July 16, 2023 19:02
@github-actions
Copy link

Benchmark for Linux-cuda adf6105

Click to hide benchmark
Test Base PR %
fib/100/execute 5.1±0.12ms 5.1±0.11ms 0.00%
fib/100/prove 1033.0±24.68ms 857.4±1.96ms -17.00%
fib/100/total 1020.0±10.26ms 863.6±1.99ms -15.33%
fib/1000/execute 5.5±0.10ms 5.5±0.12ms 0.00%
fib/1000/prove 1072.4±26.29ms 895.3±1.28ms -16.51%
fib/1000/total 1068.7±18.25ms 901.9±1.53ms -15.61%
fib/10000/execute 9.8±0.12ms 9.8±0.14ms 0.00%
fib/10000/prove 4.1±0.01s 3.4±0.00s -17.07%
fib/10000/total 4.1±0.02s 3.4±0.01s -17.07%

Benchmark for Linux-default adf6105

Click to hide benchmark
Test Base PR %
fib/100/execute 4.9±0.37ms 4.8±0.13ms -2.04%
fib/100/prove 2.5±0.35s 2.4±0.39s -4.00%
fib/100/total 2.6±0.55s 2.4±0.48s -7.69%
fib/1000/execute 5.2±0.09ms 5.1±0.12ms -1.92%
fib/1000/prove 2.6±0.40s 2.3±0.36s -11.54%
fib/1000/total 2.5±0.34s 2.5±0.52s 0.00%
fib/10000/execute 9.4±0.96ms 8.9±0.13ms -5.32%
fib/10000/prove 7.5±0.41s 7.0±0.22s -6.67%
fib/10000/total 9.3±0.64s 7.4±0.55s -20.43%

Benchmark for macOS-default adf6105

Click to hide benchmark
Test Base PR %
fib/100/execute 2.9±0.12ms 2.8±0.15ms -3.45%
fib/100/prove 3.7±0.09s 3.7±0.05s 0.00%
fib/100/total 3.7±0.05s 3.7±0.08s 0.00%
fib/1000/execute 2.9±0.09ms 2.9±0.07ms 0.00%
fib/1000/prove 3.7±0.06s 3.7±0.06s 0.00%
fib/1000/total 3.8±0.05s 3.7±0.06s -2.63%
fib/10000/execute 5.2±0.10ms 5.1±0.11ms -1.92%
fib/10000/prove 15.4±0.17s 15.2±0.14s -1.30%
fib/10000/total 15.3±0.09s 15.3±0.08s 0.00%

Benchmark for macOS-metal

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

@github-actions
Copy link

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

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

Benchmark for macOS-metal 14c9460

Click to hide benchmark
Test Base PR %
fib/100/execute 2.8±0.08ms 2.7±0.04ms -3.57%
fib/100/prove 850.7±3.88ms 847.9±3.79ms -0.33%
fib/100/total 879.9±7.40ms 868.6±5.56ms -1.28%
fib/1000/execute 3.0±0.16ms 2.9±0.07ms -3.33%
fib/1000/prove 872.1±4.01ms 867.4±4.73ms -0.54%
fib/1000/total 898.4±5.52ms 890.5±5.74ms -0.88%
fib/10000/execute 5.1±0.08ms 5.0±0.08ms -1.96%
fib/10000/prove 3.3±0.01s 3.3±0.01s 0.00%
fib/10000/total 3.3±0.01s 3.3±0.02s 0.00%

@Brando753
Copy link

Have you considered instead of out right removing thiserror to create a std and no_std option?

For example in code that I want to support no_std environments I would add:

[features]
default = ["std"]
std = ["thiserror"]

[dependencies]
thiserror = { version = "1.0.40", optional = true}

And then I would use the std features like so:

#![cfg_attr(not(feature = "std"), no_std)]

#[cfg(not(feature = "std"))]
extern crate alloc;

#[cfg(feature = "std")]
use thiserror::Error;

#[derive(Debug, PartialEq)]
#[cfg_attr(feature = "std", derive(Error))]
pub enum CustomError {
    /// The Prover was unable to construct a proof with your given inputs
    #[cfg_attr(
        feature = "std",
        error("The prover was unable to construct a proof with your given inputs")
    )]
    UnableToProve,
}

@flaub
Copy link
Member Author

flaub commented Jul 17, 2023

@Brando753 I'd like to have support for error messages in no_std as well. It's not much boilerplate and if it becomes onerous we could switch to snafu.

@Brando753
Copy link

Understood; looking over the changes, it's not that bad. If you don't have too many error structs, then it's probably not worth worrying about. I don't see the no_std crate attribute. Are you planning to add that?

@github-actions
Copy link

Benchmark for Linux-cuda b761214

Click to hide benchmark
Test Base PR %
fib/100/execute 5.0±0.11ms 5.0±0.08ms 0.00%
fib/100/prove 1287.7±16.26ms 1133.0±58.32ms -12.01%
fib/100/total 1271.7±17.06ms 1122.9±81.70ms -11.70%
fib/1000/execute 5.5±0.11ms 5.5±0.08ms 0.00%
fib/1000/prove 1312.4±17.40ms 1167.5±66.82ms -11.04%
fib/1000/total 1322.3±15.04ms 1157.9±76.85ms -12.43%
fib/10000/execute 10.0±0.12ms 9.7±0.14ms -3.00%
fib/10000/prove 4.4±0.02s 4.3±0.01s -2.27%
fib/10000/total 4.3±0.01s 4.3±0.01s 0.00%

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 b761214

Click to hide benchmark
Test Base PR %
fib/100/execute 2.8±0.19ms 2.8±0.12ms 0.00%
fib/100/prove 3.7±0.08s 3.7±0.07s 0.00%
fib/100/total 3.7±0.05s 3.7±0.06s 0.00%
fib/1000/execute 3.0±0.13ms 3.0±0.06ms 0.00%
fib/1000/prove 3.7±0.07s 3.7±0.09s 0.00%
fib/1000/total 3.7±0.06s 3.7±0.06s 0.00%
fib/10000/execute 5.2±0.08ms 5.1±0.06ms -1.92%
fib/10000/prove 15.3±0.13s 15.3±0.18s 0.00%
fib/10000/total 15.4±0.16s 15.3±0.17s -0.65%

Benchmark for macOS-metal b761214

Click to hide benchmark
Test Base PR %
fib/100/execute 2.8±0.09ms 2.8±0.05ms 0.00%
fib/100/prove 849.3±6.38ms 848.8±4.17ms -0.06%
fib/100/total 882.6±12.39ms 878.0±9.79ms -0.52%
fib/1000/execute 2.9±0.14ms 2.9±0.04ms 0.00%
fib/1000/prove 871.8±5.06ms 870.7±4.60ms -0.13%
fib/1000/total 906.3±7.43ms 897.2±7.51ms -1.00%
fib/10000/execute 5.1±0.07ms 5.0±0.08ms -1.96%
fib/10000/prove 3.4±0.01s 3.3±0.01s -2.94%
fib/10000/total 3.4±0.01s 3.3±0.01s -2.94%

@github-actions
Copy link

Benchmark for Linux-cuda b88f6f2

Click to hide benchmark
Test Base PR %
fib/100/execute 5.0±0.10ms 5.0±0.09ms 0.00%
fib/100/prove 1296.5±13.31ms 1272.0±7.44ms -1.89%
fib/100/total 1302.7±12.08ms 1270.0±13.40ms -2.51%
fib/1000/execute 5.5±0.10ms 5.5±0.10ms 0.00%
fib/1000/prove 1332.5±11.48ms 1302.6±12.28ms -2.24%
fib/1000/total 1342.5±11.09ms 1312.4±11.94ms -2.24%
fib/10000/execute 10.0±0.12ms 9.7±0.09ms -3.00%
fib/10000/prove 4.6±0.07s 4.5±0.10s -2.17%
fib/10000/total 4.7±0.09s 4.6±0.06s -2.13%

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 b88f6f2

Click to hide benchmark
Test Base PR %
fib/100/execute 2.7±0.08ms 2.7±0.08ms 0.00%
fib/100/prove 3.7±0.06s 3.7±0.06s 0.00%
fib/100/total 3.7±0.05s 3.7±0.10s 0.00%
fib/1000/execute 3.0±0.11ms 3.0±0.09ms 0.00%
fib/1000/prove 3.7±0.06s 3.7±0.06s 0.00%
fib/1000/total 3.7±0.06s 3.7±0.04s 0.00%
fib/10000/execute 5.1±0.06ms 5.1±0.04ms 0.00%
fib/10000/prove 15.3±0.16s 15.3±0.10s 0.00%
fib/10000/total 15.3±0.13s 15.2±0.07s -0.65%

Benchmark for macOS-metal b88f6f2

Click to hide benchmark
Test Base PR %
fib/100/execute 2.8±0.09ms 2.8±0.16ms 0.00%
fib/100/prove 852.2±6.63ms 851.6±4.70ms -0.07%
fib/100/total 873.1±7.51ms 868.6±4.53ms -0.52%
fib/1000/execute 2.9±0.05ms 2.9±0.04ms 0.00%
fib/1000/prove 877.2±5.27ms 865.2±5.25ms -1.37%
fib/1000/total 895.1±7.15ms 888.4±9.17ms -0.75%
fib/10000/execute 5.0±0.07ms 5.0±0.10ms 0.00%
fib/10000/prove 3.3±0.01s 3.3±0.01s 0.00%
fib/10000/total 3.3±0.01s 3.3±0.01s 0.00%

@flaub flaub disabled auto-merge July 17, 2023 07:37
@flaub flaub merged commit 9813eb6 into main Jul 17, 2023
12 checks passed
@flaub flaub deleted the flaub/no_std branch July 17, 2023 07:38
@flaub
Copy link
Member Author

flaub commented Jul 17, 2023

Understood; looking over the changes, it's not that bad. If you don't have too many error structs, then it's probably not worth worrying about. I don't see the no_std crate attribute. Are you planning to add that?

See: https://github.com/risc0/risc0/blob/main/risc0/zkp/src/lib.rs#L16

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.

None yet

2 participants