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

simple merkle benchmark #527

Merged
merged 1 commit into from
Apr 3, 2024

Conversation

spapinistarkware
Copy link
Contributor

@spapinistarkware spapinistarkware commented Mar 22, 2024

This change is Reviewable

This was referenced Mar 22, 2024
Copy link
Contributor Author

spapinistarkware commented Mar 22, 2024

@spapinistarkware spapinistarkware mentioned this pull request Mar 22, 2024
@spapinistarkware spapinistarkware force-pushed the spapini/03-22-simple_merkle_benchmark branch from 3119d84 to 6120a30 Compare March 22, 2024 14:30
This was referenced Mar 22, 2024
@spapinistarkware spapinistarkware force-pushed the spapini/03-22-use_simple_merkle_tree branch from c12eaf0 to 8224e9a Compare March 24, 2024 13:48
@spapinistarkware spapinistarkware force-pushed the spapini/03-22-simple_merkle_benchmark branch from 6120a30 to 36068f0 Compare March 24, 2024 13:48
@codecov-commenter
Copy link

codecov-commenter commented Mar 24, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.19%. Comparing base (41a66ec) to head (f76f6e5).

Additional details and impacted files
@@                          Coverage Diff                           @@
##           spapini/03-22-use_simple_merkle_tree     #527    +/-   ##
======================================================================
  Coverage                                 95.19%   95.19%            
======================================================================
  Files                                        66       66            
  Lines                                     10327    10062   -265     
  Branches                                  10327    10062   -265     
======================================================================
- Hits                                       9831     9579   -252     
+ Misses                                      427      416    -11     
+ Partials                                     69       67     -2     

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

@spapinistarkware spapinistarkware force-pushed the spapini/03-22-use_simple_merkle_tree branch from 8224e9a to b0c69c9 Compare March 24, 2024 15:07
@spapinistarkware spapinistarkware force-pushed the spapini/03-22-simple_merkle_benchmark branch from 36068f0 to f596373 Compare March 24, 2024 15:07
@spapinistarkware spapinistarkware force-pushed the spapini/03-22-use_simple_merkle_tree branch from b0c69c9 to 8cc9919 Compare March 25, 2024 05:20
@spapinistarkware spapinistarkware force-pushed the spapini/03-22-simple_merkle_benchmark branch from f596373 to 12ea9d7 Compare March 25, 2024 05:20
@spapinistarkware spapinistarkware force-pushed the spapini/03-22-use_simple_merkle_tree branch from 8cc9919 to 98e4d7a Compare March 25, 2024 06:05
@spapinistarkware spapinistarkware force-pushed the spapini/03-22-simple_merkle_benchmark branch from 12ea9d7 to a551a09 Compare March 25, 2024 06:05
@spapinistarkware spapinistarkware force-pushed the spapini/03-22-use_simple_merkle_tree branch from 98e4d7a to 05397c7 Compare March 25, 2024 09:21
@spapinistarkware spapinistarkware force-pushed the spapini/03-22-simple_merkle_benchmark branch from a551a09 to 86f7c62 Compare March 25, 2024 09:21
@spapinistarkware spapinistarkware force-pushed the spapini/03-22-use_simple_merkle_tree branch 2 times, most recently from b8935cc to 16ee392 Compare March 25, 2024 12:00
@spapinistarkware spapinistarkware force-pushed the spapini/03-22-simple_merkle_benchmark branch from 86f7c62 to 1a950fc Compare March 25, 2024 12:00
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 1 of 4 files at r1, 2 of 2 files at r2, all commit messages.
Reviewable status: 3 of 4 files reviewed, 3 unresolved discussions (waiting on @spapinistarkware)


Cargo.toml line 16 at r2 (raw file):

merging-iterator = "1.3.0"
bytemuck = { version = "1.14.3", features = ["derive"] }
blake2 = "0.10.6"

Suggestion:

[dependencies]
blake2 = "0.10.6"
blake3 = "1.5.0"
hex = "0.4.3"
itertools = "0.12.0"
num-traits = "0.2.17"
thiserror = "1.0.56"
merging-iterator = "1.3.0"
bytemuck = { version = "1.14.3", features = ["derive"] }

Cargo.toml line 60 at r2 (raw file):

[[bench]]
name = "merkle"

What is the difference?
Rename or put them together

Code quote:

[[bench]]
name = "merkle_bench"
harness = false

[[bench]]
name = "merkle"

benches/merkle_bench.rs line 97 at r2 (raw file):

        )
    });
}

Why did you remove this?
Do we need any benches from this file?

Code quote:

fn single_blake2s_hash_benchmark(c: &mut Criterion) {
    let input = [0u8; 1];
    c.bench_function("Single blake2s hash", |b| {
        b.iter_batched(
            || -> Blake2s256 { Blake2s256::new() },
            |mut h| {
                h.update(&input[..]);
                h.finalize()
            },
            BatchSize::SmallInput,
        )
    });
}

@spapinistarkware spapinistarkware force-pushed the spapini/03-22-use_simple_merkle_tree branch from 16ee392 to 67cf290 Compare March 26, 2024 06:31
@spapinistarkware spapinistarkware force-pushed the spapini/03-22-simple_merkle_benchmark branch 2 times, most recently from 01bea4e to e5f2229 Compare March 26, 2024 06:37
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: 3 of 4 files reviewed, 3 unresolved discussions (waiting on @shaharsamocha7)


Cargo.toml line 16 at r2 (raw file):

merging-iterator = "1.3.0"
bytemuck = { version = "1.14.3", features = ["derive"] }
blake2 = "0.10.6"

Done.


Cargo.toml line 60 at r2 (raw file):

Previously, shaharsamocha7 wrote…

What is the difference?
Rename or put them together

merkle_bench will be removed soon


benches/merkle_bench.rs line 97 at r2 (raw file):

Previously, shaharsamocha7 wrote…

Why did you remove this?
Do we need any benches from this file?

Done.

@spapinistarkware spapinistarkware force-pushed the spapini/03-22-use_simple_merkle_tree branch from 67cf290 to 1e5694b Compare March 26, 2024 06:42
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 r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @spapinistarkware)


benches/merkle_bench.rs line 2 at r3 (raw file):

use blake2::{Blake2s256, Digest};
// TODO(Ohad): write better benchmarks. Reduce the variance in sample size.

Revert change

Suggestion:

// TODO(Ohad): write better benchmarks. Reduce the variance in sample size.
use blake2::{Blake2s256, Digest};

@spapinistarkware spapinistarkware force-pushed the spapini/03-22-use_simple_merkle_tree branch from 1e5694b to a0261dc Compare March 27, 2024 14:05
@spapinistarkware spapinistarkware force-pushed the spapini/03-22-simple_merkle_benchmark branch from e5f2229 to ce2ef61 Compare March 27, 2024 15:05
@spapinistarkware spapinistarkware force-pushed the spapini/03-22-use_simple_merkle_tree branch from a0261dc to 58af368 Compare March 28, 2024 09:43
@spapinistarkware spapinistarkware force-pushed the spapini/03-22-simple_merkle_benchmark branch from ce2ef61 to 1e46394 Compare March 28, 2024 09:43
@spapinistarkware spapinistarkware force-pushed the spapini/03-22-use_simple_merkle_tree branch from 58af368 to 1f258dc Compare April 2, 2024 06:23
@spapinistarkware spapinistarkware force-pushed the spapini/03-22-simple_merkle_benchmark branch from 1e46394 to 80898a7 Compare April 2, 2024 06:27
@spapinistarkware spapinistarkware force-pushed the spapini/03-22-use_simple_merkle_tree branch from 1f258dc to 80d5e43 Compare April 2, 2024 11:26
@spapinistarkware spapinistarkware force-pushed the spapini/03-22-simple_merkle_benchmark branch from 80898a7 to 5f2a9e6 Compare April 2, 2024 11:26
@spapinistarkware spapinistarkware force-pushed the spapini/03-22-use_simple_merkle_tree branch from 80d5e43 to 41a66ec Compare April 3, 2024 06:21
@spapinistarkware spapinistarkware force-pushed the spapini/03-22-simple_merkle_benchmark branch from 5f2a9e6 to f76f6e5 Compare April 3, 2024 06:22
@spapinistarkware spapinistarkware changed the base branch from spapini/03-22-use_simple_merkle_tree to dev April 3, 2024 06:29
@spapinistarkware spapinistarkware force-pushed the spapini/03-22-simple_merkle_benchmark branch from f76f6e5 to 1c7e4a0 Compare April 3, 2024 06:30
Copy link
Contributor Author

spapinistarkware commented Apr 3, 2024

Merge activity

@spapinistarkware spapinistarkware merged commit 9298cc9 into dev Apr 3, 2024
11 of 12 checks passed
@spapinistarkware spapinistarkware mentioned this pull request Apr 3, 2024
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