Skip to content

Commit

Permalink
Use Arena enabled copy constructor when merging child messages
Browse files Browse the repository at this point in the history
PiperOrigin-RevId: 575841937
  • Loading branch information
martijnvels authored and copybara-github committed Oct 23, 2023
1 parent 8bf4fe9 commit 538a8e9
Show file tree
Hide file tree
Showing 7 changed files with 212 additions and 45 deletions.
4 changes: 4 additions & 0 deletions src/google/protobuf/compiler/cpp/field.cc
Original file line number Diff line number Diff line change
Expand Up @@ -263,6 +263,7 @@ void HasBitVars(const FieldDescriptor* field, const Options& opts,
absl::optional<uint32_t> idx, std::vector<Sub>& vars) {
if (!idx.has_value()) {
vars.emplace_back("set_hasbit", "");
vars.emplace_back("this_set_hasbit", "");
vars.emplace_back("clear_hasbit", "");
return;
}
Expand All @@ -283,6 +284,9 @@ void HasBitVars(const FieldDescriptor* field, const Options& opts,
vars.emplace_back("has_hasbit", has);
vars.emplace_back(Sub("set_hasbit", set).WithSuffix(";"));
vars.emplace_back(Sub("clear_hasbit", clr).WithSuffix(";"));

set = absl::StrFormat("_this->%s[%d] |= %s;", has_bits, index, mask);
vars.emplace_back(Sub("this_set_hasbit", set).WithSuffix(";"));
}

void InlinedStringVars(const FieldDescriptor* field, const Options& opts,
Expand Down
16 changes: 16 additions & 0 deletions src/google/protobuf/compiler/cpp/field.h
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,11 @@ namespace cpp {
// matter of clean composability.
class FieldGeneratorBase {
public:
// `GeneratorFunction` defines a subset of generator functions that may have
// additional optimizations or requirements such as 'uses a local `arena`
// variable instead of calling GetArena()'
enum class GeneratorFunction { kMergeFrom };

FieldGeneratorBase(const FieldDescriptor* descriptor, const Options& options,
MessageSCCAnalyzer* scc_analyzer);

Expand Down Expand Up @@ -100,6 +105,10 @@ class FieldGeneratorBase {
return has_default_constexpr_constructor_;
}

// Returns true if this generator requires an 'arena' parameter on the
// given generator function.
virtual bool RequiresArena(GeneratorFunction) const { return false; }

virtual std::vector<io::Printer::Sub> MakeVars() const { return {}; }

virtual void GeneratePrivateMembers(io::Printer* p) const = 0;
Expand Down Expand Up @@ -230,6 +239,8 @@ class FieldGenerator {
}

public:
using GeneratorFunction = FieldGeneratorBase::GeneratorFunction;

FieldGenerator(const FieldGenerator&) = delete;
FieldGenerator& operator=(const FieldGenerator&) = delete;
FieldGenerator(FieldGenerator&&) = default;
Expand All @@ -256,6 +267,11 @@ class FieldGenerator {
return impl_->has_default_constexpr_constructor();
}

// Requirements: see FieldGeneratorBase for documentation
bool RequiresArena(GeneratorFunction function) const {
return impl_->RequiresArena(function);
}

// Prints private members needed to represent this field.
//
// These are placed inside the class definition.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,8 @@ class SingularMessage : public FieldGeneratorBase {
)cc");
}

bool RequiresArena(GeneratorFunction function) const override;

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

void GenerateAccessorDeclarations(io::Printer* p) const override;
Expand Down Expand Up @@ -415,15 +417,32 @@ void SingularMessage::GenerateMessageClearingCode(io::Printer* p) const {
}
}

bool SingularMessage::RequiresArena(GeneratorFunction function) const {
switch (function) {
case GeneratorFunction::kMergeFrom:
return !(is_weak() || is_oneof() || should_split());
}
return false;
}

void SingularMessage::GenerateMergingCode(io::Printer* p) const {
if (is_weak()) {
p->Emit(
"_Internal::mutable_$name$(_this)->CheckTypeAndMergeFrom(\n"
" _Internal::$name$(&from));\n");
} else {
} else if (is_oneof() || should_split()) {
p->Emit(
"_this->_internal_mutable_$name$()->$Submsg$::MergeFrom(\n"
" from._internal_$name$());\n");
} else {
p->Emit(R"cc(
$this_set_hasbit$;
if (_this->$field_$ == nullptr) {
_this->$field_$ = CreateMaybeMessage<$Submsg$>(arena, *from.$field_$);
} else {
_this->$field_$->MergeFrom(*from.$field_$);
}
)cc");
}
}

Expand Down
14 changes: 14 additions & 0 deletions src/google/protobuf/compiler/cpp/message.cc
Original file line number Diff line number Diff line change
Expand Up @@ -3496,6 +3496,15 @@ void MessageGenerator::GenerateMergeFrom(io::Printer* p) {
}
}

bool MessageGenerator::RequiresArena(GeneratorFunction function) const {
for (const FieldDescriptor* field : FieldRange(descriptor_)) {
if (field_generators_.get(field).RequiresArena(function)) {
return true;
}
}
return false;
}

void MessageGenerator::GenerateClassSpecificMergeImpl(io::Printer* p) {
if (HasSimpleBaseClass(descriptor_, options_)) return;
// Generate the class-specific MergeFrom, which avoids the ABSL_CHECK and
Expand All @@ -3515,6 +3524,11 @@ void MessageGenerator::GenerateClassSpecificMergeImpl(io::Printer* p) {
" auto& from = static_cast<const $classname$&>(from_msg);\n");
}
format.Indent();
if (RequiresArena(GeneratorFunction::kMergeFrom)) {
p->Emit(R"cc(
::$proto_ns$::Arena* arena = _this->GetArena();
)cc");
}
format(
"$annotate_mergefrom$"
"// @@protoc_insertion_point(class_specific_merge_from_start:"
Expand Down
5 changes: 5 additions & 0 deletions src/google/protobuf/compiler/cpp/message.h
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,7 @@ class MessageGenerator {
const Descriptor* descriptor() const { return descriptor_; }

private:
using GeneratorFunction = FieldGeneratorBase::GeneratorFunction;
enum class InitType { kConstexpr, kArena, kArenaCopy };

// Generate declarations and definitions of accessors for fields.
Expand Down Expand Up @@ -148,6 +149,10 @@ class MessageGenerator {
void GenerateFieldClear(const FieldDescriptor* field, bool is_inline,
io::Printer* p);

// Returns true if any of the fields needs an `arena` variable containing
// the current message's arena, reducing `GetArena()` call churn.
bool RequiresArena(GeneratorFunction function) const;

// Returns whether impl_ has a copy ctor.
bool ImplHasCopyCtor() const;

Expand Down
18 changes: 14 additions & 4 deletions src/google/protobuf/compiler/plugin.pb.cc

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading

0 comments on commit 538a8e9

Please sign in to comment.