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

Add --benchmark_human_readable flag #1607

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

DiegoKrupitza
Copy link

Allows used to add a command line flag called --benchmark_human_readable. By adding this flag the arguments passed to benchmarks are formated in a human friendly format. This means that numbers that are the power of 2 are formatted as 2^x (e.g., 64 will be 2^6). For numbers that are the power of 10 a different formatting style is used. Numbers 0-999 no formatting is used. For numbers 1000-999999 the format k is used (e.g., 32000 -> 32k). This also works for millions and billions. For numbers greater than 999 billion no special formatting is used.
The design is rather simple allowing to by easily extendable.

Closes: #1006

src/benchmark_api_internal.h Outdated Show resolved Hide resolved
@dmah42
Copy link
Member

dmah42 commented Jun 8, 2023

"test/human_readable_formatting_test.cc:34: CheckRun: Check `name == run.benchmark_name()' failed. expected BM_base_two_args/2^1 got BM_base_two_args/2" test is failing

@DiegoKrupitza DiegoKrupitza force-pushed the feat/1006_human_readable_format branch from e44d95f to 47ce92d Compare June 19, 2023 17:42
@DiegoKrupitza
Copy link
Author

I squashed all the commits into a single one such that there is only one commit to merge.

@dmah42
Copy link
Member

dmah42 commented Jun 20, 2023

I squashed all the commits into a single one such that there is only one commit to merge.

thanks.. i generally "squash and merge" anyway, but this means less editing of commit messages for me :D

@@ -44,7 +51,8 @@ void ToExponentAndMantissa(double val, double thresh, int precision,
// in 'precision' digits.
const double adjusted_threshold =
std::max(thresh, 1.0 / std::pow(10.0, precision));
const double big_threshold = adjusted_threshold * one_k;
const double big_threshold =
(adjusted_threshold * one_k) - inclusiveBigThreshhold;
Copy link
Member

Choose a reason for hiding this comment

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

you appear to be subtracting a boolean from a double.. that's surprising and maybe you could make this explicit instead of relying on an implicit conversion.

Copy link
Author

Choose a reason for hiding this comment

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

I used this neat little hack to make the code a litte bit more concise. But I can make it more explicit and add a little bit of docu why this trick is needed.

* Check whether the given value is a power of two or not.
* @param val the value to check
*/
bool IsPowerOfTwo(const int64_t& val);
Copy link
Member

Choose a reason for hiding this comment

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

do these need to be exposed in the API? i think only FormatHumanReadable is accessed by a different implementation file.

Copy link
Author

Choose a reason for hiding this comment

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

Yes this method doesnt need to be exposed. Exposing Base10 and Base2 is needed for tests and may be needed by other functions in the future.

@DiegoKrupitza DiegoKrupitza force-pushed the feat/1006_human_readable_format branch from 6029297 to 2d68daa Compare June 20, 2023 12:58
Allows used to add a command line flag called
`--benchmark_human_readable`. By adding this flag the arguments passed
to benchmarks are formatted in a human friendly format. This means that
numbers that are the power of 2 are formatted as `2^x` (e.g., 64 will be
`2^6`). For numbers that are the power of 10 a different formatting
style is used. Numbers 0-999 no formatting is used. For numbers
1000-999999 the format `k` is used (e.g., `32000` -> `32k`). This also
works for millions, billions, trillions, ... For numbers greater than
septillions no special formatting is used.
The design is rather simple allowing to by easily extendable.

Closes: google#1006
@DexterIV
Copy link

im not a big fan of "human readable format". Id consider binary output to be not human readable but not base10 numbers. Asked chatgpt what it "thinks" about that and here is the answer:
The current name "Human Readable Format" seems appropriate for describing the functionality of the option. However, if you are looking for an alternative name that captures the essence of the feature, you could consider something like "Formatted Number Display" or "Enhanced Numeric Representation."
just a thing to consider :)

@dmah42
Copy link
Member

dmah42 commented Jun 26, 2023

im not a big fan of "human readable format". Id consider binary output to be not human readable but not base10 numbers. Asked chatgpt what it "thinks" about that and here is the answer: The current name "Human Readable Format" seems appropriate for describing the functionality of the option. However, if you are looking for an alternative name that captures the essence of the feature, you could consider something like "Formatted Number Display" or "Enhanced Numeric Representation." just a thing to consider :)

i think it's a pretty well-defined name at this point. df -h, for instance:

       -h, --human-readable
              print sizes in powers of 1024 (e.g., 1023M)

or du -h:

       -h, --human-readable
              print sizes in human readable format (e.g., 1K 234M 2G)

@@ -1,6 +1,8 @@
#ifndef BENCHMARK_API_INTERNAL_H
#define BENCHMARK_API_INTERNAL_H

#include <array>
Copy link
Member

Choose a reason for hiding this comment

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

still seeing these two headers. i'm sure they don't need to be added and i'd rather minimise any dependencies.

@@ -32,7 +38,8 @@ static const int64_t kUnitsSize = arraysize(kBigSIUnits);

void ToExponentAndMantissa(double val, double thresh, int precision,
double one_k, std::string* mantissa,
int64_t* exponent) {
int64_t* exponent,
bool inclusiveBigThreshhold = false) {
Copy link
Member

Choose a reason for hiding this comment

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

seems like we only set this to true when we're doing base-10. it might be worth having a comment why this is necessary for base-10. or even better, renaming the variable to indicate that directly.

@dmah42 dmah42 added the incomplete work needed label Aug 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
incomplete work needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FR] human readable name of benchmark
3 participants