Skip to content

Refactor how inline string fields are tracked for donation. #22413

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

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Refactor how inline string fields are tracked for donation. #22413

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
46 changes: 4 additions & 42 deletions src/google/protobuf/compiler/cpp/field.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -323,52 +323,21 @@ void HasBitVars(const FieldDescriptor* field, const Options& opts,
vars.emplace_back(Sub("set_hasbit", set).WithSuffix(";"));
vars.emplace_back(Sub("clear_hasbit", clr).WithSuffix(";"));
}

void InlinedStringVars(const FieldDescriptor* field, const Options& opts,
std::optional<uint32_t> idx, std::vector<Sub>& vars) {
if (!IsStringInlined(field, opts)) {
ABSL_CHECK(!idx.has_value());
return;
}

// The first bit is the tracking bit for on demand registering ArenaDtor.
ABSL_CHECK_GT(*idx, 0u)
<< "_inlined_string_donated_'s bit 0 is reserved for arena dtor tracking";

int32_t index = *idx / 32;
std::string mask = absl::StrFormat("0x%08xU", 1u << (*idx % 32));
vars.emplace_back("inlined_string_index", index);
vars.emplace_back("inlined_string_mask", mask);

absl::string_view array = IsMapEntryMessage(field->containing_type())
? "_inlined_string_donated_"
: "_impl_._inlined_string_donated_";

vars.emplace_back("inlined_string_donated",
absl::StrFormat("(%s[%d] & %s) != 0;", array, index, mask));
vars.emplace_back("donating_states_word",
absl::StrFormat("%s[%d]", array, index));
vars.emplace_back("mask_for_undonate", absl::StrFormat("~%s", mask));
}
} // namespace

FieldGenerator::FieldGenerator(const FieldDescriptor* field,
const Options& options,
MessageSCCAnalyzer* scc_analyzer,
std::optional<uint32_t> hasbit_index,
std::optional<uint32_t> inlined_string_index)
std::optional<uint32_t> hasbit_index)
: impl_(MakeGenerator(field, options, scc_analyzer)),
field_vars_(FieldVars(field, options)),
tracker_vars_(MakeTrackerCalls(field, options)),
per_generator_vars_(impl_->MakeVars()) {
HasBitVars(field, options, hasbit_index, field_vars_);
InlinedStringVars(field, options, inlined_string_index, field_vars_);
}

void FieldGeneratorTable::Build(
const Options& options, MessageSCCAnalyzer* scc,
absl::Span<const int32_t> has_bit_indices,
absl::Span<const int32_t> inlined_string_indices) {
void FieldGeneratorTable::Build(const Options& options, MessageSCCAnalyzer* scc,
absl::Span<const int32_t> has_bit_indices) {
// Construct all the FieldGenerators.
fields_.reserve(static_cast<size_t>(descriptor_->field_count()));
for (const auto* field : internal::FieldRange(descriptor_)) {
Expand All @@ -378,14 +347,7 @@ void FieldGeneratorTable::Build(
has_bit_index = static_cast<uint32_t>(has_bit_indices[index]);
}

std::optional<uint32_t> inlined_string_index;
if (!inlined_string_indices.empty() && inlined_string_indices[index] >= 0) {
inlined_string_index =
static_cast<uint32_t>(inlined_string_indices[index]);
}

fields_.push_back(FieldGenerator(field, options, scc, has_bit_index,
inlined_string_index));
fields_.push_back(FieldGenerator(field, options, scc, has_bit_index));
}
}

Expand Down
6 changes: 2 additions & 4 deletions src/google/protobuf/compiler/cpp/field.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -503,8 +503,7 @@ class FieldGenerator {
friend class FieldGeneratorTable;
FieldGenerator(const FieldDescriptor* field, const Options& options,
MessageSCCAnalyzer* scc_analyzer,
std::optional<uint32_t> hasbit_index,
std::optional<uint32_t> inlined_string_index);
std::optional<uint32_t> hasbit_index);

std::unique_ptr<FieldGeneratorBase> impl_;
std::vector<io::Printer::Sub> field_vars_;
Expand All @@ -522,8 +521,7 @@ class FieldGeneratorTable {
FieldGeneratorTable& operator=(const FieldGeneratorTable&) = delete;

void Build(const Options& options, MessageSCCAnalyzer* scc_analyzer,
absl::Span<const int32_t> has_bit_indices,
absl::Span<const int32_t> inlined_string_indices);
absl::Span<const int32_t> has_bit_indices);

const FieldGenerator& get(const FieldDescriptor* field) const {
ABSL_CHECK_EQ(field->containing_type(), descriptor_);
Expand Down
151 changes: 44 additions & 107 deletions src/google/protobuf/compiler/cpp/field_generators/string_field.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,10 +78,6 @@ class SingularString : public FieldGeneratorBase {

bool IsInlined() const override { return is_inlined(); }

ArenaDtorNeeds NeedsArenaDestructor() const override {
return is_inlined() ? ArenaDtorNeeds::kOnDemand : ArenaDtorNeeds::kNone;
}

void GeneratePrivateMembers(io::Printer* p) const override {
// Skips the automatic destruction if inlined; rather calls it explicitly if
// allocating arena is null.
Expand Down Expand Up @@ -114,16 +110,6 @@ class SingularString : public FieldGeneratorBase {
}
}

void GenerateArenaDestructorCode(io::Printer* p) const override {
if (!is_inlined()) return;

p->Emit(R"cc(
if (!_this->_internal_$name$_donated()) {
_this->$field_$.~InlinedStringField();
}
)cc");
}

void GenerateNonInlineAccessorDefinitions(io::Printer* p) const override {
if (EmptyDefault()) return;
p->Emit(R"cc(
Expand Down Expand Up @@ -257,33 +243,25 @@ void SingularString::GenerateAccessorDeclarations(io::Printer* p) const {
auto v3 = p->WithVars(
AnnotatedAccessors(field_, {"mutable_"}, AnnotationCollector::kAlias));

p->Emit({{"donated",
[&] {
if (!is_inlined()) return;
p->Emit(R"cc(
PROTOBUF_ALWAYS_INLINE bool _internal_$name$_donated() const;
)cc");
}}},
R"cc(
$DEPRECATED$ const ::std::string& $name$() const;
//~ Using `Arg_ = const std::string&` will make the type of `arg`
//~ default to `const std::string&`, due to reference collapse. This
//~ is necessary because there are a handful of users that rely on
//~ this default.
template <typename Arg_ = const ::std::string&, typename... Args_>
$DEPRECATED$ void $set_name$(Arg_&& arg, Args_... args);
$DEPRECATED$ ::std::string* $nonnull$ $mutable_name$();
$DEPRECATED$ [[nodiscard]] ::std::string* $nullable$ $release_name$();
$DEPRECATED$ void $set_allocated_name$(::std::string* $nullable$ value);

private:
const ::std::string& _internal_$name$() const;
PROTOBUF_ALWAYS_INLINE void _internal_set_$name$(const ::std::string& value);
::std::string* $nonnull$ _internal_mutable_$name$();
$donated$;

public:
)cc");
p->Emit(R"cc(
$DEPRECATED$ const ::std::string& $name$() const;
//~ Using `Arg_ = const std::string&` will make the type of `arg`
//~ default to `const std::string&`, due to reference collapse. This
//~ is necessary because there are a handful of users that rely on
//~ this default.
template <typename Arg_ = const ::std::string&, typename... Args_>
$DEPRECATED$ void $set_name$(Arg_&& arg, Args_... args);
$DEPRECATED$ ::std::string* $nonnull$ $mutable_name$();
$DEPRECATED$ [[nodiscard]] ::std::string* $nullable$ $release_name$();
$DEPRECATED$ void $set_allocated_name$(::std::string* $nullable$ value);

private:
const ::std::string& _internal_$name$() const;
PROTOBUF_ALWAYS_INLINE void _internal_set_$name$(const ::std::string& value);
::std::string* $nonnull$ _internal_mutable_$name$();

public:
)cc");
}

void UpdateHasbitSet(io::Printer* p, bool is_oneof) {
Expand All @@ -304,16 +282,6 @@ void UpdateHasbitSet(io::Printer* p, bool is_oneof) {
)cc");
}

void ArgsForSetter(io::Printer* p, bool inlined) {
if (!inlined) {
p->Emit("GetArena()");
return;
}
p->Emit(
"GetArena(), _internal_$name_internal$_donated(), "
"&$donating_states_word$, $mask_for_undonate$, this");
}

void SingularString::ReleaseImpl(io::Printer* p) const {
if (is_oneof()) {
p->Emit(R"cc(
Expand All @@ -340,7 +308,7 @@ void SingularString::ReleaseImpl(io::Printer* p) const {
}
$clear_hasbit$;

return $field_$.Release(GetArena(), _internal_$name_internal$_donated());
return $field_$.Release(GetArena());
)cc");
return;
}
Expand All @@ -362,7 +330,7 @@ void SingularString::ReleaseImpl(io::Printer* p) const {
p->Emit(R"cc(
auto* released = $field_$.Release();
if ($pbi$::DebugHardenForceCopyDefaultString()) {
$field_$.Set("", $set_args$);
$field_$.Set("", GetArena());
}
return released;
)cc");
Expand Down Expand Up @@ -392,22 +360,18 @@ void SingularString::SetAllocatedImpl(io::Printer* p) const {
)cc");
}

p->Emit(R"cc(
$field_$.SetAllocated(value, GetArena());
)cc");

if (is_inlined()) {
// Currently, string fields with default value can't be inlined.
p->Emit(R"cc(
$field_$.SetAllocated(nullptr, value, $set_args$);
)cc");
return;
}

p->Emit(R"cc(
$field_$.SetAllocated(value, $set_args$);
)cc");

if (EmptyDefault()) {
p->Emit(R"cc(
if ($pbi$::DebugHardenForceCopyDefaultString() && $field_$.IsDefault()) {
$field_$.Set("", $set_args$);
$field_$.Set("", GetArena());
}
)cc");
}
Expand All @@ -425,7 +389,6 @@ void SingularString::GenerateInlineAccessorDefinitions(io::Printer* p) const {
)cc");
}},
{"update_hasbit", [&] { UpdateHasbitSet(p, is_oneof()); }},
{"set_args", [&] { ArgsForSetter(p, is_inlined()); }},
{"check_hasbit",
[&] {
if (!is_oneof()) return;
Expand Down Expand Up @@ -456,7 +419,7 @@ void SingularString::GenerateInlineAccessorDefinitions(io::Printer* p) const {
$TsanDetectConcurrentMutation$;
$PrepareSplitMessageForWrite$;
$update_hasbit$;
$field_$.$Set$(static_cast<Arg_&&>(arg), args..., $set_args$);
$field_$.$Set$(static_cast<Arg_&&>(arg), args..., GetArena());
$annotate_set$;
// @@protoc_insertion_point(field_set:$pkg.Msg.field$)
}
Expand All @@ -479,12 +442,12 @@ void SingularString::GenerateInlineAccessorDefinitions(io::Printer* p) const {
$update_hasbit$;
//~ Don't use $Set$ here; we always want the std::string variant
//~ regardless of whether this is a `bytes` field.
$field_$.Set(value, $set_args$);
$field_$.Set(value, GetArena());
}
inline ::std::string* $nonnull$ $Msg$::_internal_mutable_$name_internal$() {
$TsanDetectConcurrentMutation$;
$update_hasbit$;
return $field_$.Mutable($lazy_args$, $set_args$);
return $field_$.Mutable($lazy_args$, GetArena());
}
inline ::std::string* $nullable$ $Msg$::$release_name$() {
$WeakDescriptorSelfPin$;
Expand All @@ -503,14 +466,6 @@ void SingularString::GenerateInlineAccessorDefinitions(io::Printer* p) const {
// @@protoc_insertion_point(field_set_allocated:$pkg.Msg.field$)
})cc";
p->Emit(vars, code);

if (is_inlined()) {
p->Emit(R"cc(
inline bool $Msg$::_internal_$name_internal$_donated() const {
return $inlined_string_donated$;
}
)cc");
}
}

void SingularString::GenerateClearingCode(io::Printer* p) const {
Expand Down Expand Up @@ -595,14 +550,8 @@ void SingularString::GenerateSwappingCode(io::Printer* p) const {
}

p->Emit(R"cc(
{
bool lhs_dtor_registered = ($inlined_string_donated_array$[0] & 1) == 0;
bool rhs_dtor_registered =
(other->$inlined_string_donated_array$[0] & 1) == 0;
::_pbi::InlinedStringField::InternalSwap(
&$field_$, lhs_dtor_registered, this, &other->$field_$,
rhs_dtor_registered, other, arena);
}
::_pbi::InlinedStringField::InternalSwap(&$field_$, &other->$field_$,
arena);
)cc");
}

Expand Down Expand Up @@ -632,31 +581,19 @@ void SingularString::GenerateCopyConstructorCode(io::Printer* p) const {
)cc");
}

p->Emit(
{{"hazzer",
[&] {
if (HasHasbit(field_)) {
p->Emit(R"cc((from.$has_hasbit$) != 0)cc");
} else {
p->Emit(R"cc(!from._internal_$name$().empty())cc");
}
}},
{"set_args",
[&] {
if (!is_inlined()) {
p->Emit("_this->GetArena()");
} else {
p->Emit(
"_this->GetArena(), "
"_this->_internal_$name$_donated(), "
"&_this->$donating_states_word$, $mask_for_undonate$, _this");
}
}}},
R"cc(
if ($hazzer$) {
_this->$field_$.Set(from._internal_$name$(), $set_args$);
}
)cc");
p->Emit({{"hazzer",
[&] {
if (HasHasbit(field_)) {
p->Emit(R"cc((from.$has_hasbit$) != 0)cc");
} else {
p->Emit(R"cc(!from._internal_$name$().empty())cc");
}
}}},
R"cc(
if ($hazzer$) {
_this->$field_$.Set(from._internal_$name$(), _this->GetArena());
}
)cc");
}

void SingularString::GenerateDestructorCode(io::Printer* p) const {
Expand Down
Loading
Loading