Skip to content

Commit

Permalink
Improvements to MergeFrom for weak fields:
Browse files Browse the repository at this point in the history
 - Remove _Internal accessors. They only have one caller and it's better to inline it.
 - Remove redundant has bit setting. MergeFrom does it globally instead of per field.
 - Reuse existing arena object. Avoids redundant calls to `GetArena()`.
 - Use rhs object instead of weak instance for calling `New()`.
 - Change repeated fields to use the generic MergeFrom instead of Arena::CopyConstruct.

PiperOrigin-RevId: 586680126
  • Loading branch information
protobuf-github-bot authored and copybara-github committed Nov 30, 2023
1 parent 07194fc commit bb8958d
Show file tree
Hide file tree
Showing 4 changed files with 13 additions and 69 deletions.
16 changes: 0 additions & 16 deletions src/google/protobuf/compiler/cpp/field.h
Original file line number Diff line number Diff line change
Expand Up @@ -121,10 +121,6 @@ class FieldGeneratorBase {

virtual void GenerateNonInlineAccessorDefinitions(io::Printer* p) const {}

virtual void GenerateInternalAccessorDefinitions(io::Printer* p) const {}

virtual void GenerateInternalAccessorDeclarations(io::Printer* p) const {}

virtual void GenerateClearingCode(io::Printer* p) const = 0;

virtual void GenerateMessageClearingCode(io::Printer* p) const {
Expand Down Expand Up @@ -334,18 +330,6 @@ class FieldGenerator {
impl_->GenerateNonInlineAccessorDefinitions(p);
}

// Generates declarations of accessors that are for internal purposes only.
void GenerateInternalAccessorDefinitions(io::Printer* p) const {
auto vars = PushVarsForCall(p);
impl_->GenerateInternalAccessorDefinitions(p);
}

// Generates definitions of accessors that are for internal purposes only.
void GenerateInternalAccessorDeclarations(io::Printer* p) const {
auto vars = PushVarsForCall(p);
impl_->GenerateInternalAccessorDeclarations(p);
}

// Generates statements which clear the field.
//
// This is used to define the clear_$name$() method.
Expand Down
53 changes: 7 additions & 46 deletions src/google/protobuf/compiler/cpp/field_generators/message_field.cc
Original file line number Diff line number Diff line change
Expand Up @@ -108,8 +108,6 @@ class SingularMessage : public FieldGeneratorBase {

void GenerateAccessorDeclarations(io::Printer* p) const override;
void GenerateInlineAccessorDefinitions(io::Printer* p) const override;
void GenerateInternalAccessorDeclarations(io::Printer* p) const override;
void GenerateInternalAccessorDefinitions(io::Printer* p) const override;
void GenerateClearingCode(io::Printer* p) const override;
void GenerateMessageClearingCode(io::Printer* p) const override;
void GenerateMergingCode(io::Printer* p) const override;
Expand Down Expand Up @@ -307,47 +305,6 @@ void SingularMessage::GenerateInlineAccessorDefinitions(io::Printer* p) const {
)cc");
}

void SingularMessage::GenerateInternalAccessorDeclarations(
io::Printer* p) const {
if (!is_weak()) return;

p->Emit(R"cc(
static $pb$::MessageLite* mutable_$name$($Msg$* msg);
)cc");
}

void SingularMessage::GenerateInternalAccessorDefinitions(
io::Printer* p) const {
// In theory, these accessors could be inline in _Internal. However, in
// practice, the linker is then not able to throw them out making implicit
// weak dependencies not work at all.

if (!is_weak() || is_oneof()) return;

// These private accessors are used by MergeFrom and
// MergePartialFromCodedStream, and their purpose is to provide access to
// the field without creating a strong dependency on the message type.
p->Emit(
{
{"update_hasbit",
[&] {
if (!has_hasbit_) return;
p->Emit(R"cc(
msg->$set_hasbit$;
)cc");
}},
},
R"cc(
$pb$::MessageLite* $Msg$::_Internal::mutable_$name$($Msg$* msg) {
$update_hasbit$;
if (msg->$field_$ == nullptr) {
msg->$field_$ = $kDefaultPtr$->New(msg->GetArena());
}
return msg->$field_$;
}
)cc");
}

void SingularMessage::GenerateClearingCode(io::Printer* p) const {
if (!has_hasbit_) {
// If we don't have has-bits, message presence is indicated only by ptr !=
Expand Down Expand Up @@ -381,16 +338,20 @@ void SingularMessage::GenerateMessageClearingCode(io::Printer* p) const {
bool SingularMessage::RequiresArena(GeneratorFunction function) const {
switch (function) {
case GeneratorFunction::kMergeFrom:
return !(is_weak() || should_split());
return !should_split();
}
return false;
}

void SingularMessage::GenerateMergingCode(io::Printer* p) const {
if (is_weak()) {
p->Emit(
"_Internal::mutable_$name$(_this)->CheckTypeAndMergeFrom(\n"
" *from.$field_$);\n");
R"cc(
if (_this->$field_$ == nullptr) {
_this->$field_$ = from.$field_$->New(arena);
}
_this->$field_$->CheckTypeAndMergeFrom(*from.$field_$);
)cc");
} else if (should_split()) {
p->Emit(
"_this->_internal_mutable_$name$()->$Submsg$::MergeFrom(\n"
Expand Down
7 changes: 0 additions & 7 deletions src/google/protobuf/compiler/cpp/message.cc
Original file line number Diff line number Diff line change
Expand Up @@ -2121,10 +2121,6 @@ void MessageGenerator::GenerateClassMethods(io::Printer* p) {
"static constexpr ::int32_t kOneofCaseOffset =\n"
" PROTOBUF_FIELD_OFFSET($classtype$, $oneof_case$);\n");
}
for (auto field : FieldRange(descriptor_)) {
auto t = p->WithVars(MakeTrackerCalls(field, options_));
field_generators_.get(field).GenerateInternalAccessorDeclarations(p);
}
if (num_required_fields_ > 0) {
const std::vector<uint32_t> masks_for_has_bits = RequiredFieldsBitMask();
format(
Expand All @@ -2137,9 +2133,6 @@ void MessageGenerator::GenerateClassMethods(io::Printer* p) {

format.Outdent();
format("};\n\n");
for (auto field : FieldRange(descriptor_)) {
field_generators_.get(field).GenerateInternalAccessorDefinitions(p);
}

// Generate non-inline field definitions.
for (auto field : FieldRange(descriptor_)) {
Expand Down
6 changes: 6 additions & 0 deletions src/google/protobuf/repeated_ptr_field.h
Original file line number Diff line number Diff line change
Expand Up @@ -259,6 +259,12 @@ class PROTOBUF_EXPORT RepeatedPtrFieldBase {
template <typename T>
void MergeFrom(const RepeatedPtrFieldBase& from) {
static_assert(std::is_base_of<MessageLite, T>::value, "");
#ifdef __cpp_if_constexpr
if constexpr (!std::is_base_of<Message, T>::value) {
// For LITE objects we use the generic MergeFrom to save on binary size.
return MergeFrom<MessageLite>(from);
}
#endif
MergeFromConcreteMessage(from, Arena::CopyConstruct<T>);
}

Expand Down

0 comments on commit bb8958d

Please sign in to comment.