Skip to content

Commit 276701b

Browse files
ClaytonKnittelcopybara-github
ClaytonKnittel
authored and
copybara-github
committed
Assign hasbits to repeated fields if experiment flag is enabled, and set hasbits accordingly everywhere.
This cl does not actually enable this feature, and should be a no-op. PiperOrigin-RevId: 792014145
1 parent ff147a0 commit 276701b

14 files changed

+401
-94
lines changed

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

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -413,6 +413,9 @@ void RepeatedEnum::GenerateInlineAccessorDefinitions(io::Printer* p) const {
413413
}
414414
)cc");
415415
p->Emit(R"cc(
416+
//~ Note: no need to set hasbit in set_$name$(int index). Hasbits only
417+
//~ need to be updated if a new element is (potentially) added, not if an
418+
//~ existing element is mutated.
416419
inline void $Msg$::set_$name$(int index, $Enum$ value) {
417420
$WeakDescriptorSelfPin$;
418421
$assert_valid$;
@@ -427,6 +430,7 @@ void RepeatedEnum::GenerateInlineAccessorDefinitions(io::Printer* p) const {
427430
$assert_valid$;
428431
$TsanDetectConcurrentMutation$;
429432
_internal_mutable_$name_internal$()->Add(value);
433+
$set_hasbit$;
430434
$annotate_add$
431435
// @@protoc_insertion_point(field_add:$pkg.Msg.field$)
432436
}
@@ -444,6 +448,7 @@ void RepeatedEnum::GenerateInlineAccessorDefinitions(io::Printer* p) const {
444448
inline $pb$::RepeatedField<int>* $nonnull$ $Msg$::mutable_$name$()
445449
ABSL_ATTRIBUTE_LIFETIME_BOUND {
446450
$WeakDescriptorSelfPin$;
451+
$set_hasbit$;
447452
$annotate_mutable_list$;
448453
// @@protoc_insertion_point(field_mutable_list:$pkg.Msg.field$)
449454
$TsanDetectConcurrentMutation$;

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -248,6 +248,7 @@ void Map::GenerateInlineAccessorDefinitions(io::Printer* p) const {
248248
inline $Map$* $nonnull$ $Msg$::mutable_$name$()
249249
ABSL_ATTRIBUTE_LIFETIME_BOUND {
250250
$WeakDescriptorSelfPin$;
251+
$set_hasbit$;
251252
$annotate_mutable$;
252253
// @@protoc_insertion_point(field_mutable_map:$pkg.Msg.field$)
253254
return _internal_mutable_$name_internal$();

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

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -786,8 +786,12 @@ void RepeatedMessage::GenerateAccessorDeclarations(io::Printer* p) const {
786786

787787
void RepeatedMessage::GenerateInlineAccessorDefinitions(io::Printer* p) const {
788788
// TODO: move insertion points
789+
789790
p->Emit({GetEmitRepeatedFieldMutableSub(*opts_, p)},
790791
R"cc(
792+
//~ Note: no need to set hasbit in mutable_$name$(int index).
793+
//~ Hasbits only need to be updated if a new element is
794+
//~ (potentially) added, not if an existing element is mutated.
791795
inline $Submsg$* $nonnull$ $Msg$::mutable_$name$(int index)
792796
ABSL_ATTRIBUTE_LIFETIME_BOUND {
793797
$WeakDescriptorSelfPin$;
@@ -802,6 +806,7 @@ void RepeatedMessage::GenerateInlineAccessorDefinitions(io::Printer* p) const {
802806
inline $pb$::RepeatedPtrField<$Submsg$>* $nonnull$ $Msg$::mutable_$name$()
803807
ABSL_ATTRIBUTE_LIFETIME_BOUND {
804808
$WeakDescriptorSelfPin$;
809+
$set_hasbit$;
805810
$annotate_mutable_list$;
806811
// @@protoc_insertion_point(field_mutable_list:$pkg.Msg.field$)
807812
$StrongRef$;
@@ -826,6 +831,7 @@ void RepeatedMessage::GenerateInlineAccessorDefinitions(io::Printer* p) const {
826831
$WeakDescriptorSelfPin$;
827832
$TsanDetectConcurrentMutation$;
828833
$Submsg$* _add = _internal_mutable_$name_internal$()->Add();
834+
$set_hasbit$;
829835
$annotate_add_mutable$;
830836
// @@protoc_insertion_point(field_add:$pkg.Msg.field$)
831837
return _add;

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

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -480,6 +480,9 @@ void RepeatedPrimitive::GenerateInlineAccessorDefinitions(
480480
}
481481
)cc");
482482
p->Emit(R"cc(
483+
//~ Note: no need to set hasbit in set_$name$(int index). Hasbits only need
484+
//~ to be updated if a new element is (potentially) added, not if an
485+
//~ existing element is mutated.
483486
inline void $Msg$::set_$name$(int index, $Type$ value) {
484487
$WeakDescriptorSelfPin$;
485488
$annotate_set$;
@@ -492,6 +495,7 @@ void RepeatedPrimitive::GenerateInlineAccessorDefinitions(
492495
$WeakDescriptorSelfPin$;
493496
$TsanDetectConcurrentMutation$;
494497
_internal_mutable_$name_internal$()->Add(value);
498+
$set_hasbit$;
495499
$annotate_add$;
496500
// @@protoc_insertion_point(field_add:$pkg.Msg.field$)
497501
}
@@ -509,6 +513,7 @@ void RepeatedPrimitive::GenerateInlineAccessorDefinitions(
509513
inline $pb$::RepeatedField<$Type$>* $nonnull$ $Msg$::mutable_$name$()
510514
ABSL_ATTRIBUTE_LIFETIME_BOUND {
511515
$WeakDescriptorSelfPin$;
516+
$set_hasbit$;
512517
$annotate_mutable_list$;
513518
// @@protoc_insertion_point(field_mutable_list:$pkg.Msg.field$)
514519
$TsanDetectConcurrentMutation$;

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

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -871,6 +871,7 @@ void RepeatedString::GenerateInlineAccessorDefinitions(io::Printer* p) const {
871871
$WeakDescriptorSelfPin$;
872872
$TsanDetectConcurrentMutation$;
873873
::std::string* _s = _internal_mutable_$name_internal$()->Add();
874+
$set_hasbit$;
874875
$annotate_add_mutable$;
875876
// @@protoc_insertion_point(field_add_mutable:$pkg.Msg.field$)
876877
return _s;
@@ -882,13 +883,19 @@ void RepeatedString::GenerateInlineAccessorDefinitions(io::Printer* p) const {
882883
// @@protoc_insertion_point(field_get:$pkg.Msg.field$)
883884
return $getter$;
884885
}
886+
//~ Note: no need to set hasbit in mutable_$name$(int index). Hasbits
887+
//~ only need to be updated if a new element is (potentially) added, not
888+
//~ if an existing element is mutated.
885889
inline ::std::string* $nonnull$ $Msg$::mutable_$name$(int index)
886890
ABSL_ATTRIBUTE_LIFETIME_BOUND {
887891
$WeakDescriptorSelfPin$;
888892
$annotate_mutable$;
889893
// @@protoc_insertion_point(field_mutable:$pkg.Msg.field$)
890894
return $mutable$;
891895
}
896+
//~ Note: no need to set hasbit in set_$name$(int index). Hasbits
897+
//~ only need to be updated if a new element is (potentially) added, not
898+
//~ if an existing element is mutated.
892899
template <typename Arg_, typename... Args_>
893900
inline void $Msg$::set_$name$(int index, Arg_&& value, Args_... args) {
894901
$WeakDescriptorSelfPin$;
@@ -904,6 +911,7 @@ void RepeatedString::GenerateInlineAccessorDefinitions(io::Printer* p) const {
904911
$pbi$::AddToRepeatedPtrField(*_internal_mutable_$name_internal$(),
905912
::std::forward<Arg_>(value),
906913
args... $bytes_tag$);
914+
$set_hasbit$;
907915
$annotate_add$;
908916
// @@protoc_insertion_point(field_add:$pkg.Msg.field$)
909917
}
@@ -917,6 +925,7 @@ void RepeatedString::GenerateInlineAccessorDefinitions(io::Printer* p) const {
917925
inline $pb$::RepeatedPtrField<::std::string>* $nonnull$
918926
$Msg$::mutable_$name$() ABSL_ATTRIBUTE_LIFETIME_BOUND {
919927
$WeakDescriptorSelfPin$;
928+
$set_hasbit$;
920929
$annotate_mutable_list$;
921930
// @@protoc_insertion_point(field_mutable_list:$pkg.Msg.field$)
922931
$TsanDetectConcurrentMutation$;

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

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -743,6 +743,9 @@ void RepeatedStringView::GenerateInlineAccessorDefinitions(
743743
// @@protoc_insertion_point(field_get:$pkg.Msg.field$)
744744
return $getter$;
745745
}
746+
//~ Note: no need to set hasbit in set_$name$(int index). Hasbits only
747+
//~ need to be updated if a new element is (potentially) added, not if
748+
//~ an existing element is mutated.
746749
template <typename Arg_>
747750
inline void $Msg$::set_$name$(int index, Arg_&& value) {
748751
$WeakDescriptorSelfPin$;
@@ -756,6 +759,7 @@ void RepeatedStringView::GenerateInlineAccessorDefinitions(
756759
$TsanDetectConcurrentMutation$;
757760
$pbi$::AddToRepeatedPtrField(*_internal_mutable_$name_internal$(),
758761
::std::forward<Arg_>(value) $bytes_tag$);
762+
$set_hasbit$;
759763
$annotate_add$;
760764
// @@protoc_insertion_point(field_add:$pkg.Msg.field$)
761765
}
@@ -769,6 +773,7 @@ void RepeatedStringView::GenerateInlineAccessorDefinitions(
769773
inline $pb$::RepeatedPtrField<::std::string>* $nonnull$
770774
$Msg$::mutable_$name$() ABSL_ATTRIBUTE_LIFETIME_BOUND {
771775
$WeakDescriptorSelfPin$;
776+
$set_hasbit$;
772777
$annotate_mutable_list$;
773778
// @@protoc_insertion_point(field_mutable_list:$pkg.Msg.field$)
774779
$TsanDetectConcurrentMutation$;

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

Lines changed: 88 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -116,20 +116,20 @@ TEST(GeneratedMessageTest, RecursiveMessageSize) {
116116
ABSL_CHECK_MESSAGE_SIZE(MockGenerated, 40);
117117

118118
struct MockGeneratedLazy : public MockMessageBase { // 16 bytes
119-
int has_bits[1]; // 4 bytes
120-
int cached_size; // 4 bytes
121-
void* a[2]; // 16 bytes (lazy)
122-
int32_t i; // 4 bytes
123-
PROTOBUF_TSAN_DECLARE_MEMBER; // 0-4 bytes
119+
int has_bits[1]; // 4 bytes
120+
int cached_size; // 4 bytes
121+
void* a[2]; // 16 bytes (lazy)
122+
int32_t i; // 4 bytes
123+
PROTOBUF_TSAN_DECLARE_MEMBER; // 0-4 bytes
124124
// + 0-4 bytes padding
125125
};
126126
ABSL_CHECK_MESSAGE_SIZE(MockGeneratedLazy, 48);
127127

128128
struct MockGeneratedSplit : public MockMessageBase { // 16 bytes
129129
int has_bits[1]; // 4 bytes
130130
int cached_size; // 4 bytes
131-
void* split; // 8 bytes
132-
PROTOBUF_TSAN_DECLARE_MEMBER; // 0-4 bytes
131+
void* split; // 8 bytes
132+
PROTOBUF_TSAN_DECLARE_MEMBER; // 0-4 bytes
133133
// + 0-4 bytes padding
134134
};
135135
ABSL_CHECK_MESSAGE_SIZE(MockGeneratedSplit, 32);
@@ -172,13 +172,26 @@ TEST(GeneratedMessageTest, OneStringSize) {
172172
TEST(GeneratedMessageTest, MoreStringSize) {
173173
// TODO: remove once synthetic_pdproto lands.
174174
#ifndef PROTOBUF_FORCE_SPLIT
175-
struct MockGenerated : public MockMessageBase { // 16 bytes
176-
int cached_size; // 4 bytes
177-
PROTOBUF_TSAN_DECLARE_MEMBER; // 0-4 bytes
178-
// + 0-4 bytes padding
179-
MockRepeatedPtrField data; // 24 bytes
180-
};
181-
ABSL_CHECK_MESSAGE_SIZE(MockGenerated, 48);
175+
if constexpr (internal::EnableExperimentalHintHasBitsForRepeatedFields()) {
176+
struct MockGenerated : public MockMessageBase { // 16 bytes
177+
int has_bits[1]; // 4 bytes
178+
int cached_size; // 4 bytes
179+
PROTOBUF_TSAN_DECLARE_MEMBER; // 0-4 bytes
180+
// + 0-4 bytes padding
181+
MockRepeatedPtrField data; // 24 bytes
182+
};
183+
ABSL_CHECK_MESSAGE_SIZE(MockGenerated, 48);
184+
EXPECT_EQ(sizeof(proto2_unittest::MoreString), sizeof(MockGenerated));
185+
} else {
186+
struct MockGenerated : public MockMessageBase { // 16 bytes
187+
int cached_size; // 4 bytes
188+
PROTOBUF_TSAN_DECLARE_MEMBER; // 0-4 bytes
189+
// + 0-4 bytes padding
190+
MockRepeatedPtrField data; // 24 bytes
191+
};
192+
ABSL_CHECK_MESSAGE_SIZE(MockGenerated, 48);
193+
EXPECT_EQ(sizeof(proto2_unittest::MoreString), sizeof(MockGenerated));
194+
}
182195
#else // !PROTOBUF_FORCE_SPLIT
183196
struct MockGenerated : public MockMessageBase { // 16 bytes
184197
int cached_size; // 4 bytes
@@ -187,8 +200,8 @@ TEST(GeneratedMessageTest, MoreStringSize) {
187200
// + 0-4 bytes padding
188201
};
189202
ABSL_CHECK_MESSAGE_SIZE(MockGenerated, 32);
190-
#endif // PROTOBUF_FORCE_SPLIT
191203
EXPECT_EQ(sizeof(proto2_unittest::MoreString), sizeof(MockGenerated));
204+
#endif // PROTOBUF_FORCE_SPLIT
192205
}
193206

194207
TEST(GeneratedMessageTest, Int32MessageSize) {
@@ -288,26 +301,26 @@ TEST(GeneratedMessageTest, FieldOrderingsSize) {
288301
ABSL_CHECK_MESSAGE_SIZE(MockGenerated, 80);
289302

290303
struct MockGeneratedExperiments : public MockMessageBase { // 16 bytes
291-
int has_bits[1]; // 4 bytes
292-
int donated[1]; // 4 bytes
293-
int cached_size; // 4 bytes
304+
int has_bits[1]; // 4 bytes
305+
int donated[1]; // 4 bytes
306+
int cached_size; // 4 bytes
294307
// + 0-4 bytes padding
295-
MockExtensionSet extensions; // 24 bytes
296-
std::string my_string; // sizeof(std::string)
308+
MockExtensionSet extensions; // 24 bytes
309+
std::string my_string; // sizeof(std::string)
297310
void* optional_nested_message[2]; // 16 bytes (lazy)
298311
int64_t my_int; // 8 bytes
299312
float my_float; // 4 bytes
300-
PROTOBUF_TSAN_DECLARE_MEMBER; // 0-4 bytes
313+
PROTOBUF_TSAN_DECLARE_MEMBER; // 0-4 bytes
301314
// + 0-4 bytes padding
302315
};
303316
ABSL_CHECK_MESSAGE_SIZE(MockGeneratedExperiments, 112);
304317

305318
struct MockGeneratedSplit : public MockMessageBase { // 16 bytes
306319
int has_bits[1]; // 4 bytes
307320
int cached_size; // 4 bytes
308-
MockExtensionSet extensions; // 24 bytes
309-
void* split; // 8 bytes
310-
PROTOBUF_TSAN_DECLARE_MEMBER; // 0-4 bytes
321+
MockExtensionSet extensions; // 24 bytes
322+
void* split; // 8 bytes
323+
PROTOBUF_TSAN_DECLARE_MEMBER; // 0-4 bytes
311324
// + 0-4 bytes padding
312325
};
313326
ABSL_CHECK_MESSAGE_SIZE(MockGeneratedSplit, 56);
@@ -359,11 +372,11 @@ TEST(GeneratedMessageTest, TestMessageSize) {
359372
ABSL_CHECK_MESSAGE_SIZE(MockGenerated, 56);
360373

361374
struct MockGeneratedSplit : public MockMessageBase { // 16 bytes
362-
int has_bits[1]; // 4 bytes
363-
int cached_size; // 4 bytes
364-
void* split; // 8 bytes
365-
PROTOBUF_TSAN_DECLARE_MEMBER; // 0-4 bytes
366-
// + 0-4 bytes padding
375+
int has_bits[1]; // 4 bytes
376+
int cached_size; // 4 bytes
377+
void* split; // 8 bytes
378+
PROTOBUF_TSAN_DECLARE_MEMBER; // 0-4 bytes
379+
// + 0-4 bytes padding
367380
};
368381
ABSL_CHECK_MESSAGE_SIZE(MockGeneratedSplit, 32);
369382
#ifndef PROTOBUF_FORCE_SPLIT
@@ -378,6 +391,33 @@ TEST(GeneratedMessageTest, TestMessageSize) {
378391
TEST(GeneratedMessageTest, PackedTypesSize) {
379392
// TODO: remove once synthetic_pdproto lands.
380393
#ifndef PROTOBUF_FORCE_SPLIT
394+
struct MockGeneratedWithHasBits : public MockMessageBase { // 16 bytes
395+
int has_bits[1]; // 4 bytes
396+
int cached_size; // 4 bytes
397+
MockRepeatedField packed_int32; // 16 bytes
398+
int packed_int32_cached_byte_size; // 4 bytes + 4 bytes padding
399+
MockRepeatedField packed_int64; // 16 bytes
400+
int packed_int64_cached_byte_size; // 4 bytes + 4 bytes padding
401+
MockRepeatedField packed_uint32; // 16 bytes
402+
int packed_uint32_cached_byte_size; // 4 bytes + 4 bytes padding
403+
MockRepeatedField packed_uint64; // 16 bytes
404+
int packed_uint64_cached_byte_size; // 4 bytes + 4 bytes padding
405+
MockRepeatedField packed_sint32; // 16 bytes
406+
int packed_sint32_cached_byte_size; // 4 bytes + 4 bytes padding
407+
MockRepeatedField packed_sint64; // 16 bytes
408+
int packed_sint64_cached_byte_size; // 4 bytes + 4 bytes padding
409+
MockRepeatedField packed_fixed32; // 16 bytes
410+
MockRepeatedField packed_fixed64; // 16 bytes
411+
MockRepeatedField packed_sfixed32; // 16 bytes
412+
MockRepeatedField packed_sfixed64; // 16 bytes
413+
MockRepeatedField packed_float; // 16 bytes
414+
MockRepeatedField packed_double; // 16 bytes
415+
MockRepeatedField packed_bool; // 16 bytes
416+
MockRepeatedField packed_enum; // 16 bytes
417+
int packed_enum_cached_byte_size; // 4 bytes
418+
PROTOBUF_TSAN_DECLARE_MEMBER; // 0-4 bytes
419+
// + 4-0 bytes padding
420+
};
381421
struct MockGenerated : public MockMessageBase { // 16 bytes
382422
MockRepeatedField packed_int32; // 16 bytes
383423
int packed_int32_cached_byte_size; // 4 bytes + 4 bytes padding
@@ -402,10 +442,21 @@ TEST(GeneratedMessageTest, PackedTypesSize) {
402442
int packed_enum_cached_byte_size; // 4 bytes
403443
int cached_size; // 4 bytes
404444
PROTOBUF_TSAN_DECLARE_MEMBER; // 0-4 bytes
405-
// + 0-4 bytes padding
445+
// + 4-0 bytes padding
406446
};
407-
ABSL_CHECK_MESSAGE_SIZE(MockGenerated, 16 * 15 + 8 * 6 + 8);
447+
if constexpr (internal::EnableExperimentalHintHasBitsForRepeatedFields()) {
448+
ABSL_CHECK_MESSAGE_SIZE(MockGeneratedWithHasBits, 8 + 16 * 15 + 8 * 6 + 8);
449+
} else {
450+
ABSL_CHECK_MESSAGE_SIZE(MockGenerated, 16 * 15 + 8 * 6 + 8);
451+
}
408452
#else // !PROTOBUF_FORCE_SPLIT
453+
struct MockGeneratedWithHasBits : public MockMessageBase { // 16 bytes
454+
int has_bits[1]; // 4 bytes
455+
int cached_size; // 4 bytes + 4 bytes padding
456+
void* split; // 8 bytes
457+
PROTOBUF_TSAN_DECLARE_MEMBER; // 0-4 bytes
458+
// + 0-4 bytes padding
459+
};
409460
struct MockGenerated : public MockMessageBase { // 16 bytes
410461
int cached_size; // 4 bytes + 4 bytes padding
411462
void* split; // 8 bytes
@@ -414,7 +465,12 @@ TEST(GeneratedMessageTest, PackedTypesSize) {
414465
};
415466
ABSL_CHECK_MESSAGE_SIZE(MockGenerated, 32);
416467
#endif // PROTOBUF_FORCE_SPLIT
417-
EXPECT_EQ(sizeof(proto2_unittest::TestPackedTypes), sizeof(MockGenerated));
468+
if constexpr (internal::EnableExperimentalHintHasBitsForRepeatedFields()) {
469+
EXPECT_EQ(sizeof(proto2_unittest::TestPackedTypes),
470+
sizeof(MockGeneratedWithHasBits));
471+
} else {
472+
EXPECT_EQ(sizeof(proto2_unittest::TestPackedTypes), sizeof(MockGenerated));
473+
}
418474
}
419475

420476
} // namespace cpp_unittest

‎src/google/protobuf/descriptor.cc

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -10689,12 +10689,14 @@ HasbitMode GetFieldHasbitModeWithoutProfile(const FieldDescriptor* field) {
1068910689
return HasbitMode::kTrueHasbit;
1069010690
}
1069110691

10692-
// Implicit presence fields.
10693-
if (!field->is_repeated()) {
10692+
if constexpr (EnableExperimentalHintHasBitsForRepeatedFields()) {
10693+
// With hasbits for repeated fields enabled, both implicit-presence and
10694+
// repeated/map fields have hint hasbits.
1069410695
return HasbitMode::kHintHasbit;
1069510696
}
10696-
// We currently don't implement hasbits for implicit repeated fields.
10697-
return HasbitMode::kNoHasbit;
10697+
10698+
// Implicit-presence fields have hint hasbits, repeated/map fields do not.
10699+
return field->is_repeated() ? HasbitMode::kNoHasbit : HasbitMode::kHintHasbit;
1069810700
}
1069910701

1070010702
bool HasHasbitWithoutProfile(const FieldDescriptor* field) {

0 commit comments

Comments
 (0)