[BUG] inconsistent suffixes in console reporter

[cwpearson]

Describe the bug

for user counters, k means 1000 and for the bytes_per_second counter, k means 1024.

System Which OS, compiler, and compiler version are you using:

• OS: Ubuntu 18.04
• Compiler and version: g++ 7.5.0

To reproduce Steps to reproduce the behavior:

1. sync to tag 1.5.1
2. Create a benchmark that uses state.SetBytesProcessed(...) and state.counters["bytes"] = ....
3. Compare the results of the csv reporter and the console reporter for the bytes_per_second and bytes counters.

Expected behavior

Perhaps these suffixes should have the same meaning, or the power-of-2 versions should change to ki, Mi, Gi.

Additional context

The csv output format prints the true values, for reference, so it's easy to see that the suffixes have different meanings for different user counters.

[dmah42]

The false on this line could be changed to 'true' depending on one_k. However, i'd also appreciate a more complete refactor that changes the double one_k to be an enum SI vs IEC which can be plumbed through correctly.

[varshneydevansh]

Hi @dmah42, if this is still pending, I would love to work on this one? I did look at the code file.

It will help me gain a more profound understanding of benchmarking.

Regards.

[LebedevRI]

I'm not exactly sure how it should work (as opposed to how it works now; should said counters be fully Unitful/dimension-ful?), but this does seem like it should be reasonably simple changes. I don't think anyone is looking into this currently, so feel free to. Thanks.

[dmah42]

we do have the ability to differentiate between SI and IEC but it only allows a single character suffix. might be good to expand this to multi-character strings and correctly differentiate in the output.

// kilo, Mega, Giga, Tera, Peta, Exa, Zetta, Yotta.
const char kBigSIUnits[] = "kMGTPEZY";
// Kibi, Mebi, Gibi, Tebi, Pebi, Exbi, Zebi, Yobi.
const char kBigIECUnits[] = "KMGTPEZY";

[varshneydevansh]

To somewhat like this?

// kilo, Mega, Giga, Tera, Peta, Exa, Zetta, Yotta.
const std::string kBigSIUnits[] = {"k", "M", "G", "T", "P", "E", "Z", "Y"};
// Kibi, Mebi, Gibi, Tebi, Pebi, Exbi, Zebi, Yobi.
const std::string kBigIECUnits[] = {"Ki", "Mi", "Gi", "Ti", "Pi", "Ei", "Zi", "Yi"};
// milli, micro, nano, pico, femto, atto, zepto, yocto.
const std::string kSmallSIUnits[] = {"m", "u", "n", "p", "f", "a", "z", "y"};

[dmah42]

yeah, and then picking the right one for user counters vs other reports. in addition we might offer the user counter to be SI or IEC (and then do the calculation and reporting appropriately) but that's probably a second phase.

[varshneydevansh]

Regarding oneK we have to modify probably these locations according to the proposed changes, i.e. double one_k to enum.

1. benchmark/include/benchmark/benchmark.h: This file contains the definition of the Counter class and I think here we have to add the enumeration to this class, and modify the constructor.
2. benchmark/include/benchmark/benchmark.cc: Here, in this file we have to ensure that when a Counter instance is created with the proper change as we might have to add an extra parameter.
3. benchmark/src/string_util.cc: Here in this file in the function in the permalink we might have to add a check whether the one_k counter is SI or IEC. Furthermore, in file src/string_util.h we might also need to, modify the function declaration HumanReadableNumber(double n, double one_k = 1024.0);
4. console_reporter.cc: We might also have to adjust reporting in this file and some other files too.

In the file benchmark.h

Can we add something like in the Counter class?

class Counter {
    // ... existing code ...

enum class UnitScale {
    SI,     // k means 1000
    IEC     // k means 1024
};
    UnitSystem unitSystem;
};

Do we also need to introduce a new flag to distinguish between user counters and other reports in the Flag's enum within the class Counter?

Along with incorporating the suggested modifications for 'multi-character strings' in. string_util.cc

---

Apologies, for the delayed response, today we had a major power cut as the monsoon just arrived here.

Please, guide me whether am I going in the right direction or not?

[dmah42]

OneK is already an enum and included in the Counter, so i don't think you need to change anything there. HumanReadableNumber is already passed oneK from the Counter so i don't think you need to change that either.

i think first changing the units to differentiate correctly between SI and IEC (they're currently the same for MGT, etc, and ambiguous for k/K) would get us a long way.