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

*Display* aggregates only. #665

Merged
merged 11 commits into from
Sep 12, 2018
Merged

Conversation

LebedevRI
Copy link
Collaborator

There is a flag

DEFINE_bool(benchmark_report_aggregates_only, false,
"Report the result of each benchmark repetitions. When 'true' is "
"specified only the mean, standard deviation, and other statistics "
"are reported for repeated benchmarks.");

and a call
// Specify if each repetition of the benchmark should be reported separately
// or if only the final statistics should be reported. If the benchmark
// is not repeated then the single result is always reported.
Benchmark* ReportAggregatesOnly(bool value = true);

But that affects everything, every reporter, destination:
if (report_aggregates_only) reports.clear();

It would be quite useful to have an ability to be more picky.

More specifically, i would like to be able to only see the aggregates in the on-screen output,
but for the file output to still contain everything. The former is useful in case of a lot of repetition
(or even more so if every iteration is reported separately), while the former is great for tooling.

This does work, and is even less intrusive than i thought it will be.
But there is a slight test coverage issue :)
These tests are all testing the first, display, reporter.
So we don't actually check what happens with the second, file, reporter...
This problem isn't a new one, but here i would say it is more severe.
I'll see if i can easily fix it for this particular case..

Fixes #664

@coveralls
Copy link

coveralls commented Sep 2, 2018

Coverage Status

Coverage increased (+0.7%) to 88.351% when pulling 4f05864 on LebedevRI:display-aggregates-only into f090141 on google:master.

@AppVeyorBot
Copy link

Build benchmark 1402 failed (commit 82c280381b by @LebedevRI)

@LebedevRI
Copy link
Collaborator Author

After digging a bit more, this is the problematic bit:

benchmark/src/benchmark.cc

Lines 566 to 567 in 5159967

file_reporter->SetOutputStream(&output_file);
file_reporter->SetErrorStream(&output_file);

If we wanted to test that, we'd need to add some internal debug option to
disable this destination rebinding. Reading back from file seems rather fragile..

ARM_ReportAggregatesOnly };
{ ARM_Unspecified, // The mode has not been manually specified
ARM_Default, // The mode is user-specified as default.
ARM_ReportAggregatesOnly, // All reporters only display aggregates.
Copy link
Member

Choose a reason for hiding this comment

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

does the PR get even simpler if these are renamed to:

ARM_FileReportAggregatesOnly,
ARM_DisplayReportAggregatesOnly,

and then the existing flag sets them both to true, and the new flag sets only the Display one to true?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmm, kinda.
It gets a bit messier due to the command-line flags.

src/benchmark.cc Outdated
std::vector<BenchmarkReporter::Run> RunBenchmark(
const benchmark::internal::Benchmark::Instance& b,
std::vector<BenchmarkReporter::Run>* complexity_reports) {
std::pair<std::vector<BenchmarkReporter::Run> /*everything*/,
Copy link
Member

Choose a reason for hiding this comment

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

is it worth returning a custom struct to avoid having the comments tracking which member of the pair is which?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Makes sense.

src/benchmark.cc Outdated
display_aggregates_only);

const std::vector<BenchmarkReporter::Run>& report_for_display =
display_aggregates_only ? reports.second /*aggregates_only*/
Copy link
Member

Choose a reason for hiding this comment

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

would this also be simpler if the return type was 'non-aggregate', 'aggregate' instead of having the first be everything?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmm, actually it might be.
I didn't do so initially because i thought it was important to print the whole benchmark at once
(for header/context pre-print), but maybe it is too pessimistic..

@AppVeyorBot
Copy link

Build benchmark 1403 failed (commit 658116960e by @LebedevRI)

src/benchmark.cc Outdated Show resolved Hide resolved
src/benchmark.cc Outdated
(b.aggregation_report_mode == internal::ARM_Unspecified
? FLAGS_benchmark_report_aggregates_only
: b.aggregation_report_mode == internal::ARM_ReportAggregatesOnly);
run_results.display_report_aggregates_only =
Copy link
Member

Choose a reason for hiding this comment

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

why do you need to return this? you should be able to (as it was) have a local constant and decide to add to the vector or not based on the value.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Then i don't understand the previous suggestion, #665 (comment).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

But to attempt to answer, display_report_aggregates_only is needed,
because i need to somehow convey back which result (all the reports, or aggregates only)
to report, for both of the reporters. I need to do so because i actually look at the
benchmark::internal::Benchmark::Instance::aggregation_report_mode,
which is per-instance, not just FLAGS_benchmark_report_aggregates_only /
FLAGS_benchmark_display_aggregates_only, which is global.

Copy link
Member

Choose a reason for hiding this comment

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

Ah right, sorry.

@AppVeyorBot
Copy link

Build benchmark 1413 failed (commit ebf7a6f2ad by @LebedevRI)

@LebedevRI
Copy link
Collaborator Author

Any further thoughts here? :)
The testing issue is unresolved, but i'm not seeing any clear/clean solution for it..

@dmah42
Copy link
Member

dmah42 commented Sep 7, 2018

I have nothing further. I've been unable to come up with a satisfactory testing approach without radical replumbing of the file reporter.

@LebedevRI
Copy link
Collaborator Author

Makes sense..
Would it be unacceptably ugly to simply read back that file and check that?
I guess the troubling part is coming up with the tmp filename.

@dmah42
Copy link
Member

dmah42 commented Sep 7, 2018

#include <cstdio>
#include <filesystem>

std::filesystem::read_symlink(
    std::filesystem::path("/proc/self/fd") / std::to_string(fileno(tmpfile()))
);

would at least work on linux :P

@LebedevRI
Copy link
Collaborator Author

Hmm, actually now that i think about it std::tmpnam() might work, too,
and be more cross-platform. (ignoring the fact that std::filesystem is C++17)

@LebedevRI
Copy link
Collaborator Author

This is so horrible ^^

@AppVeyorBot
Copy link

Build benchmark 1415 failed (commit 3580387aea by @LebedevRI)

@AppVeyorBot
Copy link

Build benchmark 1416 failed (commit b92ecfb4ff by @LebedevRI)

@AppVeyorBot
Copy link

Build benchmark 1417 failed (commit 45bfd09832 by @LebedevRI)

README.md Outdated Show resolved Hide resolved

// Read the output back from the file, and delete the file.
std::ifstream tmp_stream(tmp_file_name);
std::string JsonOutput((std::istreambuf_iterator<char>(tmp_stream)),
Copy link
Member

Choose a reason for hiding this comment

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

there's extra parentheses here that i don't think are necessary.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, they are necessary.
https://godbolt.org/z/V6ASzJ

Copy link
Member

Choose a reason for hiding this comment

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

gah, vexing parse.

nit: s/JsonOutput/json_output/

@LebedevRI
Copy link
Collaborator Author

@dominichamon Thank you for taking a look!

@AppVeyorBot
Copy link

Build benchmark 1426 failed (commit 38bc928fb7 by @LebedevRI)

test/output_test.h Outdated Show resolved Hide resolved
@AppVeyorBot
Copy link

Build benchmark 1427 failed (commit e87c71478b by @LebedevRI)

@LebedevRI
Copy link
Collaborator Author

@dominichamon thank you for the review! I will merge this soon-ish.

@LebedevRI LebedevRI changed the title [RFC] *Display* aggregates only. *Display* aggregates only. Sep 12, 2018
@LebedevRI LebedevRI merged commit c614dfc into google:master Sep 12, 2018
@LebedevRI LebedevRI deleted the display-aggregates-only branch September 12, 2018 13:26
JBakamovic pushed a commit to JBakamovic/benchmark that referenced this pull request Dec 6, 2018
There is a flag 
https://github.com/google/benchmark/blob/d9cab612e40017af10bddaa5b60c7067032a9e1c/src/benchmark.cc#L75-L78
and a call
https://github.com/google/benchmark/blob/d9cab612e40017af10bddaa5b60c7067032a9e1c/include/benchmark/benchmark.h#L837-L840
But that affects everything, every reporter, destination:
https://github.com/google/benchmark/blob/d9cab612e40017af10bddaa5b60c7067032a9e1c/src/benchmark.cc#L316


It would be quite useful to have an ability to be more picky.


More specifically, i would like to be able to only see the aggregates in the on-screen output,
but for the file output to still contain everything. The former is useful in case of a lot of repetition
(or even more so if every iteration is reported separately), while the former is **great** for tooling.

Fixes google#664
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[RFC] *Display* aggregates only.
5 participants