Skip to content

Commit ba27c15

Browse files
sbenzaquencopybara-github
sbenzaquen
authored and
copybara-github
committed
Refactor how inline string fields are tracked for donation.
Instead of a separate HasBits object with one bit per field we track donation in one arbitrary bit in the capacity. When the string IsLong that bit in the capacity determines if it is donated. We also remove all the plumbing for these donation bits, on demand arena destructor, etc. PiperOrigin-RevId: 770243667
1 parent 7b1cebd commit ba27c15

31 files changed

+256
-1013
lines changed

‎src/google/protobuf/compiler/cpp/field.cc‎

Lines changed: 4 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -393,52 +393,21 @@ void HasBitVars(const FieldDescriptor* field, const Options& opts,
393393
vars.emplace_back("exclude_mask",
394394
absl::StrFormat("0x%08xU", ~(1u << (*idx % 32))));
395395
}
396-
397-
void InlinedStringVars(const FieldDescriptor* field, const Options& opts,
398-
absl::optional<uint32_t> idx, std::vector<Sub>& vars) {
399-
if (!IsStringInlined(field, opts)) {
400-
ABSL_CHECK(!idx.has_value());
401-
return;
402-
}
403-
404-
// The first bit is the tracking bit for on demand registering ArenaDtor.
405-
ABSL_CHECK_GT(*idx, 0u)
406-
<< "_inlined_string_donated_'s bit 0 is reserved for arena dtor tracking";
407-
408-
int32_t index = *idx / 32;
409-
std::string mask = absl::StrFormat("0x%08xU", 1u << (*idx % 32));
410-
vars.emplace_back("inlined_string_index", index);
411-
vars.emplace_back("inlined_string_mask", mask);
412-
413-
absl::string_view array = IsMapEntryMessage(field->containing_type())
414-
? "_inlined_string_donated_"
415-
: "_impl_._inlined_string_donated_";
416-
417-
vars.emplace_back("inlined_string_donated",
418-
absl::StrFormat("(%s[%d] & %s) != 0;", array, index, mask));
419-
vars.emplace_back("donating_states_word",
420-
absl::StrFormat("%s[%d]", array, index));
421-
vars.emplace_back("mask_for_undonate", absl::StrFormat("~%s", mask));
422-
}
423396
} // namespace
424397

425398
FieldGenerator::FieldGenerator(const FieldDescriptor* field,
426399
const Options& options,
427400
MessageSCCAnalyzer* scc_analyzer,
428-
absl::optional<uint32_t> hasbit_index,
429-
absl::optional<uint32_t> inlined_string_index)
401+
absl::optional<uint32_t> hasbit_index)
430402
: impl_(MakeGenerator(field, options, scc_analyzer)),
431403
field_vars_(FieldVars(field, options)),
432404
tracker_vars_(MakeTrackerCalls(field, options)),
433405
per_generator_vars_(impl_->MakeVars()) {
434406
HasBitVars(field, options, hasbit_index, field_vars_);
435-
InlinedStringVars(field, options, inlined_string_index, field_vars_);
436407
}
437408

438-
void FieldGeneratorTable::Build(
439-
const Options& options, MessageSCCAnalyzer* scc,
440-
absl::Span<const int32_t> has_bit_indices,
441-
absl::Span<const int32_t> inlined_string_indices) {
409+
void FieldGeneratorTable::Build(const Options& options, MessageSCCAnalyzer* scc,
410+
absl::Span<const int32_t> has_bit_indices) {
442411
// Construct all the FieldGenerators.
443412
fields_.reserve(static_cast<size_t>(descriptor_->field_count()));
444413
for (const auto* field : internal::FieldRange(descriptor_)) {
@@ -448,14 +417,7 @@ void FieldGeneratorTable::Build(
448417
has_bit_index = static_cast<uint32_t>(has_bit_indices[index]);
449418
}
450419

451-
absl::optional<uint32_t> inlined_string_index;
452-
if (!inlined_string_indices.empty() && inlined_string_indices[index] >= 0) {
453-
inlined_string_index =
454-
static_cast<uint32_t>(inlined_string_indices[index]);
455-
}
456-
457-
fields_.push_back(FieldGenerator(field, options, scc, has_bit_index,
458-
inlined_string_index));
420+
fields_.push_back(FieldGenerator(field, options, scc, has_bit_index));
459421
}
460422
}
461423

‎src/google/protobuf/compiler/cpp/field.h‎

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -493,8 +493,7 @@ class FieldGenerator {
493493
friend class FieldGeneratorTable;
494494
FieldGenerator(const FieldDescriptor* field, const Options& options,
495495
MessageSCCAnalyzer* scc_analyzer,
496-
absl::optional<uint32_t> hasbit_index,
497-
absl::optional<uint32_t> inlined_string_index);
496+
absl::optional<uint32_t> hasbit_index);
498497

499498
std::unique_ptr<FieldGeneratorBase> impl_;
500499
std::vector<io::Printer::Sub> field_vars_;
@@ -512,8 +511,7 @@ class FieldGeneratorTable {
512511
FieldGeneratorTable& operator=(const FieldGeneratorTable&) = delete;
513512

514513
void Build(const Options& options, MessageSCCAnalyzer* scc_analyzer,
515-
absl::Span<const int32_t> has_bit_indices,
516-
absl::Span<const int32_t> inlined_string_indices);
514+
absl::Span<const int32_t> has_bit_indices);
517515

518516
const FieldGenerator& get(const FieldDescriptor* field) const {
519517
ABSL_CHECK_EQ(field->containing_type(), descriptor_);

‎src/google/protobuf/compiler/cpp/field_generators/string_field.cc‎

Lines changed: 32 additions & 94 deletions
Original file line numberDiff line numberDiff line change
@@ -77,10 +77,6 @@ class SingularString : public FieldGeneratorBase {
7777

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

80-
ArenaDtorNeeds NeedsArenaDestructor() const override {
81-
return is_inlined() ? ArenaDtorNeeds::kOnDemand : ArenaDtorNeeds::kNone;
82-
}
83-
8480
void GeneratePrivateMembers(io::Printer* p) const override {
8581
// Skips the automatic destruction if inlined; rather calls it explicitly if
8682
// allocating arena is null.
@@ -113,16 +109,6 @@ class SingularString : public FieldGeneratorBase {
113109
}
114110
}
115111

116-
void GenerateArenaDestructorCode(io::Printer* p) const override {
117-
if (!is_inlined()) return;
118-
119-
p->Emit(R"cc(
120-
if (!_this->_internal_$name$_donated()) {
121-
_this->$field_$.~InlinedStringField();
122-
}
123-
)cc");
124-
}
125-
126112
void GenerateNonInlineAccessorDefinitions(io::Printer* p) const override {
127113
if (EmptyDefault()) return;
128114
p->Emit(R"cc(
@@ -255,33 +241,25 @@ void SingularString::GenerateAccessorDeclarations(io::Printer* p) const {
255241
auto v3 = p->WithVars(
256242
AnnotatedAccessors(field_, {"mutable_"}, AnnotationCollector::kAlias));
257243

258-
p->Emit({{"donated",
259-
[&] {
260-
if (!is_inlined()) return;
261-
p->Emit(R"cc(
262-
PROTOBUF_ALWAYS_INLINE bool _internal_$name$_donated() const;
263-
)cc");
264-
}}},
265-
R"cc(
266-
$DEPRECATED$ const ::std::string& $name$() const;
267-
//~ Using `Arg_ = const std::string&` will make the type of `arg`
268-
//~ default to `const std::string&`, due to reference collapse. This
269-
//~ is necessary because there are a handful of users that rely on
270-
//~ this default.
271-
template <typename Arg_ = const ::std::string&, typename... Args_>
272-
$DEPRECATED$ void $set_name$(Arg_&& arg, Args_... args);
273-
$DEPRECATED$ ::std::string* $nonnull$ $mutable_name$();
274-
$DEPRECATED$ [[nodiscard]] ::std::string* $nullable$ $release_name$();
275-
$DEPRECATED$ void $set_allocated_name$(::std::string* $nullable$ value);
276-
277-
private:
278-
const ::std::string& _internal_$name$() const;
279-
PROTOBUF_ALWAYS_INLINE void _internal_set_$name$(const ::std::string& value);
280-
::std::string* $nonnull$ _internal_mutable_$name$();
281-
$donated$;
282-
283-
public:
284-
)cc");
244+
p->Emit(R"cc(
245+
$DEPRECATED$ const ::std::string& $name$() const;
246+
//~ Using `Arg_ = const std::string&` will make the type of `arg`
247+
//~ default to `const std::string&`, due to reference collapse. This
248+
//~ is necessary because there are a handful of users that rely on
249+
//~ this default.
250+
template <typename Arg_ = const ::std::string&, typename... Args_>
251+
$DEPRECATED$ void $set_name$(Arg_&& arg, Args_... args);
252+
$DEPRECATED$ ::std::string* $nonnull$ $mutable_name$();
253+
$DEPRECATED$ [[nodiscard]] ::std::string* $nullable$ $release_name$();
254+
$DEPRECATED$ void $set_allocated_name$(::std::string* $nullable$ value);
255+
256+
private:
257+
const ::std::string& _internal_$name$() const;
258+
PROTOBUF_ALWAYS_INLINE void _internal_set_$name$(const ::std::string& value);
259+
::std::string* $nonnull$ _internal_mutable_$name$();
260+
261+
public:
262+
)cc");
285263
}
286264

287265
void UpdateHasbitSet(io::Printer* p, bool is_oneof) {
@@ -302,16 +280,6 @@ void UpdateHasbitSet(io::Printer* p, bool is_oneof) {
302280
)cc");
303281
}
304282

305-
void ArgsForSetter(io::Printer* p, bool inlined) {
306-
if (!inlined) {
307-
p->Emit("GetArena()");
308-
return;
309-
}
310-
p->Emit(
311-
"GetArena(), _internal_$name_internal$_donated(), "
312-
"&$donating_states_word$, $mask_for_undonate$, this");
313-
}
314-
315283
void SingularString::ReleaseImpl(io::Printer* p) const {
316284
if (is_oneof()) {
317285
p->Emit(R"cc(
@@ -338,7 +306,7 @@ void SingularString::ReleaseImpl(io::Printer* p) const {
338306
}
339307
$clear_hasbit$;
340308
341-
return $field_$.Release(GetArena(), _internal_$name_internal$_donated());
309+
return $field_$.Release(GetArena());
342310
)cc");
343311
return;
344312
}
@@ -360,7 +328,7 @@ void SingularString::ReleaseImpl(io::Printer* p) const {
360328
p->Emit(R"cc(
361329
auto* released = $field_$.Release();
362330
if ($pbi$::DebugHardenForceCopyDefaultString()) {
363-
$field_$.Set("", $set_args$);
331+
$field_$.Set("", GetArena());
364332
}
365333
return released;
366334
)cc");
@@ -390,22 +358,18 @@ void SingularString::SetAllocatedImpl(io::Printer* p) const {
390358
)cc");
391359
}
392360

361+
p->Emit(R"cc(
362+
$field_$.SetAllocated(value, GetArena());
363+
)cc");
364+
393365
if (is_inlined()) {
394-
// Currently, string fields with default value can't be inlined.
395-
p->Emit(R"cc(
396-
$field_$.SetAllocated(nullptr, value, $set_args$);
397-
)cc");
398366
return;
399367
}
400368

401-
p->Emit(R"cc(
402-
$field_$.SetAllocated(value, $set_args$);
403-
)cc");
404-
405369
if (EmptyDefault()) {
406370
p->Emit(R"cc(
407371
if ($pbi$::DebugHardenForceCopyDefaultString() && $field_$.IsDefault()) {
408-
$field_$.Set("", $set_args$);
372+
$field_$.Set("", GetArena());
409373
}
410374
)cc");
411375
}
@@ -423,7 +387,6 @@ void SingularString::GenerateInlineAccessorDefinitions(io::Printer* p) const {
423387
)cc");
424388
}},
425389
{"update_hasbit", [&] { UpdateHasbitSet(p, is_oneof()); }},
426-
{"set_args", [&] { ArgsForSetter(p, is_inlined()); }},
427390
{"check_hasbit",
428391
[&] {
429392
if (!is_oneof()) return;
@@ -454,7 +417,7 @@ void SingularString::GenerateInlineAccessorDefinitions(io::Printer* p) const {
454417
$TsanDetectConcurrentMutation$;
455418
$PrepareSplitMessageForWrite$;
456419
$update_hasbit$;
457-
$field_$.$Set$(static_cast<Arg_&&>(arg), args..., $set_args$);
420+
$field_$.$Set$(static_cast<Arg_&&>(arg), args..., GetArena());
458421
$annotate_set$;
459422
// @@protoc_insertion_point(field_set:$pkg.Msg.field$)
460423
}
@@ -477,11 +440,11 @@ void SingularString::GenerateInlineAccessorDefinitions(io::Printer* p) const {
477440
$TsanDetectConcurrentMutation$;
478441
//~ Don't use $Set$ here; we always want the std::string variant
479442
//~ regardless of whether this is a `bytes` field.
480-
$field_$.Set(value, $set_args$);
443+
$field_$.Set(value, GetArena());
481444
}
482445
inline ::std::string* $nonnull$ $Msg$::_internal_mutable_$name_internal$() {
483446
$TsanDetectConcurrentMutation$;
484-
return $field_$.Mutable($lazy_args$, $set_args$);
447+
return $field_$.Mutable($lazy_args$, GetArena());
485448
}
486449
inline ::std::string* $nullable$ $Msg$::$release_name$() {
487450
$WeakDescriptorSelfPin$;
@@ -500,14 +463,6 @@ void SingularString::GenerateInlineAccessorDefinitions(io::Printer* p) const {
500463
// @@protoc_insertion_point(field_set_allocated:$pkg.Msg.field$)
501464
})cc";
502465
p->Emit(vars, code);
503-
504-
if (is_inlined()) {
505-
p->Emit(R"cc(
506-
inline bool $Msg$::_internal_$name_internal$_donated() const {
507-
return $inlined_string_donated$;
508-
}
509-
)cc");
510-
}
511466
}
512467

513468
void SingularString::GenerateClearingCode(io::Printer* p) const {
@@ -592,14 +547,8 @@ void SingularString::GenerateSwappingCode(io::Printer* p) const {
592547
}
593548

594549
p->Emit(R"cc(
595-
{
596-
bool lhs_dtor_registered = ($inlined_string_donated_array$[0] & 1) == 0;
597-
bool rhs_dtor_registered =
598-
(other->$inlined_string_donated_array$[0] & 1) == 0;
599-
::_pbi::InlinedStringField::InternalSwap(
600-
&$field_$, lhs_dtor_registered, this, &other->$field_$,
601-
rhs_dtor_registered, other, arena);
602-
}
550+
::_pbi::InlinedStringField::InternalSwap(&$field_$, &other->$field_$,
551+
arena);
603552
)cc");
604553
}
605554

@@ -634,21 +583,10 @@ void SingularString::GenerateCopyConstructorCode(io::Printer* p) const {
634583
} else {
635584
p->Emit(R"cc(!from._internal_$name$().empty())cc");
636585
}
637-
}},
638-
{"set_args",
639-
[&] {
640-
if (!is_inlined()) {
641-
p->Emit("_this->GetArena()");
642-
} else {
643-
p->Emit(
644-
"_this->GetArena(), "
645-
"_this->_internal_$name$_donated(), "
646-
"&_this->$donating_states_word$, $mask_for_undonate$, _this");
647-
}
648586
}}},
649587
R"cc(
650588
if ($hazzer$) {
651-
_this->$field_$.Set(from._internal_$name$(), $set_args$);
589+
_this->$field_$.Set(from._internal_$name$(), _this->GetArena());
652590
}
653591
)cc");
654592
}

0 commit comments

Comments
 (0)