Skip to content

Commit

Permalink
StrCat: do not use intermediate buffer when result fits in SSO.
Browse files Browse the repository at this point in the history
PiperOrigin-RevId: 557811632
Change-Id: I370fa17d2fb82a1f1ca86f84529bae31b34b18e4
  • Loading branch information
Abseil Team authored and copybara-github committed Aug 17, 2023
1 parent 9377c75 commit 65d7b6d
Show file tree
Hide file tree
Showing 3 changed files with 91 additions and 0 deletions.
64 changes: 64 additions & 0 deletions absl/strings/str_cat.h
Original file line number Diff line number Diff line change
Expand Up @@ -89,16 +89,21 @@

#include <algorithm>
#include <array>
#include <cassert>
#include <cstddef>
#include <cstdint>
#include <cstring>
#include <limits>
#include <string>
#include <type_traits>
#include <utility>
#include <vector>

#include "absl/base/attributes.h"
#include "absl/base/port.h"
#include "absl/meta/type_traits.h"
#include "absl/strings/internal/has_absl_stringify.h"
#include "absl/strings/internal/resize_uninitialized.h"
#include "absl/strings/internal/stringify_sink.h"
#include "absl/strings/numbers.h"
#include "absl/strings/string_view.h"
Expand Down Expand Up @@ -444,10 +449,69 @@ std::string CatPieces(std::initializer_list<absl::string_view> pieces);
void AppendPieces(std::string* dest,
std::initializer_list<absl::string_view> pieces);

template <typename Integer>
std::string IntegerToString(Integer i) {
// Any integer (signed/unsigned) up to 64 bits can be formatted into a buffer
// with 22 bytes (including NULL at the end).
constexpr size_t kMaxDigits10 = 22;
std::string result;
strings_internal::STLStringResizeUninitialized(&result, kMaxDigits10);
char* start = &result[0];
// note: this can be optimized to not write last zero.
char* end = numbers_internal::FastIntToBuffer(i, start);
auto size = static_cast<size_t>(end - start);
assert((size < result.size()) &&
"StrCat(Integer) does not fit into kMaxDigits10");
result.erase(size);
return result;
}
template <typename Float>
std::string FloatToString(Float f) {
std::string result;
strings_internal::STLStringResizeUninitialized(
&result, numbers_internal::kSixDigitsToBufferSize);
char* start = &result[0];
result.erase(numbers_internal::SixDigitsToBuffer(f, start));
return result;
}

// `SingleArgStrCat` overloads take built-in `int`, `long` and `long long` types
// (signed / unsigned) to avoid ambiguity on the call side. If we used int32_t
// and int64_t, then at least one of the three (`int` / `long` / `long long`)
// would have been ambiguous when passed to `SingleArgStrCat`.
inline std::string SingleArgStrCat(int x) { return IntegerToString(x); }
inline std::string SingleArgStrCat(unsigned int x) {
return IntegerToString(x);
}
// NOLINTNEXTLINE
inline std::string SingleArgStrCat(long x) { return IntegerToString(x); }
// NOLINTNEXTLINE
inline std::string SingleArgStrCat(unsigned long x) {
return IntegerToString(x);
}
// NOLINTNEXTLINE
inline std::string SingleArgStrCat(long long x) { return IntegerToString(x); }
// NOLINTNEXTLINE
inline std::string SingleArgStrCat(unsigned long long x) {
return IntegerToString(x);
}
inline std::string SingleArgStrCat(float x) { return FloatToString(x); }
inline std::string SingleArgStrCat(double x) { return FloatToString(x); }


template <typename T, typename = std::enable_if_t<std::is_arithmetic<T>{} &&
!std::is_same<T, char>{}>>
using EnableIfFastCase = T;

} // namespace strings_internal

ABSL_MUST_USE_RESULT inline std::string StrCat() { return std::string(); }

template <typename T>
ABSL_MUST_USE_RESULT inline std::string StrCat(
strings_internal::EnableIfFastCase<T> a) {
return strings_internal::SingleArgStrCat(a);
}
ABSL_MUST_USE_RESULT inline std::string StrCat(const AlphaNum& a) {
return std::string(a.data(), a.size());
}
Expand Down
11 changes: 11 additions & 0 deletions absl/strings/str_cat_benchmark.cc
Original file line number Diff line number Diff line change
Expand Up @@ -188,4 +188,15 @@ void StrAppendConfig(B* benchmark) {

BENCHMARK(BM_StrAppend)->Apply(StrAppendConfig);

void BM_StrCat_int(benchmark::State& state) {
int i = 0;
for (auto s : state) {
std::string result = absl::StrCat(i);
benchmark::DoNotOptimize(result);
i = IncrementAlternatingSign(i);
}
}

BENCHMARK(BM_StrCat_int);

} // namespace
16 changes: 16 additions & 0 deletions absl/strings/str_cat_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -665,4 +665,20 @@ TEST(StrCat, AbslStringifyWithEnum) {
EXPECT_EQ(absl::StrCat(e), "Choices");
}

template <typename Integer>
void CheckSingleArgumentIntegerLimits() {
Integer max = std::numeric_limits<Integer>::max();
Integer min = std::numeric_limits<Integer>::min();

EXPECT_EQ(absl::StrCat(max), std::to_string(max));
EXPECT_EQ(absl::StrCat(min), std::to_string(min));
}

TEST(StrCat, SingleArgumentLimits) {
CheckSingleArgumentIntegerLimits<int32_t>();
CheckSingleArgumentIntegerLimits<uint32_t>();
CheckSingleArgumentIntegerLimits<int64_t>();
CheckSingleArgumentIntegerLimits<uint64_t>();
}

} // namespace

5 comments on commit 65d7b6d

@MBkkt
Copy link
Contributor

@MBkkt MBkkt commented on 65d7b6d Sep 8, 2023

Choose a reason for hiding this comment

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

@derekmauro
It's possible to use this optimization only for libc++?
Just make this code under defined(_LIBCPP_VERSION)?
Because for other libraries it will be always allocate string, even in cases when it's not necessary

Also if you're interested in optimization of StrCat:
Now AlphaNum always stored buffer which length is 32
Did you try to use different buffer size for different ctor?

@derekmauro
Copy link
Member

Choose a reason for hiding this comment

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

I was neither the author nor a reviewer of this change, but I agree that this looks like a regression when not using libc++.

I believe the two issues here are that

  1. The SSO size on libstc++ and MSSTL are both 15, which is too small.
  2. Probably slightly less important, we don't have a true STLStringResizeUninitialized on libstdc++ or MSSTL.

I'll get this patched. It's a bit unfortunate that since our production platform uses libc++, many changes don't get benchmarked on other platforms, so thank you for reporting this.

Now there are a lot of recent changes to this code to support AbslStringify that I haven't followed too closely. Can you say more about the other optimization you have in mind?

@MBkkt
Copy link
Contributor

@MBkkt MBkkt commented on 65d7b6d Sep 9, 2023

Choose a reason for hiding this comment

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

Probably slightly less important, we don't have a true STLStringResizeUninitialized on libstdc++ or MSSTL.

Yes, I know, I can't understand why libstdc++ cannot make some members public :(
I know folly makes access to the private fields, but it looks like it generates a lot of templates

I'll get this patched.

Thanks! I appreciate that

Can you say more about the other optimization you have in mind?

I will make an issue

@MBkkt
Copy link
Contributor

@MBkkt MBkkt commented on 65d7b6d Sep 9, 2023

Choose a reason for hiding this comment

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

@derekmauro
Copy link
Member

Choose a reason for hiding this comment

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

f3eae68 fixes the performance regression.

Please sign in to comment.