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.

Sign up for GitHub

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

Jump to bottom

hash bench #77

Closed
wants to merge 1 commit into from
Closed

hash bench #77

wants to merge 1 commit into from

Conversation

hackaugusto
Copy link
Contributor

Describe your changes

A few changes to the hash benchmarks done while reviewing #76

Ref: #76 (comment)

Checklist before requesting a review

  • Repo forked and branch created from next according to naming convention.
  • Commit messages and codestyle follow conventions.
  • Relevant issues are linked in the PR description.
  • Tests added for new functionality.
  • Documentation/comments updated according to changes.

@bobbinth bobbinth changed the base branch from main to next February 24, 2023 18:13
@hackaugusto hackaugusto mentioned this pull request Feb 28, 2023
Merged
@bobbinth bobbinth mentioned this pull request Mar 1, 2023
Merged
5 tasks
@bobbinth bobbinth mentioned this pull request Apr 2, 2023
Merged
8 tasks
@hackaugusto
Copy link
Contributor Author

@bobbinth let me know if there is anything to be done for this PR to get it merged on v0.3 :)

bobbinth
bobbinth previously requested changes Apr 4, 2023
View reviewed changes
Copy link
Contributor

@bobbinth bobbinth left a comment

Choose a reason for hiding this comment

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

Thank you! I left some small comment inline.

I guess the bigger question is whether these show results which are any different from what we have here. If so, we'll need to figure out what to understand what drives the discrepancy and potentially re-run the benchmarks on various platforms to update the table.

benches/hash.rs Outdated
)
});
for size in BATCH_SIZES {
let v: Vec<Felt> = (0..size).map(|_| rand_value::<Felt>()).collect();
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not use rand_vector() function here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed to rand_vector

benches/hash.rs Outdated
let v: Vec<Felt> = (0..size).map(|_| rand_value::<Felt>()).collect();

group.bench_with_input(
BenchmarkId::new(format!("RPO256/{size}"), size),
Copy link
Contributor

Choose a reason for hiding this comment

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

I think doing /{size} is redundant here. We could probably just do: BenchmarkId::new("RPO256", size).

benches/hash.rs Outdated
);

group.bench_with_input(
BenchmarkId::new(format!("Blake3/256/{size}"), size),
Copy link
Contributor

Choose a reason for hiding this comment

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

I think doing /{size} is redundant here. We could probably just do: BenchmarkId::new("Blake3_256", size).

benches/hash.rs Outdated
Comment on lines 12 to 21
static BATCH_SIZES: [usize; 8] = [
2usize.pow(0),
2usize.pow(1),
2usize.pow(2),
2usize.pow(3),
2usize.pow(4),
2usize.pow(5),
2usize.pow(6),
2usize.pow(7),
];
Copy link
Contributor

@bobbinth bobbinth Apr 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

We probably don't need as many batches. I'd probably do the following:

static BATCH_SIZES: [usize; 8] = [
    16,
    32,
    64,
    100,
    256,
];

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed

benches/hash.rs Outdated
[Blake3_256::hash(&[1_u8]), Blake3_256::hash(&[2_u8])];
c.bench_function("Blake3 2-to-1 hashing (cached)", |bench| {
bench.iter(|| Blake3_256::merge(black_box(&v)))
group.bench_with_input(BenchmarkId::new("Blake3/256", 1), &s2, |b, i| {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I'd change benchmark ID to: BenchmarkId::new("Blake3_256", 1)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed all / to _

benches/hash.rs
// Benchmark `merge` in a hot loop with varying number of elements, the goal is to spend more
// time in the hashing function so that the cost of measuring it will dissipiate in the
// background and give better view of the algorithms performance.
for size in BATCH_SIZES {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it is fine if we test it on just a couple of sizes - e.g, 64 and 256.

Copy link
Contributor Author

@hackaugusto hackaugusto Jun 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

This request conflicts with the one above: https://github.com/0xPolygonMiden/crypto/pull/77/files#r1156632675

I kept the list above. Let me know if it should change.

Note that exercising different sizes to fill the L1/L2/L3 caches is important for reliability of the results. (and the size as mentioned in the comment above)

benches/hash.rs Outdated
.map(|[l, r]| [l.as_bytes().into(), r.as_bytes().into()])
.collect();
group.bench_with_input(
BenchmarkId::new(format!("Blake3/256/{size}"), size),
Copy link
Contributor

Choose a reason for hiding this comment

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

I think doing /{size} is redundant here. We could probably just do: BenchmarkId::new("Blake3_256", size).

benches/hash.rs Outdated
.collect();

group.bench_with_input(
BenchmarkId::new(format!("RPO256/{size}"), size),
Copy link
Contributor

Choose a reason for hiding this comment

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

I think doing /{size} is redundant here. We could probably just do: BenchmarkId::new("RPO256", size).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed the size of all ids

@bobbinth bobbinth mentioned this pull request Apr 9, 2023
Merged
12 tasks
@bobbinth bobbinth mentioned this pull request Apr 21, 2023
Merged
12 tasks
@bobbinth bobbinth mentioned this pull request May 26, 2023
Merged
9 tasks
@hackaugusto hackaugusto force-pushed the hacka-hash-bench branch 2 times, most recently from 59e6822 to 9ddba07 Compare June 18, 2023 19:10
@hackaugusto hackaugusto dismissed bobbinth’s stale review June 23, 2023 06:20

applied requested changes

@bobbinth bobbinth mentioned this pull request Jun 25, 2023
Merged
10 tasks
@bobbinth bobbinth mentioned this pull request Oct 12, 2023
Merged
12 tasks
@hackaugusto hackaugusto deleted the hacka-hash-bench branch October 27, 2023 13:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vlopes11 vlopes11 Awaiting requested review from vlopes11

@bobbinth bobbinth Awaiting requested review from bobbinth

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@hackaugusto @bobbinth