Skip to content

Remove arena pointer from SmallByteStringRep #2340

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
copybara-service merged 1 commit into from
Oct 8, 2026
+19 −35
Merged

Remove arena pointer from SmallByteStringRep #2340

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 5 additions & 6 deletions common/internal/byte_string.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<char*>(arena->AllocateAligned(value.size()));
Expand All @@ -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<absl::Cord>(arena, std::move(value)));
Expand All @@ -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(
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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: {
Expand Down Expand Up @@ -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: {
Expand Down
28 changes: 8 additions & 20 deletions common/internal/byte_string.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 =
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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:
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
}

Expand Down
11 changes: 6 additions & 5 deletions common/internal/byte_string_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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) {
Expand All @@ -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) {
Expand All @@ -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) {
Expand All @@ -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) {
Expand Down Expand Up @@ -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) {
Expand Down
4 changes: 0 additions & 4 deletions common/values/string_value.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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: {
Expand Down Expand Up @@ -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: {
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down
Loading