From e6fb890eace1e3f524c4ee124dea23091cb9980d Mon Sep 17 00:00:00 2001 From: Justin King Date: Thu, 8 Oct 2026 15:07:41 -0700 Subject: [PATCH] Remove arena pointer from `SmallByteStringRep` Small string data is not actually owned by any arena, so preserving the arena pointer does not make sense. This allows us to have 23 bytes of inline storage instead of 15. PiperOrigin-RevId: 996097021 --- common/internal/byte_string.cc | 11 +++++------ common/internal/byte_string.h | 28 ++++++++-------------------- common/internal/byte_string_test.cc | 11 ++++++----- common/values/string_value.cc | 4 ---- 4 files changed, 19 insertions(+), 35 deletions(-) diff --git a/common/internal/byte_string.cc b/common/internal/byte_string.cc index 4daaa66a1..cbf28f422 100644 --- a/common/internal/byte_string.cc +++ b/common/internal/byte_string.cc @@ -37,7 +37,7 @@ ByteString ByteString::From(absl::string_view value, ABSL_DCHECK(arena != nullptr); ByteString result(UninitializedTag{}); if (value.size() <= kSmallByteStringCapacity) { - result.SetSmall(arena, value); + result.SetSmall(value); } else { char* arena_value = reinterpret_cast(arena->AllocateAligned(value.size())); @@ -52,7 +52,7 @@ ByteString ByteString::From(absl::Cord value, ABSL_DCHECK(arena != nullptr); ByteString result(UninitializedTag{}); if (value.size() <= kSmallByteStringCapacity) { - result.SetSmall(arena, value); + result.SetSmall(value); } else { result.SetLarge(arena, google::protobuf::Arena::Create(arena, std::move(value))); @@ -65,7 +65,7 @@ ByteString ByteString::From(std::string&& value, ABSL_DCHECK(arena != nullptr); ByteString result(UninitializedTag{}); if (value.size() <= kSmallByteStringCapacity) { - result.SetSmall(arena, value); + result.SetSmall(value); } else if (value.size() > sizeof(std::string)) { value.shrink_to_fit(); result.SetMedium( @@ -107,7 +107,6 @@ ByteString ByteString::Concat(const ByteString& lhs, const ByteString& rhs, // If the resulting string fits in inline storage, do it. result.rep_.header.kind = ByteStringKind::kSmall; result.rep_.small.size = result_size; - result.rep_.small.arena = arena; lhs.CopyToArray(result.rep_.small.data); rhs.CopyToArray(result.rep_.small.data + lhs_size); } else { @@ -221,7 +220,7 @@ ByteString ByteString::Substring(size_t pos, size_t npos) const { switch (GetKind()) { case ByteStringKind::kSmall: { ByteString result(UninitializedTag{}); - result.SetSmall(GetSmallArena(), GetSmall().substr(pos, npos - pos)); + result.SetSmall(GetSmall().substr(pos, npos - pos)); return result; } case ByteStringKind::kMedium: { @@ -420,7 +419,7 @@ ByteString ByteString::Clone(google::protobuf::Arena* absl_nonnull arena) const switch (GetKind()) { case ByteStringKind::kSmall: { ByteString result(UninitializedTag{}); - result.SetSmall(arena, GetSmall()); + result.SetSmall(GetSmall()); return result; } case ByteStringKind::kMedium: { diff --git a/common/internal/byte_string.h b/common/internal/byte_string.h index 6027d08ec..b9fb222a8 100644 --- a/common/internal/byte_string.h +++ b/common/internal/byte_string.h @@ -74,8 +74,7 @@ struct SmallByteStringRep final { #ifdef _MSC_VER #pragma pack(pop) #endif - char data[23 - sizeof(google::protobuf::Arena*)]; - google::protobuf::Arena* absl_nullable arena; + char data[23]; }; inline constexpr size_t kSmallByteStringCapacity = @@ -215,7 +214,7 @@ class [[nodiscard]] ByteString final { static ByteString Concat(const ByteString& lhs, const ByteString& rhs, google::protobuf::Arena* absl_nonnull arena); - ByteString() noexcept { SetSmallEmpty(nullptr); } + ByteString() noexcept { SetSmallEmpty(); } ByteString(const ByteString&) = default; ByteString(ByteString&&) = default; @@ -303,10 +302,12 @@ class [[nodiscard]] ByteString final { std::string* absl_nonnull scratch ABSL_ATTRIBUTE_LIFETIME_BOUND) const ABSL_ATTRIBUTE_LIFETIME_BOUND; + // Returns the arena which owns the underling data for this byte string. + // Returns null when the data is not owned by an arena. google::protobuf::Arena* absl_nullable GetArena() const { switch (GetKind()) { case ByteStringKind::kSmall: - return GetSmallArena(); + return nullptr; case ByteStringKind::kMedium: return GetMediumArena(); case ByteStringKind::kLarge: @@ -373,16 +374,6 @@ class [[nodiscard]] ByteString final { return absl::string_view(rep.data, rep.size); } - google::protobuf::Arena* absl_nullable GetSmallArena() const { - ABSL_DCHECK_EQ(GetKind(), ByteStringKind::kSmall); - return GetSmallArena(rep_.small); - } - - static google::protobuf::Arena* absl_nullable GetSmallArena( - const SmallByteStringRep& rep) { - return rep.arena; - } - google::protobuf::Arena* absl_nullable GetMediumArena() const { ABSL_DCHECK_EQ(GetKind(), ByteStringKind::kMedium); return GetMediumArena(rep_.medium); @@ -413,27 +404,24 @@ class [[nodiscard]] ByteString final { return rep.arena; } - void SetSmallEmpty(google::protobuf::Arena* absl_nullable arena) { + void SetSmallEmpty() { rep_.header.kind = ByteStringKind::kSmall; rep_.small.size = 0; - rep_.small.arena = arena; } - void SetSmall(google::protobuf::Arena* absl_nullable arena, absl::string_view string) { + void SetSmall(absl::string_view string) { ABSL_DCHECK_LE(string.size(), kSmallByteStringCapacity); rep_.header.kind = ByteStringKind::kSmall; rep_.small.size = string.size(); - rep_.small.arena = arena; if (!string.empty()) { std::memcpy(rep_.small.data, string.data(), rep_.small.size); } } - void SetSmall(google::protobuf::Arena* absl_nullable arena, const absl::Cord& cord) { + void SetSmall(const absl::Cord& cord) { ABSL_DCHECK_LE(cord.size(), kSmallByteStringCapacity); rep_.header.kind = ByteStringKind::kSmall; rep_.small.size = cord.size(); - rep_.small.arena = arena; CopyCordToArray(cord, rep_.small.data); } diff --git a/common/internal/byte_string_test.cc b/common/internal/byte_string_test.cc index 121158ada..b0cdee814 100644 --- a/common/internal/byte_string_test.cc +++ b/common/internal/byte_string_test.cc @@ -40,6 +40,7 @@ namespace { using ::testing::_; using ::testing::Eq; using ::testing::IsEmpty; +using ::testing::IsNull; using ::testing::Not; using ::testing::Optional; using ::testing::SizeIs; @@ -114,7 +115,7 @@ TEST_F(ByteStringTest, Default) { TEST_F(ByteStringTest, ConstructNullDataStringView) { ByteString byte_string = ByteString::From(absl::string_view(), GetArena()); EXPECT_THAT(byte_string, IsEmpty()); - EXPECT_EQ(byte_string.GetArena(), GetArena()); + EXPECT_THAT(byte_string.GetArena(), IsNull()); } TEST_F(ByteStringTest, ConstructSmallCString) { @@ -124,7 +125,7 @@ TEST_F(ByteStringTest, ConstructSmallCString) { EXPECT_THAT(byte_string, Not(IsEmpty())); EXPECT_EQ(byte_string, GetSmallStringView()); EXPECT_EQ(GetKind(byte_string), ByteStringKind::kSmall); - EXPECT_EQ(byte_string.GetArena(), GetArena()); + EXPECT_THAT(byte_string.GetArena(), IsNull()); } TEST_F(ByteStringTest, ConstructMediumCString) { @@ -143,7 +144,7 @@ TEST_F(ByteStringTest, ConstructSmallRValueString) { EXPECT_THAT(byte_string, Not(IsEmpty())); EXPECT_EQ(byte_string, GetSmallStringView()); EXPECT_EQ(GetKind(byte_string), ByteStringKind::kSmall); - EXPECT_EQ(byte_string.GetArena(), GetArena()); + EXPECT_THAT(byte_string.GetArena(), IsNull()); } TEST_F(ByteStringTest, ConstructSmallLValueString) { @@ -153,7 +154,7 @@ TEST_F(ByteStringTest, ConstructSmallLValueString) { EXPECT_THAT(byte_string, Not(IsEmpty())); EXPECT_EQ(byte_string, GetSmallStringView()); EXPECT_EQ(GetKind(byte_string), ByteStringKind::kSmall); - EXPECT_EQ(byte_string.GetArena(), GetArena()); + EXPECT_THAT(byte_string.GetArena(), IsNull()); } TEST_F(ByteStringTest, ConstructMediumRValueString) { @@ -181,7 +182,7 @@ TEST_F(ByteStringTest, ConstructSmallCord) { EXPECT_THAT(byte_string, Not(IsEmpty())); EXPECT_EQ(byte_string, GetSmallStringView()); EXPECT_EQ(GetKind(byte_string), ByteStringKind::kSmall); - EXPECT_EQ(byte_string.GetArena(), GetArena()); + EXPECT_THAT(byte_string.GetArena(), IsNull()); } TEST_F(ByteStringTest, ConstructMediumOrLargeCord) { diff --git a/common/values/string_value.cc b/common/values/string_value.cc index 6aa7ac543..b9495fa69 100644 --- a/common/values/string_value.cc +++ b/common/values/string_value.cc @@ -616,7 +616,6 @@ Value StringValue::Substring(int64_t start, std::memcpy(result.value_.rep_.small.data, value_.rep_.small.data + *status_or_index, result.value_.rep_.small.size); - result.value_.rep_.small.arena = value_.rep_.small.arena; return result; } case common_internal::ByteStringKind::kMedium: { @@ -744,7 +743,6 @@ Value StringValue::Substring(int64_t start, int64_t end, std::memcpy(result.value_.rep_.small.data, value_.rep_.small.data + status_or_indices->first, result.value_.rep_.small.size); - result.value_.rep_.small.arena = value_.rep_.small.arena; return result; } case common_internal::ByteStringKind::kMedium: { @@ -1430,7 +1428,6 @@ Value StringValue::CharAt(int64_t pos, common_internal::ByteStringKind::kSmall; result.value_.rep_.small.size = cel::internal::Utf8Encode( code_point, result.value_.rep_.small.data); - result.value_.rep_.small.arena = value_.GetArena(); return result; } rep.remove_prefix(code_units); @@ -1460,7 +1457,6 @@ Value StringValue::CharAt(int64_t pos, common_internal::ByteStringKind::kSmall; result.value_.rep_.small.size = cel::internal::Utf8Encode( code_point, result.value_.rep_.small.data); - result.value_.rep_.small.arena = nullptr; return result; } absl::Cord::Advance(&begin, code_units);