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

Serialize command parameters to data output files #131

Merged
merged 1 commit into from
Jan 18, 2019

Conversation

bbannier
Copy link
Contributor

@bbannier bbannier commented Jan 16, 2019

This patch adds any parameter used to data output files (currently JSON
and CSV files). With that users have direct access to both input values
(i.e., benchmark parameters) and the resulting timings which should
simplify follow-up analyses.

@sharkdp
Copy link
Owner

sharkdp commented Jan 17, 2019

Thank you very much for your contribution.

Can you please provide a little bit of background / motivation for this change? This would make it easier to review.

@bbannier bbannier changed the title Serialize command parameters to JSON Serialize command parameters to data output files Jan 17, 2019
@bbannier
Copy link
Contributor Author

bbannier commented Jan 17, 2019

@sharkdp, thank you for responding, and sorry for the sloppy PR.

I have updated the initial comment and the subject. Ultimately this patch is driven by me wanting to directly plot results from JSON output files for parameterized benchmarks (e.g., to inspect scaling behavior and identify breakdown of naive scaling). For that I need direct access to benchmark parameter which currently requires command string parsing or adding values by hand.

I also realized that in addition to JSON, CSV output could also benefit from this improvement and updated the patches for that (which actually lead to a simplification).

Copy link
Owner

@sharkdp sharkdp 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 very much for the explanation and for the updates. This looks really great. I have just two minor comments.

@@ -412,5 +412,9 @@ pub fn run_benchmark(
t_min,
t_max,
times_real,
match cmd.get_parameter() {
Copy link
Owner

Choose a reason for hiding this comment

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

Could this match statement be shortened to something like

cmd.get_parameter().map(|p| p.1)

?

@@ -196,6 +201,7 @@ impl BenchmarkResult {
min,
max,
times: Some(times),
parameter: parameter,
Copy link
Owner

Choose a reason for hiding this comment

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

I think this could just be parameter instead of parameter: parameter.

This patch adds any parameter used to data output files (currently JSON
and CSV files). With that users have direct access to both input values
(i.e., benchmark parameters) and the resulting timings which should
simplify follow-up analyses.
@sharkdp sharkdp merged commit b252531 into sharkdp:master Jan 18, 2019
@sharkdp
Copy link
Owner

sharkdp commented Jan 18, 2019

Thank you for the updates!

@sharkdp
Copy link
Owner

sharkdp commented Jun 8, 2019

Released in v1.6.0.

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.

2 participants