bug: Inconsistent suffixes console reporter 1009 #1631

varshneydevansh · 2023-07-11T12:16:57Z

Issue ->

[BUG] inconsistent suffixes in console reporter #1009

Proposed Solution ->

Setting true or false based on the value of one_k which is being converted from a double to enum in std::string ToBinaryStringFullySpecified() in file string_util.cc

and changed the SI and IEC single character suffixes to multi-character strings

… strings

…/github.com/varshneydevansh/benchmark into inconsistent_suffixes_console_reporter_1009

src/string_util.cc

…ng_util.h

…/github.com/varshneydevansh/benchmark into inconsistent_suffixes_console_reporter_1009

include/benchmark/benchmark.h

…t of Public API

dmah42 · 2023-07-11T16:41:54Z

you'll need to update some tests, and run clang-format (Google style) to get the CI bots to be happy.

varshneydevansh · 2023-07-11T16:52:59Z

you'll need to update some tests, and run clang-format (Google style) to get the CI bots to be happy.

I think in the string_util_gtest.cc file, there is a typo in the first comment. Should I also correct it?

varshneydevansh · 2023-07-11T18:11:39Z

Hi @dmah42 ,
Should I commit the changes? I also ran the clang-format. Furthermore, are these test cases correct?

I added the following test cases in the string_util_gtest.cc file -

TEST(StringUtilTest, ExponentToPrefix) {
  EXPECT_EQ("", benchmark::ExponentToPrefix(0, true));
  EXPECT_EQ("", benchmark::ExponentToPrefix(0, false));

  EXPECT_EQ("k", benchmark::ExponentToPrefix(1, true));
  EXPECT_EQ("K", benchmark::ExponentToPrefix(1, false));

  EXPECT_EQ("Mi", benchmark::ExponentToPrefix(2, true));
  EXPECT_EQ("M", benchmark::ExponentToPrefix(2, false));

  EXPECT_EQ("Gi", benchmark::ExponentToPrefix(3, true));
  EXPECT_EQ("G", benchmark::ExponentToPrefix(3, false));

  EXPECT_EQ("m", benchmark::ExponentToPrefix(-1, true));
  EXPECT_EQ("m", benchmark::ExponentToPrefix(-1, false));

  EXPECT_EQ("u", benchmark::ExponentToPrefix(-2, true));
  EXPECT_EQ("u", benchmark::ExponentToPrefix(-2, false));

  EXPECT_EQ("n", benchmark::ExponentToPrefix(-3, true));
  EXPECT_EQ("n", benchmark::ExponentToPrefix(-3, false));
}

TEST(StringUtilTest, ToBinaryStringFullySpecified) {
  // Test with Counter::kIs1024
  EXPECT_EQ("1.00", benchmark::ToBinaryStringFullySpecified(1.0, 1.0, 2));
  EXPECT_EQ("1.23k", benchmark::ToBinaryStringFullySpecified(1234.0, 1.0, 2));
  EXPECT_EQ("1.00M", benchmark::ToBinaryStringFullySpecified(1.0e6, 1.0, 2));
  EXPECT_EQ("1.00G", benchmark::ToBinaryStringFullySpecified(1.0e9, 1.0, 2));

  // Test with Counter::kIs1000
  EXPECT_EQ("1.00", benchmark::ToBinaryStringFullySpecified(
                        1.0, 1.0, 2, benchmark::Counter::kIs1000));
  EXPECT_EQ("1.23k", benchmark::ToBinaryStringFullySpecified(
                         1234.0, 1.0, 2, benchmark::Counter::kIs1000));
  EXPECT_EQ("1.00M", benchmark::ToBinaryStringFullySpecified(
                         1.0e6, 1.0, 2, benchmark::Counter::kIs1000));
  EXPECT_EQ("1.00G", benchmark::ToBinaryStringFullySpecified(
                         1.0e9, 1.0, 2, benchmark::Counter::kIs1000));
}

TEST(StringUtilTest, AppendHumanReadable) {
  std::string str;

  benchmark::AppendHumanReadable(0, &str);
  EXPECT_EQ("0", str);

  benchmark::AppendHumanReadable(999, &str);
  EXPECT_EQ("999", str);

  benchmark::AppendHumanReadable(1000, &str);
  EXPECT_EQ("1.00k", str);

  benchmark::AppendHumanReadable(1000000, &str);
  EXPECT_EQ("1.00M", str);

  benchmark::AppendHumanReadable(1000000000, &str);
  EXPECT_EQ("1.00G", str);
}

TEST(StringUtilTest, HumanReadableNumber) {
  EXPECT_EQ("1.00", benchmark::HumanReadableNumber(1.0));
  EXPECT_EQ("1.23k", benchmark::HumanReadableNumber(1234.0));
  EXPECT_EQ("1.00M", benchmark::HumanReadableNumber(1.0e6));
  EXPECT_EQ("1.00G", benchmark::HumanReadableNumber(1.0e9));

  EXPECT_EQ("1.00",
            benchmark::HumanReadableNumber(1.0, benchmark::Counter::kIs1000));
  EXPECT_EQ("1.23k", benchmark::HumanReadableNumber(
                         1234.0, benchmark::Counter::kIs1000));
  EXPECT_EQ("1.00M",
            benchmark::HumanReadableNumber(1.0e6, benchmark::Counter::kIs1000));
  EXPECT_EQ("1.00G",
            benchmark::HumanReadableNumber(1.0e9, benchmark::Counter::kIs1000));
}

and the result is this

➜  test git:(inconsistent_suffixes_console_reporter_1009) ✗ ./string_util_gtest
Running main() from gmock_main.cc
[==========] Running 8 tests from 1 test suite.
[----------] Global test environment set-up.
[----------] 8 tests from StringUtilTest
[ RUN      ] StringUtilTest.stoul
[       OK ] StringUtilTest.stoul (2 ms)
[ RUN      ] StringUtilTest.stoi
[       OK ] StringUtilTest.stoi (0 ms)
[ RUN      ] StringUtilTest.stod
[       OK ] StringUtilTest.stod (0 ms)
[ RUN      ] StringUtilTest.StrSplit
[       OK ] StringUtilTest.StrSplit (0 ms)
[ RUN      ] StringUtilTest.ExponentToPrefix
/home/devansh/benchmark/test/string_util_gtest.cc:168: Failure
Expected equality of these values:
  "k"
  benchmark::ExponentToPrefix(1, true)
    Which is: "Ki"
/home/devansh/benchmark/test/string_util_gtest.cc:169: Failure
Expected equality of these values:
  "K"
  benchmark::ExponentToPrefix(1, false)
    Which is: "k"
[  FAILED  ] StringUtilTest.ExponentToPrefix (0 ms)
[ RUN      ] StringUtilTest.ToBinaryStringFullySpecified
/home/devansh/benchmark/test/string_util_gtest.cc:189: Failure
Expected equality of these values:
  "1.00"
  benchmark::ToBinaryStringFullySpecified(1.0, 1.0, 2)
    Which is: "1"
/home/devansh/benchmark/test/string_util_gtest.cc:190: Failure
Expected equality of these values:
  "1.23k"
  benchmark::ToBinaryStringFullySpecified(1234.0, 1.0, 2)
    Which is: "1.20508Ki"
/home/devansh/benchmark/test/string_util_gtest.cc:191: Failure
Expected equality of these values:
  "1.00M"
  benchmark::ToBinaryStringFullySpecified(1.0e6, 1.0, 2)
    Which is: "976.562Ki"
/home/devansh/benchmark/test/string_util_gtest.cc:192: Failure
Expected equality of these values:
  "1.00G"
  benchmark::ToBinaryStringFullySpecified(1.0e9, 1.0, 2)
    Which is: "953.674Mi"
/home/devansh/benchmark/test/string_util_gtest.cc:195: Failure
Expected equality of these values:
  "1.00"
  benchmark::ToBinaryStringFullySpecified( 1.0, 1.0, 2, benchmark::Counter::kIs1000)
    Which is: "1"
/home/devansh/benchmark/test/string_util_gtest.cc:197: Failure
Expected equality of these values:
  "1.23k"
  benchmark::ToBinaryStringFullySpecified( 1234.0, 1.0, 2, benchmark::Counter::kIs1000)
    Which is: "1.234k"
/home/devansh/benchmark/test/string_util_gtest.cc:199: Failure
Expected equality of these values:
  "1.00M"
  benchmark::ToBinaryStringFullySpecified( 1.0e6, 1.0, 2, benchmark::Counter::kIs1000)
    Which is: "1000k"
/home/devansh/benchmark/test/string_util_gtest.cc:201: Failure
Expected equality of these values:
  "1.00G"
  benchmark::ToBinaryStringFullySpecified( 1.0e9, 1.0, 2, benchmark::Counter::kIs1000)
    Which is: "1000M"
[  FAILED  ] StringUtilTest.ToBinaryStringFullySpecified (0 ms)
[ RUN      ] StringUtilTest.AppendHumanReadable
/home/devansh/benchmark/test/string_util_gtest.cc:212: Failure
Expected equality of these values:
  "999"
  str
    Which is: "0999"
/home/devansh/benchmark/test/string_util_gtest.cc:215: Failure
Expected equality of these values:
  "1.00k"
  str
    Which is: "09991000"
/home/devansh/benchmark/test/string_util_gtest.cc:218: Failure
Expected equality of these values:
  "1.00M"
  str
    Which is: "09991000976.562Ki"
/home/devansh/benchmark/test/string_util_gtest.cc:221: Failure
Expected equality of these values:
  "1.00G"
  str
    Which is: "09991000976.562Ki953.674Mi"
[  FAILED  ] StringUtilTest.AppendHumanReadable (0 ms)
[ RUN      ] StringUtilTest.HumanReadableNumber
/home/devansh/benchmark/test/string_util_gtest.cc:225: Failure
Expected equality of these values:
  "1.00"
  benchmark::HumanReadableNumber(1.0)
    Which is: "1"
/home/devansh/benchmark/test/string_util_gtest.cc:226: Failure
Expected equality of these values:
  "1.23k"
  benchmark::HumanReadableNumber(1234.0)
    Which is: "1.20508Ki"
/home/devansh/benchmark/test/string_util_gtest.cc:227: Failure
Expected equality of these values:
  "1.00M"
  benchmark::HumanReadableNumber(1.0e6)
    Which is: "976.562Ki"
/home/devansh/benchmark/test/string_util_gtest.cc:228: Failure
Expected equality of these values:
  "1.00G"
  benchmark::HumanReadableNumber(1.0e9)
    Which is: "953.674Mi"
/home/devansh/benchmark/test/string_util_gtest.cc:230: Failure
Expected equality of these values:
  "1.00"
  benchmark::HumanReadableNumber(1.0, benchmark::Counter::kIs1000)
    Which is: "1"
/home/devansh/benchmark/test/string_util_gtest.cc:232: Failure
Expected equality of these values:
  "1.23k"
  benchmark::HumanReadableNumber( 1234.0, benchmark::Counter::kIs1000)
    Which is: "1.234k"
/home/devansh/benchmark/test/string_util_gtest.cc:234: Failure
Expected equality of these values:
  "1.00M"
  benchmark::HumanReadableNumber(1.0e6, benchmark::Counter::kIs1000)
    Which is: "1000k"
/home/devansh/benchmark/test/string_util_gtest.cc:236: Failure
Expected equality of these values:
  "1.00G"
  benchmark::HumanReadableNumber(1.0e9, benchmark::Counter::kIs1000)
    Which is: "1000M"
[  FAILED  ] StringUtilTest.HumanReadableNumber (0 ms)
[----------] 8 tests from StringUtilTest (3 ms total)

[----------] Global test environment tear-down
[==========] 8 tests from 1 test suite ran. (3 ms total)
[  PASSED  ] 4 tests.
[  FAILED  ] 4 tests, listed below:
[  FAILED  ] StringUtilTest.ExponentToPrefix
[  FAILED  ] StringUtilTest.ToBinaryStringFullySpecified
[  FAILED  ] StringUtilTest.AppendHumanReadable
[  FAILED  ] StringUtilTest.HumanReadableNumber

 4 FAILED TESTS

include/benchmark/benchmark.h

test/string_util_gtest.cc

…r_1009

…/github.com/varshneydevansh/benchmark into inconsistent_suffixes_console_reporter_1009

varshneydevansh · 2023-07-12T17:45:48Z

I wanna know something about this function -

in the string_util.cc file

void AppendHumanReadable(int n, std::string* str) {
  std::stringstream ss;
  // Round down to the nearest SI prefix.
  ss << ToBinaryStringFullySpecified(n, 1.0, 0);
  *str += ss.str();
}

and the function which this is calling has a default value for one_k = 1024.0

std::string ToBinaryStringFullySpecified(
    double value, double threshold, int precision,
    Counter::OneK one_k = Counter::kIs1024) {
  std::string mantissa;
  int64_t exponent;
  ToExponentAndMantissa(value, threshold, precision, one_k, &mantissa,
                        &exponent);

  return mantissa + ExponentToPrefix(exponent, one_k == Counter::kIs1024);
}

And this std::string ToBinaryStringFullySpecified() works fine with the std::string HumanReadableNumber(double n, Counter::OneK one_k) as we are sending the flag value along with this.

In that case, the comment "// Round down to the nearest SI prefix" may be misleading or incorrect. If the intention is to use the IEC prefixes, the comment should reflect that, such as "// Round down to the nearest IEC prefix."

If that is not the intention, then we might need to send the value of kIs1000 in the ss << ToBinaryStringFullySpecified(n, 1.0, 0);.

And regarding the test cases, I now only added test cases for these two methods HumanReadableNumber() and AppendHumanReadable() as they are only available in the string_util.h file and all of our changes depends on them.

varshneydevansh · 2023-07-12T20:06:31Z

I did find my one mistake with the following function, where I missed changing the type of `one_k` from double to enum.

The following is the updated code -

void ToExponentAndMantissa(double val, double thresh, int precision,
                           Counter::OneK one_k, std::string* mantissa,
                           int64_t* exponent) {
  std::stringstream mantissa_stream;

  if (val < 0) {
    mantissa_stream << "-";
    val = -val;
  }

  // Adjust threshold so that it never excludes things which can't be rendered
  // 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 == Counter::kIs1024 ? 1024.0 : 1000.0);
  const double small_threshold = adjusted_threshold;
  // Values in ]simple_threshold,small_threshold[ will be printed as-is
  const double simple_threshold = 0.01;

  if (val > big_threshold) {
    // Positive powers
    double scaled = val;
    for (size_t i = 0; i < arraysize(kBigSIUnits); ++i) {
      scaled /= (one_k == Counter::kIs1024 ? 1024.0 : 1000.0);
      if (scaled <= big_threshold) {
        mantissa_stream << scaled;
        *exponent = i + 1;
        *mantissa = mantissa_stream.str();
        return;
      }
    }
    mantissa_stream << val;
    *exponent = 0;
  } else if (val < small_threshold) {
    // Negative powers
    if (val < simple_threshold) {
      double scaled = val;
      for (size_t i = 0; i < arraysize(kSmallSIUnits); ++i) {
        scaled *= (one_k == Counter::kIs1024 ? 1024.0 : 1000.0);
        if (scaled >= small_threshold) {
          mantissa_stream << scaled;
          *exponent = -static_cast<int64_t>(i + 1);
          *mantissa = mantissa_stream.str();
          return;
        }
      }
    }
    mantissa_stream << val;
    *exponent = 0;
  } else {
    mantissa_stream << val;
    *exponent = 0;
  }
  *mantissa = mantissa_stream.str();
}

And just for my clarification, could the below if be written like this for the for-loop? And I think the arraysize macro is meant to be used with a statically sized array, and it cannot determine the size of an array determined at runtime.

  if (val > big_threshold) {
    // Positive powers
    double scaled = val;
    for (size_t i = 0; i < arraysize((one_k == Counter::kIs1024 ? kBigIECUnits : kBigSIUnits)); ++i) {

OR I may could store the result in a variable?

double checked_one_k = (one_k == Counter::kIs1024 ? 1024.0 : 1000.0)

dmah42 · 2023-07-13T09:01:51Z

I wanna know something about this function -

in the string_util.cc file
void AppendHumanReadable(int n, std::string* str) {
  std::stringstream ss;
  // Round down to the nearest SI prefix.
  ss << ToBinaryStringFullySpecified(n, 1.0, 0);
  *str += ss.str();
}
and the function which this is calling has a default value for one_k = 1024.0
std::string ToBinaryStringFullySpecified(
    double value, double threshold, int precision,
    Counter::OneK one_k = Counter::kIs1024) {
  std::string mantissa;
  int64_t exponent;
  ToExponentAndMantissa(value, threshold, precision, one_k, &mantissa,
                        &exponent);

  return mantissa + ExponentToPrefix(exponent, one_k == Counter::kIs1024);
}
And this std::string ToBinaryStringFullySpecified() works fine with the std::string HumanReadableNumber(double n, Counter::OneK one_k) as we are sending the flag value along with this.

In that case, the comment "// Round down to the nearest SI prefix" may be misleading or incorrect. If the intention is to use the IEC prefixes, the comment should reflect that, such as "// Round down to the nearest IEC prefix."

If that is not the intention, then we might need to send the value of kIs1000 in the ss << ToBinaryStringFullySpecified(n, 1.0, 0);.

that's a good observation. yes, we should change the behaviour to match the comment as the comment describes the intent. (kIs1000 is more "natural" for human readable).

in future we may make this an option on the outer function.

And regarding the test cases, I now only added test cases for these two methods HumanReadableNumber() and AppendHumanReadable() as they are only available in the string_util.h file and all of our changes depends on them.

perfect.

dmah42 · 2023-07-13T09:03:06Z

I did find my one mistake with the following function, where I missed changing the type of `one_k` from double to enum.

The following is the updated code -

void ToExponentAndMantissa(double val, double thresh, int precision,
                           Counter::OneK one_k, std::string* mantissa,
                           int64_t* exponent) {
  std::stringstream mantissa_stream;

  if (val < 0) {
    mantissa_stream << "-";
    val = -val;
  }

  // Adjust threshold so that it never excludes things which can't be rendered
  // 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 == Counter::kIs1024 ? 1024.0 : 1000.0);
  const double small_threshold = adjusted_threshold;
  // Values in ]simple_threshold,small_threshold[ will be printed as-is
  const double simple_threshold = 0.01;

  if (val > big_threshold) {
    // Positive powers
    double scaled = val;
    for (size_t i = 0; i < arraysize(kBigSIUnits); ++i) {
      scaled /= (one_k == Counter::kIs1024 ? 1024.0 : 1000.0);
      if (scaled <= big_threshold) {
        mantissa_stream << scaled;
        *exponent = i + 1;
        *mantissa = mantissa_stream.str();
        return;
      }
    }
    mantissa_stream << val;
    *exponent = 0;
  } else if (val < small_threshold) {
    // Negative powers
    if (val < simple_threshold) {
      double scaled = val;
      for (size_t i = 0; i < arraysize(kSmallSIUnits); ++i) {
        scaled *= (one_k == Counter::kIs1024 ? 1024.0 : 1000.0);
        if (scaled >= small_threshold) {
          mantissa_stream << scaled;
          *exponent = -static_cast<int64_t>(i + 1);
          *mantissa = mantissa_stream.str();
          return;
        }
      }
    }
    mantissa_stream << val;
    *exponent = 0;
  } else {
    mantissa_stream << val;
    *exponent = 0;
  }
  *mantissa = mantissa_stream.str();
}

And just for my clarification, could the below if be written like this for the for-loop? And I think the arraysize macro is meant to be used with a statically sized array, and it cannot determine the size of an array determined at runtime.

  if (val > big_threshold) {
    // Positive powers
    double scaled = val;
    for (size_t i = 0; i < arraysize((one_k == Counter::kIs1024 ? kBigIECUnits : kBigSIUnits)); ++i) {

OR I may could store the result in a variable?

double checked_one_k = (one_k == Counter::kIs1024 ? 1024.0 : 1000.0)

there's readability benefits to pull the calculation out of the for loop into a local variable (and small performance benefits if it's marked const, though most compilers will figure that out anyway).

varshneydevansh · 2023-07-13T12:05:09Z

#include <iomanip.h>
void ToExponentAndMantissa(double val, double thresh, int precision,
                           Counter::OneK one_k, std::string* mantissa,
                           int64_t* exponent) {
  std::stringstream mantissa_stream;

  if (val < 0) {
    mantissa_stream << "-";
    val = -val;
  }

  // Adjust threshold so that it never excludes things which can't be rendered
  // 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 == Counter::kIs1024 ? 1024.0 : 1000.0);
  const double small_threshold = adjusted_threshold;
  // Values in ]simple_threshold,small_threshold[ will be printed as-is
  const double simple_threshold = 0.01;

  if (val > big_threshold) {
    // Positive powers
    double scaled = val;
    for (size_t i = 0; i < arraysize(kBigSIUnits); ++i) {
      scaled /= (one_k == Counter::kIs1024 ? 1024.0 : 1000.0);
      if (scaled < big_threshold) {
        const double tolerance = 1e-9;
        if (std::abs(scaled - 1.0) <= tolerance) {
          mantissa_stream << std::setprecision(precision) << std::fixed
                          << scaled;
          *exponent = i + 2;
        } else {
          mantissa_stream << std::setprecision(precision) << std::fixed
                          << scaled;
          *exponent = i + 1;
        }
        *mantissa = mantissa_stream.str();
        return;
      }
    }
    mantissa_stream << std::setprecision(precision) << std::fixed << val;
    *exponent = 0;
  } else if (val < small_threshold) {
    // Negative powers
    if (val < simple_threshold) {
      double scaled = val;
      for (size_t i = 0; i < arraysize(kSmallSIUnits); ++i) {
        scaled *= (one_k == Counter::kIs1024 ? 1024.0 : 1000.0);
        if (scaled >= small_threshold) {
          mantissa_stream << std::setprecision(precision) << std::fixed
                          << scaled;
          *exponent = -static_cast<int64_t>(i + 1);
          *mantissa = mantissa_stream.str();
          return;
        }
      }
    }
    mantissa_stream << std::setprecision(precision) << std::fixed << val;
    *exponent = 0;
  } else {
    mantissa_stream << std::setprecision(precision) << std::fixed << val;
    *exponent = 0;
  }
  *mantissa = mantissa_stream.str();
}

I did try to modify the above function, but the if-clause inside the big_threshold started to shoot-off -

        const double tolerance = 1e-9;
        if (std::abs(scaled - 1.0) <= tolerance) {
          mantissa_stream << std::setprecision(precision) << std::fixed
                          << scaled;
          *exponent = i + 2;
        }

I added this to promote to the next higher unit when the value is exactly 1000 or so on.

These are the test cases -

TEST(StringUtilTest, AppendHumanReadable) {
  std::string str;

  benchmark::AppendHumanReadable(0, &str);
  EXPECT_EQ("0", str);
  str.clear();

  benchmark::AppendHumanReadable(999, &str);
  EXPECT_EQ("999", str);
  str.clear();

  benchmark::AppendHumanReadable(1000, &str);
  EXPECT_EQ("1k", str);
  str.clear();

  benchmark::AppendHumanReadable(1000000, &str);
  EXPECT_EQ("1M", str);
  str.clear();

  benchmark::AppendHumanReadable(1000000000, &str);
  EXPECT_EQ("1G", str);
  str.clear();
}

TEST(StringUtilTest, HumanReadableNumber) {
  EXPECT_EQ("1.0", benchmark::HumanReadableNumber(1.0));
  EXPECT_EQ("1.2Ki", benchmark::HumanReadableNumber(1234.0));
  EXPECT_EQ("976.6Ki", benchmark::HumanReadableNumber(1.0e6));
  EXPECT_EQ("953.7Mi", benchmark::HumanReadableNumber(1.0e9));

  EXPECT_EQ("1.0",
            benchmark::HumanReadableNumber(1.0, benchmark::Counter::kIs1000));
  EXPECT_EQ("1.2k", benchmark::HumanReadableNumber(
                        1234.0, benchmark::Counter::kIs1000));
  EXPECT_EQ("1M",
            benchmark::HumanReadableNumber(1.0e6, benchmark::Counter::kIs1000));
  EXPECT_EQ("1G",
            benchmark::HumanReadableNumber(1.0e9, benchmark::Counter::kIs1000));
}

Output for the modified code -

➜  test git:(inconsistent_suffixes_console_reporter_1009) ✗ ./string_util_gtest
Running main() from gmock_main.cc
[==========] Running 6 tests from 1 test suite.
[----------] Global test environment set-up.
[----------] 6 tests from StringUtilTest
[ RUN      ] StringUtilTest.stoul
[       OK ] StringUtilTest.stoul (0 ms)
[ RUN      ] StringUtilTest.stoi
[       OK ] StringUtilTest.stoi (0 ms)
[ RUN      ] StringUtilTest.stod
[       OK ] StringUtilTest.stod (0 ms)
[ RUN      ] StringUtilTest.StrSplit
[       OK ] StringUtilTest.StrSplit (0 ms)
[ RUN      ] StringUtilTest.AppendHumanReadable
/home/devansh/benchmark/test/string_util_gtest.cc:175: Failure
Expected equality of these values:
  "1k"
  str
    Which is: "1000"
/home/devansh/benchmark/test/string_util_gtest.cc:179: Failure
Expected equality of these values:
  "1M"
  str
    Which is: "1G"
/home/devansh/benchmark/test/string_util_gtest.cc:183: Failure
Expected equality of these values:
  "1G"
  str
    Which is: "1T"
[  FAILED  ] StringUtilTest.AppendHumanReadable (0 ms)
[ RUN      ] StringUtilTest.HumanReadableNumber
/home/devansh/benchmark/test/string_util_gtest.cc:197: Failure
Expected equality of these values:
  "1M"
  benchmark::HumanReadableNumber(1.0e6, benchmark::Counter::kIs1000)
    Which is: "1000.0k"
/home/devansh/benchmark/test/string_util_gtest.cc:199: Failure
Expected equality of these values:
  "1G"
  benchmark::HumanReadableNumber(1.0e9, benchmark::Counter::kIs1000)
    Which is: "1000.0M"
[  FAILED  ] StringUtilTest.HumanReadableNumber (0 ms)
[----------] 6 tests from StringUtilTest (0 ms total)

[----------] Global test environment tear-down
[==========] 6 tests from 1 test suite ran. (0 ms total)
[  PASSED  ] 4 tests.
[  FAILED  ] 2 tests, listed below:
[  FAILED  ] StringUtilTest.AppendHumanReadable
[  FAILED  ] StringUtilTest.HumanReadableNumber

 2 FAILED TESTS

Although, I have reverted these changes back to our latest discussion.

varshneydevansh · 2023-07-13T15:19:15Z

I need help with the test cases. As I think I am missing something.

This is the code which I am currently modifying -

void ToExponentAndMantissa(double val, double thresh, int precision,
                           Counter::OneK one_k, std::string* mantissa,
                           int64_t* exponent) {
  std::stringstream mantissa_stream;

  if (val < 0) {
    mantissa_stream << "-";
    val = -val;
  }

  // Adjust threshold so that it never excludes things which can't be rendered
  // in 'precision' digits.
  const double adjusted_threshold =
      std::max(thresh, 1.0 / std::pow(10.0, precision));
  const double which_one_k = (one_k == Counter::kIs1024 ? 1024.0 : 1000.0);
  const double big_threshold = adjusted_threshold * which_one_k;
  const double small_threshold = adjusted_threshold;
  // Values in ]simple_threshold,small_threshold[ will be printed as-is
  const double simple_threshold = 0.01;

  if (val > big_threshold) {
    // Positive powers
    double scaled = val;
    for (size_t i = 0; i < arraysize(kBigSIUnits); ++i) {
      scaled /= which_one_k;
      if (scaled < big_threshold) {
        mantissa_stream << std::fixed << std::setprecision(precision) << scaled;
        *exponent = i + 1;
        *mantissa = mantissa_stream.str();
        return;
      }
    }
    mantissa_stream << std::fixed << std::setprecision(precision) << val;
    *exponent = 0;
  } else if (val < small_threshold) {
    // Negative powers
    if (val < simple_threshold) {
      double scaled = val;
      for (size_t i = 0; i < arraysize(kSmallSIUnits); ++i) {
        scaled *= which_one_k;
        if (scaled >= small_threshold) {
          mantissa_stream << std::fixed << std::setprecision(precision)
                          << scaled;
          *exponent = -static_cast<int64_t>(i + 1);
          *mantissa = mantissa_stream.str();
          return;
        }
      }
    }
    mantissa_stream << std::fixed << std::setprecision(precision) << val;
    *exponent = 0;
  } else {
    double scaled = val;
    size_t i = 0;
    while (scaled >= which_one_k && i < arraysize(kBigSIUnits)) {
      scaled /= which_one_k;
      i++;
    }
    mantissa_stream << std::fixed << std::setprecision(precision) << scaled;
    *exponent = i;
  }
  *mantissa = mantissa_stream.str();
}

I modified this else clause at the last -
It handles the case where the value val falls within the range between the small threshold and the big threshold.

else {
    double scaled = val;
    size_t i = 0;
    while (scaled >= which_one_k && i < arraysize(kBigSIUnits)) {
      scaled /= which_one_k;
      i++;
    }
    mantissa_stream << std::fixed << std::setprecision(precision) << scaled;
    *exponent = i;
  }
  *mantissa = mantissa_stream.str();
}

Now I am left with one failing test case -

➜  test git:(inconsistent_suffixes_console_reporter_1009) ✗ ./string_util_gtest 
Running main() from gmock_main.cc
[==========] Running 6 tests from 1 test suite.
[----------] Global test environment set-up.
[----------] 6 tests from StringUtilTest
[ RUN      ] StringUtilTest.stoul
[       OK ] StringUtilTest.stoul (0 ms)
[ RUN      ] StringUtilTest.stoi
[       OK ] StringUtilTest.stoi (0 ms)
[ RUN      ] StringUtilTest.stod
[       OK ] StringUtilTest.stod (0 ms)
[ RUN      ] StringUtilTest.StrSplit
[       OK ] StringUtilTest.StrSplit (0 ms)
[ RUN      ] StringUtilTest.AppendHumanReadable
n: 0, mantissa: 0, exponent: 0
n: 999, mantissa: 999, exponent: 0
n: 1000, mantissa: 1, exponent: 1
n: 1e+06, mantissa: 1, exponent: 2
n: 1e+09, mantissa: 1, exponent: 3
[       OK ] StringUtilTest.AppendHumanReadable (0 ms)
[ RUN      ] StringUtilTest.HumanReadableNumber
n: 1, mantissa: 1.0, exponent: 0
n: 1234, mantissa: 1.2, exponent: 1
n: 1e+06, mantissa: 976.6, exponent: 1
n: 1e+09, mantissa: 953.7, exponent: 2
n: 1, mantissa: 1.0, exponent: 0
n: 1234, mantissa: 1.2, exponent: 1
n: 1e+06, mantissa: 1000.0, exponent: 1
/home/devansh/benchmark/test/string_util_gtest.cc:197: Failure
Expected equality of these values:
  "1.0M"
  benchmark::HumanReadableNumber(1.0e6, benchmark::Counter::kIs1000)
    Which is: "1000.0k"
n: 1e+09, mantissa: 1000.0, exponent: 2
/home/devansh/benchmark/test/string_util_gtest.cc:199: Failure
Expected equality of these values:
  "1.0G"
  benchmark::HumanReadableNumber(1.0e9, benchmark::Counter::kIs1000)
    Which is: "1000.0M"
[  FAILED  ] StringUtilTest.HumanReadableNumber (0 ms)
[----------] 6 tests from StringUtilTest (0 ms total)

[----------] Global test environment tear-down
[==========] 6 tests from 1 test suite ran. (0 ms total)
[  PASSED  ] 5 tests.
[  FAILED  ] 1 test, listed below:
[  FAILED  ] StringUtilTest.HumanReadableNumber

 1 FAILED TEST

added this cout for the debugging purpose only.

std::string ToBinaryStringFullySpecified(
    double value, double threshold, int precision,
    Counter::OneK one_k = Counter::kIs1024) {
  std::string mantissa;
  int64_t exponent;
  ToExponentAndMantissa(value, threshold, precision, one_k, &mantissa,
                        &exponent);
  std::cout << "n: " << value << ", mantissa: " << mantissa
            << ", exponent: " << exponent << std::endl;

  return mantissa + ExponentToPrefix(exponent, one_k == Counter::kIs1024);
}

dmah42 · 2023-07-13T17:23:37Z

can you commit and push your local version of the code to the PR? then i can see the diff and the test failures here clearly.

src/string_util.cc

dmah42 · 2023-07-18T16:04:44Z

@dmah42 i do not understand where the change to the kBigIECUnits should manifest. We seem to be missing test coverage for that.

benchmark/src/string_util.cc

Line 110 in 27d64a2

return mantissa + ExponentToPrefix(exponent, one_k == Counter::kIs1024);

is where we determine IEC vs SI (and previously didn't).

called

benchmark/src/string_util.cc

Line 153 in 27d64a2

return ToBinaryStringFullySpecified(n, 1, one_k);

and tested directly

benchmark/test/string_util_gtest.cc

Line 212 in 27d64a2

TEST_P(HumanReadableFixture, HumanReadableNumber) {

and through console reporter https://github.com/google/benchmark/blob/27d64a2351b98d48dd5b18c75ff536982a4ce26a/test/user_counters_test.cc and https://github.com/google/benchmark/blob/27d64a2351b98d48dd5b18c75ff536982a4ce26a/test/user_counters_thousands_test.cc

the latter tests were updated as part of my change, but we still use SI for user counters by default unless it's overridden in the Counter constructor.

LebedevRI · 2023-07-18T16:15:29Z

@dmah42 i do not understand where the change to the kBigIECUnits should manifest. We seem to be missing test coverage for that.
<...>

but we still use SI for user counters by default unless it's overridden in the Counter constructor.

user_counters_test.cc does test the overrides, yes?
My question is more like: why does this not show up in any tests?

dmah42 · 2023-07-18T16:41:17Z

@dmah42 i do not understand where the change to the kBigIECUnits should manifest. We seem to be missing test coverage for that.
<...>

but we still use SI for user counters by default unless it's overridden in the Counter constructor.

user_counters_test.cc does test the overrides, yes?
My question is more like: why does this not show up in any tests?

I'm missing something, sorry. user counter thousands test has all the counter variants including the default.

what are you looking for and can't find in the tests?

LebedevRI · 2023-07-18T16:45:11Z

Is this change as it is now supposed to still cause externally-observable (in reporters,
not in the AppendHumanReadable() test) behavior change?

varshneydevansh · 2023-07-19T15:57:36Z

Should I remove the function AppendHumanReadable()? I even looked at other places, but HumanReadableNumber() was the one which is getting used in the code base.

…/github.com/varshneydevansh/benchmark into inconsistent_suffixes_console_reporter_1009

LebedevRI · 2023-07-20T23:49:19Z

Please fix the missing newlines at the end of files i mentioned, otherwise i suppose this is good to go?

varshneydevansh · 2023-07-21T17:40:19Z

I have checked it with clang-format and even have run the test cases, these new lines have no impact on as of my understanding. I think we are good to go.

src/string_util.h

test/string_util_gtest.cc

LebedevRI · 2023-07-31T17:47:29Z

(@dmah42 merge?)

dmah42 · 2023-07-31T17:59:35Z

(@dmah42 merge?)

still shows as changes requested.. I was waiting for those.

dmah42 · 2023-08-01T07:47:18Z

amazing. thank you.

varshneydevansh · 2023-08-01T11:08:44Z

I actually got a little lost with the statement regarding the new line. I did ask about this from a few of my friends, but they also couldn't get it well as English is not our native language. Maybe I should have asked with you guys regarding that.

Thank you, Dominic and Roman, for guiding me. The use of pointers and memory access and how to improve efficiency of code by carefully getting rid of assignment headers. I learned so much. 🙌

varshneydevansh added 2 commits July 11, 2023 17:40

bug: updated double one_k to be an enum and SI IEC to multi-character…

32e7c8a

… strings

Merge branch 'inconsistent_suffixes_console_reporter_1009' of https:/…

ddff6e4

…/github.com/varshneydevansh/benchmark into inconsistent_suffixes_console_reporter_1009

dmah42 requested changes Jul 11, 2023

View reviewed changes

src/string_util.cc Outdated Show resolved Hide resolved

src/string_util.cc Outdated Show resolved Hide resolved

src/string_util.cc Outdated Show resolved Hide resolved

src/string_util.cc Outdated Show resolved Hide resolved

varshneydevansh added 4 commits July 11, 2023 21:24

made changes according to cpp-styleguide according to review

3e300bb

Merge branch 'main' into inconsistent_suffixes_console_reporter_1009

1ebc863

moved the OneK enum from Counter class to benchmark namespace of stri…

802a2d4

…ng_util.h

Merge branch 'inconsistent_suffixes_console_reporter_1009' of https:/…

af31c83

…/github.com/varshneydevansh/benchmark into inconsistent_suffixes_console_reporter_1009

dmah42 reviewed Jul 11, 2023

View reviewed changes

include/benchmark/benchmark.h Outdated Show resolved Hide resolved

Reverted the enum OneK back to its place in Counter class as it's par…

0180b69

…t of Public API

updated string_util_gtest and formated according to clang

aeca121

dmah42 reviewed Jul 12, 2023

View reviewed changes

include/benchmark/benchmark.h Outdated Show resolved Hide resolved

dmah42 reviewed Jul 12, 2023

View reviewed changes

test/string_util_gtest.cc Outdated Show resolved Hide resolved

dmah42 reviewed Jul 12, 2023

View reviewed changes

test/string_util_gtest.cc Outdated Show resolved Hide resolved

varshneydevansh added 4 commits July 12, 2023 22:19

Merge branch 'google:main' into inconsistent_suffixes_console_reporte…

c5d5ce2

…r_1009

Updated the test case file according to the public API

ca67301

Merge branch 'inconsistent_suffixes_console_reporter_1009' of https:/…

6e7c102

…/github.com/varshneydevansh/benchmark into inconsistent_suffixes_console_reporter_1009

reverted clang-format from benchmark.h at line 715

68fd962

Made changes according to discussin #issuecomment-1633849641

7bdbf44

modified else clause in ToExponentAndMantissa( )

9e3f156

varshneydevansh commented Jul 13, 2023

View reviewed changes

src/string_util.cc Show resolved Hide resolved

varshneydevansh added 4 commits July 20, 2023 12:32

removed appendHumanReadable as it was not used anywhere

0a45a1f

Merge branch 'main' into inconsistent_suffixes_console_reporter_1009

53883b5

clang-formatted the file again

fe5e4be

Merge branch 'inconsistent_suffixes_console_reporter_1009' of https:/…

2d50a0d

…/github.com/varshneydevansh/benchmark into inconsistent_suffixes_console_reporter_1009

varshneydevansh added 2 commits July 26, 2023 22:17

added newline back

e40eb4f

Merge branch 'main' into inconsistent_suffixes_console_reporter_1009

d414e38

LebedevRI reviewed Jul 31, 2023

View reviewed changes

src/string_util.h Outdated Show resolved Hide resolved

LebedevRI reviewed Jul 31, 2023

View reviewed changes

test/string_util_gtest.cc Outdated Show resolved Hide resolved

dmah42 and others added 3 commits July 31, 2023 17:00

Merge branch 'main' into inconsistent_suffixes_console_reporter_1009

85b3021

correctly added the new lines at last

f1feda2

Merge branch 'main' into inconsistent_suffixes_console_reporter_1009

9845056

LebedevRI approved these changes Jul 31, 2023

View reviewed changes

LebedevRI requested review from LebedevRI and dmah42 July 31, 2023 18:03

dmah42 approved these changes Aug 1, 2023

View reviewed changes

dmah42 merged commit 02a354f into google:main Aug 1, 2023

varshneydevansh deleted the inconsistent_suffixes_console_reporter_1009 branch August 1, 2023 11:08

This was referenced Jan 31, 2024

[BUG] Weird behavior of bytes_per_second and items_per_second #1749

Closed

[C++] Bump bundled Google benchmark version apache/arrow#39863

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bug: Inconsistent suffixes console reporter 1009 #1631

bug: Inconsistent suffixes console reporter 1009 #1631

varshneydevansh commented Jul 11, 2023 •

edited

Loading

dmah42 commented Jul 11, 2023

varshneydevansh commented Jul 11, 2023

varshneydevansh commented Jul 11, 2023

varshneydevansh commented Jul 12, 2023

varshneydevansh commented Jul 12, 2023

dmah42 commented Jul 13, 2023

dmah42 commented Jul 13, 2023

I did find my one mistake with the following function, where I missed changing the type of `one_k` from double to enum.

varshneydevansh commented Jul 13, 2023

varshneydevansh commented Jul 13, 2023

dmah42 commented Jul 13, 2023

dmah42 commented Jul 18, 2023

LebedevRI commented Jul 18, 2023

dmah42 commented Jul 18, 2023

LebedevRI commented Jul 18, 2023

varshneydevansh commented Jul 19, 2023

LebedevRI commented Jul 20, 2023

varshneydevansh commented Jul 21, 2023

LebedevRI commented Jul 31, 2023

dmah42 commented Jul 31, 2023

dmah42 commented Aug 1, 2023

varshneydevansh commented Aug 1, 2023 •

edited

Loading

bug: Inconsistent suffixes console reporter 1009 #1631

bug: Inconsistent suffixes console reporter 1009 #1631

Conversation

varshneydevansh commented Jul 11, 2023 • edited Loading

dmah42 commented Jul 11, 2023

varshneydevansh commented Jul 11, 2023

varshneydevansh commented Jul 11, 2023

varshneydevansh commented Jul 12, 2023

varshneydevansh commented Jul 12, 2023

I did find my one mistake with the following function, where I missed changing the type of one_k from double to enum.

dmah42 commented Jul 13, 2023

dmah42 commented Jul 13, 2023

I did find my one mistake with the following function, where I missed changing the type of one_k from double to enum.

varshneydevansh commented Jul 13, 2023

varshneydevansh commented Jul 13, 2023

dmah42 commented Jul 13, 2023

dmah42 commented Jul 18, 2023

LebedevRI commented Jul 18, 2023

dmah42 commented Jul 18, 2023

LebedevRI commented Jul 18, 2023

varshneydevansh commented Jul 19, 2023

LebedevRI commented Jul 20, 2023

varshneydevansh commented Jul 21, 2023

LebedevRI commented Jul 31, 2023

dmah42 commented Jul 31, 2023

dmah42 commented Aug 1, 2023

varshneydevansh commented Aug 1, 2023 • edited Loading

varshneydevansh commented Jul 11, 2023 •

edited

Loading

I did find my one mistake with the following function, where I missed changing the type of `one_k` from double to enum.

I did find my one mistake with the following function, where I missed changing the type of `one_k` from double to enum.

varshneydevansh commented Aug 1, 2023 •

edited

Loading