Bench overview #325
Bench overview #325
Conversation
There was a problem hiding this comment.
Things look good, I approve. I had one nit about a typo, and I left, some optional minor comments about var naming.
optional PR-level comment: Maybe update the wiki to better reflect the verbiage in the PR description. The PR description mentions overview and detailed (<- with the inline code formatting), making me think that those are terms. The wiki doesn't actually formally define those terms, though, even though there are descriptions of "overview" and "detailed benchmarks".
| @@ -1,3 +1,24 @@ | |||
| #[macro_export] | |||
| macro_rules! overview { | |||
| ($c:expr, $struct:ident, $data_str:expr, $compare:expr) => { | |||
There was a problem hiding this comment.
optional: the first argument to the macro is a Criterion benchmark group (according to what I see in the next file), so $c seems cryptic.
There was a problem hiding this comment.
in criterion we often use c to refer to either the group or the testsuite, all other macros use $c - I'll leave the rename to a PR for all macros at the same time.
| // This benchmark focuses on short numbers and performs: | ||
| // * Construction of FixedDecimals from isize | ||
| // * Construction of FixedDecimals from strings | ||
| // * Serialization of FixedDEcimal to string |
There was a problem hiding this comment.
nit: FixedDEcimal -> FixedDecimal
zbraniecki
force-pushed
the
bench-overview
branch
from
4bf8c77 to
308d05d
Compare
|
Notice: the branch changed across the force-push!
~ Your Friendly Jira-GitHub PR Checker Bot |
zbraniecki
force-pushed
the
bench-overview
branch
from
308d05d to
31386d2
Compare
|
Notice: the branch changed across the force-push!
~ Your Friendly Jira-GitHub PR Checker Bot |
|
@zbraniecki I don't remember how the CI checks were when I reviewed (so I may have overlooked things), but currently the benchmark commands have errors during the CI steps for creating the benchmark dashboards. Thankfully, the existing dashboards and API docs are unaffected. |
This PR is on top of #323 and it fixes #324.
The goal is to separate benchmarks that are
overviewand have them available by default ascargo benchboth in each component/util and from the main directory.All other benchmarks are behind flag
bench.I also updated a couple benchmark names adding top level crate name to make it easier to recognize that
parser/overviewis part of plurals (so, renamed toplurals/parser/overview).The choice of what's in
overviewis a bit arbitrary and I expect us to continue refining them, but much likeexamplesI'd like theoverviewbenchmarks to be a decent approximation of what we expect common users to use.