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

Bench improvement #1342

Closed
wants to merge 5 commits into from
Closed

Bench improvement #1342

wants to merge 5 commits into from

Conversation

evgenykuzyakov
Copy link
Collaborator

Switch bencher to criterion. Update sum_n to do work instead of running pre-compiled code

Copy link
Contributor

@MaksymZavershynskyi MaksymZavershynskyi left a comment

Choose a reason for hiding this comment

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

Please add the report itself (including html) so that we can diff it whenever we have a change. Also, exclude the report from the repo stats using gitattributes.

}
sum /= n as u128;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this line needed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Otherwise it overfits u64 for larger sums

expected_value += (i * i) as u128;
}
expected_value /= n as u128;
group.bench_function(BenchmarkId::new("wasm", benchmark_param_display.clone()), |b| {
Copy link
Contributor

Choose a reason for hiding this comment

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

benchmark id should include name of the function, e.g. "sum_n"?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

the group has the name

@MaksymZavershynskyi MaksymZavershynskyi added the A-transaction-runtime Area: transaction runtime (transaction and receipts processing, state transition, etc) label Nov 20, 2019
@MaksymZavershynskyi
Copy link
Contributor

@evgenykuzyakov Is this an abandoned PR? Should we close it?

@evgenykuzyakov
Copy link
Collaborator Author

Sorry. It still worth to complete it. I just didn't get back to address the changes. Will take a look later this week.

@MaksymZavershynskyi
Copy link
Contributor

@evgenykuzyakov Did you have a chance to look at it?

@ilblackdragon
Copy link
Member

@evgenykuzyakov ping?

@MaksymZavershynskyi
Copy link
Contributor

We can probably close it since now we have runtime-params-estimator.

@evgenykuzyakov
Copy link
Collaborator Author

It's also less useful, since it just runs wasm in isolation.

@bowenwang1996 bowenwang1996 deleted the bench-improvement branch April 27, 2020 23:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-transaction-runtime Area: transaction runtime (transaction and receipts processing, state transition, etc)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants