Conversation
| // rounding mode, so retain the old path when callers select one other than | ||
| // round-to-nearest. The old path is also the fallback for standard libraries | ||
| // without floating-point to_chars. | ||
| if (std::isfinite(d) && std::fegetround() == FE_TONEAREST) { |
There was a problem hiding this comment.
When we wrote this I don't think we considered floating point rounding modes. <cfenv> is actually a banned header at Google.
I actually think std::fegetround() == FE_TONEAREST is the opposite of what we want. absl::SimpleAtod() is rounding-mode independent, so if we fallback using a non-default rounding mode, we will actually break round-tripping.
So I think this is actually broken today under non-default rounding modes. Removing the std::fegetround() == FE_TONEAREST test will fix this on all platforms that support floating-point to_chars.
I think we can just accept the old broken behavior on older compilers until they are deprecated and the fallback is removed.
| EXPECT_STREQ(test_case.expected, buffer); | ||
|
|
||
| double parsed = 0; | ||
| EXPECT_TRUE(SimpleAtod(buffer, &parsed)); |
There was a problem hiding this comment.
This doesn't compile. It needs to be absl::SimpleAtod.
| for (auto _ : state) { | ||
| benchmark::DoNotOptimize( | ||
| absl::numbers_internal::RoundTripDoubleToBuffer(test_case.value, | ||
| buffer)); |
There was a problem hiding this comment.
Likely needs benchmark::DoNotOptimize(buffer) too.
| double d, char* absl_nonnull buffer) { | ||
| auto to_chars = [buffer, d](int precision) { | ||
| std::to_chars_result result = std::to_chars( | ||
| buffer, buffer + numbers_internal::kFastToBufferSize, d, |
There was a problem hiding this comment.
I think this needs to be numbers_internal::kFastToBufferSize - 1, because unlike snprintf, it doesn't NUL-terminate. Line 444 would write outside the buffer.
This isn't an issue today because numbers_internal::kFastToBufferSize has headroom, but we should still practice correctness.
| #if defined(__GLIBCXX__) && defined(_GLIBCXX_RELEASE) && \ | ||
| _GLIBCXX_RELEASE >= 11 | ||
| #define ABSL_INTERNAL_HAVE_STD_TO_CHARS_FLOAT 1 | ||
| #elif defined(_LIBCPP_VERSION) && _LIBCPP_VERSION >= 14000 |
There was a problem hiding this comment.
I can't tell you how many times we've broken Apple because their standard library uses availability attributes. We should make this:
#elif defined(_LIBCPP_VERSION) && _LIBCPP_VERSION >= 14000 && \
(!defined(_LIBCPP_AVAILABILITY_HAS_TO_CHARS_FLOATING_POINT) || _LIBCPP_AVAILABILITY_HAS_TO_CHARS_FLOATING_POINT)
3 participants
RoundTripDoubleToBuffer currently uses snprintf and a parse-back check for
finite doubles. On standard libraries that provide floating-point
std::to_chars, this change uses std::chars_format::general with the existing
15/17-digit precision policy and absl::from_chars for the retry check.
The fast path is gated by standard-library support rather than compiler
version. The existing snprintf implementation remains the fallback for
unsupported standard libraries and for non-FE_TONEAREST rounding modes, where
std::to_chars would not preserve the current runtime rounding behavior.
The existing behavior for NaNs, signed zero, locale-independent output, the
DBL_MAX guard, buffer sizing, and round-trip retry semantics is preserved.
This work originated from protobuf/protobuf#29763, where a Protobuf maintainer
suggested moving the optimization into Abseil so downstream users can share the
same formatting primitive.
Correctness validation on GCC 15/libstdc++ 15:
Performance
Pinned to one CPU on an Intel i5-12500H, seven repetitions, median ns/op:
Representative perf stat measurements also showed substantial reductions in
retired work. Across the tested distributions, CPU cycles fell by roughly
57–84% and retired instructions by roughly 59–86%.