bug: Inconsistent suffixes console reporter 1009

varshneydevansh commented 1 year ago

Issue ->

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

dmah42 commented 1 year ago

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

varshneydevansh commented 1 year ago

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 commented 1 year ago

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

varshneydevansh commented 12 months ago

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 commented 12 months ago

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 commented 12 months ago

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 commented 12 months ago

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 commented 12 months ago

#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 commented 12 months ago

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 commented 12 months ago

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.

dmah42 commented 12 months ago

there's a bug in the existing implementation, which is making your life harder.

std::string ToBinaryStringFullySpecified(double value, double threshold,
                                         int precision, double one_k = 1024.0) {
  std::string mantissa;
  int64_t exponent;
  ToExponentAndMantissa(value, threshold, precision, one_k, &mantissa,
                        &exponent);
  return mantissa + ExponentToPrefix(exponent, false);
}

ie, we pass in one_k to ToExponentAndMantissa and then ignore it in ExponentToPrefix. i suspect "just" fixing this would make things more consistent directly.. i will try this locally to see if i can get some tests to pass. then we can see about what we need to improve.

dmah42 commented 12 months ago

i have tests locally that are passing with minimal changes to the code. i'll create a pull request for that and then we should rebase your work onto it.

dmah42 commented 12 months ago

i have tests locally that are passing with minimal changes to the code. i'll create a pull request for that and then we should rebase your work onto it.

https://github.com/google/benchmark/pull/1632

dmah42 commented 12 months ago

i think you're bringing in some old changes here with the rebase.

i think the only thing we need is to decide if AppendHumanReadable is intended to be SI (per the comment) or IEC (per the current implementation) and make that one change (and then update the tests).

i think "human readable" suggests SI units in which case the comment is correct and we just need to add the right enum to the call to ToBinaryStringFullySpecified. i'd also remove the default while you're there so all the calls are explicit.

varshneydevansh commented 12 months ago

This is exactly what I was going to ask you. I merged the changes introduced by you in my current local repo.

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

std::string HumanReadableNumber(double n, Counter::OneK one_k) {
  // 1 means that we should show one decimal place of precision.
  return ToBinaryStringFullySpecified(n, 1, one_k);
}

using AppendHumanReadableFixture =
    ::testing::TestWithParam<std::tuple<int, std::string>>;

INSTANTIATE_TEST_SUITE_P(
    AppendHumanReadableTests, AppendHumanReadableFixture,
    ::testing::Values(std::make_tuple(0, "0"), std::make_tuple(999, "999"),
                      std::make_tuple(1000, "1000"),
                      std::make_tuple(1024, "1Ki"),
                      std::make_tuple(1000 * 1000, "976\\.56.Ki"),
                      std::make_tuple(1024 * 1024, "1Mi"),
                      std::make_tuple(1000 * 1000 * 1000, "953\\.674Mi"),
                      std::make_tuple(1024 * 1024 * 1024, "1Gi")));

TEST_P(AppendHumanReadableFixture, AppendHumanReadable) {
  std::string str;
  benchmark::AppendHumanReadable(std::get<0>(GetParam()), &str);
  ASSERT_THAT(str, ::testing::MatchesRegex(std::get<1>(GetParam())));
}

This is the output which we are now getting -

I want to ask since appendHumanReadable() is going to return the SI values, which means Testing for IEC values is also going to return the result in the term of SI value.

[----------] 4 tests from StringUtilTest (1 ms total)

[----------] 8 tests from AppendHumanReadableTests/AppendHumanReadableFixture
[ RUN      ] AppendHumanReadableTests/AppendHumanReadableFixture.AppendHumanReadable/0
[       OK ] AppendHumanReadableTests/AppendHumanReadableFixture.AppendHumanReadable/0 (0 ms)
[ RUN      ] AppendHumanReadableTests/AppendHumanReadableFixture.AppendHumanReadable/1
[       OK ] AppendHumanReadableTests/AppendHumanReadableFixture.AppendHumanReadable/1 (0 ms)
[ RUN      ] AppendHumanReadableTests/AppendHumanReadableFixture.AppendHumanReadable/2
/home/devansh/benchmark/test/string_util_gtest.cc:180: Failure
Value of: str
Expected: matches regular expression "1000"
  Actual: "1k"
[  FAILED  ] AppendHumanReadableTests/AppendHumanReadableFixture.AppendHumanReadable/2, where GetParam() = (1000, "1000") (0 ms)
[ RUN      ] AppendHumanReadableTests/AppendHumanReadableFixture.AppendHumanReadable/3
/home/devansh/benchmark/test/string_util_gtest.cc:180: Failure
Value of: str
Expected: matches regular expression "1Ki"
  Actual: "1k"
[  FAILED  ] AppendHumanReadableTests/AppendHumanReadableFixture.AppendHumanReadable/3, where GetParam() = (1024, "1Ki") (0 ms)
[ RUN      ] AppendHumanReadableTests/AppendHumanReadableFixture.AppendHumanReadable/4
/home/devansh/benchmark/test/string_util_gtest.cc:180: Failure
Value of: str
Expected: matches regular expression "976\\.56.Ki"
  Actual: "1M"
[  FAILED  ] AppendHumanReadableTests/AppendHumanReadableFixture.AppendHumanReadable/4, where GetParam() = (1000000, "976\\.56.Ki") (0 ms)
[ RUN      ] AppendHumanReadableTests/AppendHumanReadableFixture.AppendHumanReadable/5
/home/devansh/benchmark/test/string_util_gtest.cc:180: Failure
Value of: str
Expected: matches regular expression "1Mi"
  Actual: "1M"
[  FAILED  ] AppendHumanReadableTests/AppendHumanReadableFixture.AppendHumanReadable/5, where GetParam() = (1048576, "1Mi") (0 ms)
[ RUN      ] AppendHumanReadableTests/AppendHumanReadableFixture.AppendHumanReadable/6
/home/devansh/benchmark/test/string_util_gtest.cc:180: Failure
Value of: str
Expected: matches regular expression "953\\.674Mi"
  Actual: "1G"
[  FAILED  ] AppendHumanReadableTests/AppendHumanReadableFixture.AppendHumanReadable/6, where GetParam() = (1000000000, "953\\.674Mi") (0 ms)
[ RUN      ] AppendHumanReadableTests/AppendHumanReadableFixture.AppendHumanReadable/7
/home/devansh/benchmark/test/string_util_gtest.cc:180: Failure
Value of: str
Expected: matches regular expression "1Gi"
  Actual: "1G"
[  FAILED  ] AppendHumanReadableTests/AppendHumanReadableFixture.AppendHumanReadable/7, where GetParam() = (1073741824, "1Gi") (0 ms)
[----------] 8 tests from AppendHumanReadableTests/AppendHumanReadableFixture (0 ms total)

[----------] 16 tests from HumanReadableTests/HumanReadableFixture
[ RUN      ] HumanReadableTests/HumanReadableFixture.HumanReadableNumber/0
/home/devansh/benchmark/test/string_util_gtest.cc:215: Failure
Value of: str
Expected: matches regular expression "0"
  Actual: "0.0"
[  FAILED  ] HumanReadableTests/HumanReadableFixture.HumanReadableNumber/0, where GetParam() = (0, 1024, "0") (0 ms)
[ RUN      ] HumanReadableTests/HumanReadableFixture.HumanReadableNumber/1
/home/devansh/benchmark/test/string_util_gtest.cc:215: Failure
Value of: str
Expected: matches regular expression "999"
  Actual: "999.0"
[  FAILED  ] HumanReadableTests/HumanReadableFixture.HumanReadableNumber/1, where GetParam() = (999, 1024, "999") (0 ms)
[ RUN      ] HumanReadableTests/HumanReadableFixture.HumanReadableNumber/2
/home/devansh/benchmark/test/string_util_gtest.cc:215: Failure
Value of: str
Expected: matches regular expression "1000"
  Actual: "1000.0"
[  FAILED  ] HumanReadableTests/HumanReadableFixture.HumanReadableNumber/2, where GetParam() = (1000, 1024, "1000") (0 ms)
[ RUN      ] HumanReadableTests/HumanReadableFixture.HumanReadableNumber/3
/home/devansh/benchmark/test/string_util_gtest.cc:215: Failure
Value of: str
Expected: matches regular expression "1Ki"
  Actual: "1.0k"
[  FAILED  ] HumanReadableTests/HumanReadableFixture.HumanReadableNumber/3, where GetParam() = (1024, 1024, "1Ki") (0 ms)
[ RUN      ] HumanReadableTests/HumanReadableFixture.HumanReadableNumber/4
/home/devansh/benchmark/test/string_util_gtest.cc:215: Failure
Value of: str
Expected: matches regular expression "976\\.56.Ki"
  Actual: "976.6k"
[  FAILED  ] HumanReadableTests/HumanReadableFixture.HumanReadableNumber/4, where GetParam() = (1e+06, 1024, "976\\.56.Ki") (0 ms)
[ RUN      ] HumanReadableTests/HumanReadableFixture.HumanReadableNumber/5
/home/devansh/benchmark/test/string_util_gtest.cc:215: Failure
Value of: str
Expected: matches regular expression "1Mi"
  Actual: "1.0M"
[  FAILED  ] HumanReadableTests/HumanReadableFixture.HumanReadableNumber/5, where GetParam() = (1.04858e+06, 1024, "1Mi") (0 ms)
[ RUN      ] HumanReadableTests/HumanReadableFixture.HumanReadableNumber/6
/home/devansh/benchmark/test/string_util_gtest.cc:215: Failure
Value of: str
Expected: matches regular expression "953\\.674Mi"
  Actual: "953.7M"
[  FAILED  ] HumanReadableTests/HumanReadableFixture.HumanReadableNumber/6, where GetParam() = (1e+09, 1024, "953\\.674Mi") (0 ms)
[ RUN      ] HumanReadableTests/HumanReadableFixture.HumanReadableNumber/7
/home/devansh/benchmark/test/string_util_gtest.cc:215: Failure
Value of: str
Expected: matches regular expression "1Gi"
  Actual: "1.0G"
[  FAILED  ] HumanReadableTests/HumanReadableFixture.HumanReadableNumber/7, where GetParam() = (1.07374e+09, 1024, "1Gi") (0 ms)
[ RUN      ] HumanReadableTests/HumanReadableFixture.HumanReadableNumber/8
/home/devansh/benchmark/test/string_util_gtest.cc:215: Failure
Value of: str
Expected: matches regular expression "0"
  Actual: "0.0"
[  FAILED  ] HumanReadableTests/HumanReadableFixture.HumanReadableNumber/8, where GetParam() = (0, 1000, "0") (0 ms)
[ RUN      ] HumanReadableTests/HumanReadableFixture.HumanReadableNumber/9
/home/devansh/benchmark/test/string_util_gtest.cc:215: Failure
Value of: str
Expected: matches regular expression "999"
  Actual: "999.0"
[  FAILED  ] HumanReadableTests/HumanReadableFixture.HumanReadableNumber/9, where GetParam() = (999, 1000, "999") (0 ms)
[ RUN      ] HumanReadableTests/HumanReadableFixture.HumanReadableNumber/10
/home/devansh/benchmark/test/string_util_gtest.cc:215: Failure
Value of: str
Expected: matches regular expression "1k"
  Actual: "1.0k"
[  FAILED  ] HumanReadableTests/HumanReadableFixture.HumanReadableNumber/10, where GetParam() = (1000, 1000, "1k") (0 ms)
[ RUN      ] HumanReadableTests/HumanReadableFixture.HumanReadableNumber/11
/home/devansh/benchmark/test/string_util_gtest.cc:215: Failure
Value of: str
Expected: matches regular expression "1.024k"
  Actual: "1.0k"
[  FAILED  ] HumanReadableTests/HumanReadableFixture.HumanReadableNumber/11, where GetParam() = (1024, 1000, "1.024k") (0 ms)
[ RUN      ] HumanReadableTests/HumanReadableFixture.HumanReadableNumber/12
/home/devansh/benchmark/test/string_util_gtest.cc:215: Failure
Value of: str
Expected: matches regular expression "1M"
  Actual: "1.0M"
[  FAILED  ] HumanReadableTests/HumanReadableFixture.HumanReadableNumber/12, where GetParam() = (1e+06, 1000, "1M") (0 ms)
[ RUN      ] HumanReadableTests/HumanReadableFixture.HumanReadableNumber/13
/home/devansh/benchmark/test/string_util_gtest.cc:215: Failure
Value of: str
Expected: matches regular expression "1\\.04858M"
  Actual: "1.0M"
[  FAILED  ] HumanReadableTests/HumanReadableFixture.HumanReadableNumber/13, where GetParam() = (1.04858e+06, 1000, "1\\.04858M") (0 ms)
[ RUN      ] HumanReadableTests/HumanReadableFixture.HumanReadableNumber/14
/home/devansh/benchmark/test/string_util_gtest.cc:215: Failure
Value of: str
Expected: matches regular expression "1G"
  Actual: "1.0G"
[  FAILED  ] HumanReadableTests/HumanReadableFixture.HumanReadableNumber/14, where GetParam() = (1e+09, 1000, "1G") (0 ms)
[ RUN      ] HumanReadableTests/HumanReadableFixture.HumanReadableNumber/15
/home/devansh/benchmark/test/string_util_gtest.cc:215: Failure
Value of: str
Expected: matches regular expression "1\\.07374G"
  Actual: "1.1G"
[  FAILED  ] HumanReadableTests/HumanReadableFixture.HumanReadableNumber/15, where GetParam() = (1.07374e+09, 1000, "1\\.07374G") (0 ms)
[----------] 16 tests from HumanReadableTests/HumanReadableFixture (1 ms total)

[----------] Global test environment tear-down
[==========] 28 tests from 3 test suites ran. (3 ms total)
[  PASSED  ] 6 tests.
[  FAILED  ] 22 tests, listed below:
[  FAILED  ] AppendHumanReadableTests/AppendHumanReadableFixture.AppendHumanReadable/2, where GetParam() = (1000, "1000")
[  FAILED  ] AppendHumanReadableTests/AppendHumanReadableFixture.AppendHumanReadable/3, where GetParam() = (1024, "1Ki")
[  FAILED  ] AppendHumanReadableTests/AppendHumanReadableFixture.AppendHumanReadable/4, where GetParam() = (1000000, "976\\.56.Ki")
[  FAILED  ] AppendHumanReadableTests/AppendHumanReadableFixture.AppendHumanReadable/5, where GetParam() = (1048576, "1Mi")
[  FAILED  ] AppendHumanReadableTests/AppendHumanReadableFixture.AppendHumanReadable/6, where GetParam() = (1000000000, "953\\.674Mi")
[  FAILED  ] AppendHumanReadableTests/AppendHumanReadableFixture.AppendHumanReadable/7, where GetParam() = (1073741824, "1Gi")
[  FAILED  ] HumanReadableTests/HumanReadableFixture.HumanReadableNumber/0, where GetParam() = (0, 1024, "0")
[  FAILED  ] HumanReadableTests/HumanReadableFixture.HumanReadableNumber/1, where GetParam() = (999, 1024, "999")
[  FAILED  ] HumanReadableTests/HumanReadableFixture.HumanReadableNumber/2, where GetParam() = (1000, 1024, "1000")
[  FAILED  ] HumanReadableTests/HumanReadableFixture.HumanReadableNumber/3, where GetParam() = (1024, 1024, "1Ki")
[  FAILED  ] HumanReadableTests/HumanReadableFixture.HumanReadableNumber/4, where GetParam() = (1e+06, 1024, "976\\.56.Ki")
[  FAILED  ] HumanReadableTests/HumanReadableFixture.HumanReadableNumber/5, where GetParam() = (1.04858e+06, 1024, "1Mi")
[  FAILED  ] HumanReadableTests/HumanReadableFixture.HumanReadableNumber/6, where GetParam() = (1e+09, 1024, "953\\.674Mi")
[  FAILED  ] HumanReadableTests/HumanReadableFixture.HumanReadableNumber/7, where GetParam() = (1.07374e+09, 1024, "1Gi")
[  FAILED  ] HumanReadableTests/HumanReadableFixture.HumanReadableNumber/8, where GetParam() = (0, 1000, "0")
[  FAILED  ] HumanReadableTests/HumanReadableFixture.HumanReadableNumber/9, where GetParam() = (999, 1000, "999")
[  FAILED  ] HumanReadableTests/HumanReadableFixture.HumanReadableNumber/10, where GetParam() = (1000, 1000, "1k")
[  FAILED  ] HumanReadableTests/HumanReadableFixture.HumanReadableNumber/11, where GetParam() = (1024, 1000, "1.024k")
[  FAILED  ] HumanReadableTests/HumanReadableFixture.HumanReadableNumber/12, where GetParam() = (1e+06, 1000, "1M")
[  FAILED  ] HumanReadableTests/HumanReadableFixture.HumanReadableNumber/13, where GetParam() = (1.04858e+06, 1000, "1\\.04858M")
[  FAILED  ] HumanReadableTests/HumanReadableFixture.HumanReadableNumber/14, where GetParam() = (1e+09, 1000, "1G")
[  FAILED  ] HumanReadableTests/HumanReadableFixture.HumanReadableNumber/15, where GetParam() = (1.07374e+09, 1000, "1\\.07374G")

22 FAILED TESTS

varshneydevansh commented 12 months ago

And yes, I have created a new branch where I am just modifying your updated changes with the proposed one.

Changing the -

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

and

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

std::string HumanReadableNumber(double n, Counter::OneK one_k) {
  return ToBinaryStringFullySpecified(n, 1, one_k);
}

varshneydevansh commented 12 months ago

I made a couple of changes to the test case for AppendHumanReadable() in my new branch and are left with 3 failing test cases -

[ RUN      ] AppendHumanReadableTests/AppendHumanReadableFixture.AppendHumanReadable/3
/home/devansh/benchmark/test/string_util_gtest.cc:179: Failure
Value of: str
Expected: matches regular expression "1Ki"
  Actual: "1.024k"
[  FAILED  ] AppendHumanReadableTests/AppendHumanReadableFixture.AppendHumanReadable/3, where GetParam() = (1024, "1Ki") (0 ms)
[ RUN      ] AppendHumanReadableTests/AppendHumanReadableFixture.AppendHumanReadable/4
[       OK ] AppendHumanReadableTests/AppendHumanReadableFixture.AppendHumanReadable/4 (0 ms)
[ RUN      ] AppendHumanReadableTests/AppendHumanReadableFixture.AppendHumanReadable/5
/home/devansh/benchmark/test/string_util_gtest.cc:179: Failure
Value of: str
Expected: matches regular expression "1Mi"
  Actual: "1.04858M"
[  FAILED  ] AppendHumanReadableTests/AppendHumanReadableFixture.AppendHumanReadable/5, where GetParam() = (1048576, "1Mi") (0 ms)
[ RUN      ] AppendHumanReadableTests/AppendHumanReadableFixture.AppendHumanReadable/6
[       OK ] AppendHumanReadableTests/AppendHumanReadableFixture.AppendHumanReadable/6 (0 ms)
[ RUN      ] AppendHumanReadableTests/AppendHumanReadableFixture.AppendHumanReadable/7
/home/devansh/benchmark/test/string_util_gtest.cc:179: Failure
Value of: str
Expected: matches regular expression "1Gi"
  Actual: "1.07374G"
[  FAILED  ] AppendHumanReadableTests/AppendHumanReadableFixture.AppendHumanReadable/7, where GetParam() = (1073741824, "1Gi") (0 ms)
[----------] 8 tests from AppendHumanReadableTests/AppendHumanReadableFixture (0 ms total)

Should, I merge this branch with this PR or copy the changes from this new branch to the current PR branch?

All we now left with changes in test case and probably have to look at the output of AppendHumanReadable() why it's giving more decimal places for those IEC values.

varshneydevansh commented 11 months ago

From past two days I have been thinking of creating a new PR for this as I think I have messed a lot of code here.

dmah42 commented 11 months ago

From past two days I have been thinking of creating a new PR for this as I think I have messed a lot of code here.

From past two days I have been thinking of creating a new PR for this as I think I have messed a lot of code here.

i think your current set of changes are fine, you just need to figure out what you expect AppendHumanReadable to return and then update the test to match. the HEAD version assumes kIs1024. that's changed. so what test cases are now invalid?

varshneydevansh commented 11 months ago

Hi @dmah42,

I have been tweaking the code for the past 4 hours to determine the correct patch/ behavior which I want from AppendHumanReadable(int n, std::string* str).

As far as I am able to understand, the behavior of the methods void AppendHumanReadable(int n, std::string* str) and std::string HumanReadableNumber(double n, Counter::OneK one_k) are the same with the only difference is that when user just wanna see the result quickly they will use the AppendHumanReadable which is somewhat the same as Big O analysis where we are somewhat concerned with the highest degree of the polynomial.

The same goes with the case of AppendHumanReadable here we want the result to be rounded down to the nearest SI prefix by that I mean is they are not particularly not concerned with the other value in the same number.

I even tried to convert the returning string from ToBinaryStringFullySpecified in this method to an integer, then appending the last SI value to it.

As per my understanding, the test cases should be like this (quick readable number) -

 INSTANTIATE_TEST_SUITE_P(
    AppendHumanReadableTests, AppendHumanReadableFixture,
    ::testing::Values(std::make_tuple(0, "0"), std::make_tuple(999, "999"),
                      std::make_tuple(1000, "1k"), std::make_tuple(1024, "1k"),
                      std::make_tuple(1000 * 1000, "1M"),
                      std::make_tuple(1234 * 1000, "1M"),
                      std::make_tuple(1024 * 1024, "1M"),
                      std::make_tuple(1000 * 1000 * 1000, "1G"),
                      std::make_tuple(1024 * 1024 * 1024, "1G")));

But, if we want the precise values, then we have the method HumanReadableNumber() which works as expected.

I was even looking into this PR - https://github.com/google/benchmark/pull/1607/files

dmah42 commented 11 months ago

I don't think we want it rounded down, just consistent. so if it should be SI then we should use SI. the issue today is that we expect SI but get IEC (and before my PR we got IEC with SI suffixes I think...).

so just switching from kIs1024 to kIs1000 and updating the tests should be sufficient for the main issue, as I understand it.

varshneydevansh commented 11 months ago

Passing all the test cases. Pushing the changes from the test file- string_util_gtest.cc

[----------] Global test environment tear-down
[==========] 29 tests from 3 test suites ran. (1 ms total)
[  PASSED  ] 29 tests.
➜  test git:(inconsistent_suffixes_console_reporter_1009) ✗

using AppendHumanReadableFixture =
    ::testing::TestWithParam<std::tuple<int, std::string>>;

INSTANTIATE_TEST_SUITE_P(
    AppendHumanReadableTests, AppendHumanReadableFixture,
    ::testing::Values(std::make_tuple(0, "0"), std::make_tuple(999, "999"),
                      std::make_tuple(1000, "1k"), std::make_tuple(1024, "1.024k"),
                      std::make_tuple(1000 * 1000, "1M"),
                      std::make_tuple(1234 * 1000, "1.234M"),
                      std::make_tuple(1024 * 1024, "1.04858M"),
                      std::make_tuple(1000 * 1000 * 1000, "1G"),
                      std::make_tuple(1024 * 1024 * 1024, "1.07374G")));

TEST_P(AppendHumanReadableFixture, AppendHumanReadable) {
  std::string str;
  benchmark::AppendHumanReadable(std::get<0>(GetParam()), &str);
  ASSERT_THAT(str, ::testing::MatchesRegex(std::get<1>(GetParam())));
}

varshneydevansh commented 11 months ago

Can I pick this challenge? [FR] cc_libraries should use "includes", not "strip_include_prefix"- 1512 https://github.com/google/benchmark/issues/1512

I did understand how Bazel works

dmah42 commented 11 months ago

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

https://github.com/google/benchmark/blob/27d64a2351b98d48dd5b18c75ff536982a4ce26a/src/string_util.cc#L110 is where we determine IEC vs SI (and previously didn't).

called https://github.com/google/benchmark/blob/27d64a2351b98d48dd5b18c75ff536982a4ce26a/src/string_util.cc#L153 and tested directly https://github.com/google/benchmark/blob/27d64a2351b98d48dd5b18c75ff536982a4ce26a/test/string_util_gtest.cc#L212 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 commented 11 months ago

@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 commented 11 months ago

@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 commented 11 months ago

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

varshneydevansh commented 11 months ago

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.

LebedevRI commented 11 months ago

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

varshneydevansh commented 11 months ago

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.

LebedevRI commented 11 months ago

(@dmah42 merge?)

dmah42 commented 11 months ago

(@dmah42 merge?)

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

dmah42 commented 11 months ago

amazing. thank you.

varshneydevansh commented 11 months ago

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. 🙌

google / benchmark