Skip to content

Commit

Permalink
Use bit-field int values in buildPartial to skip work on unset groups…
Browse files Browse the repository at this point in the history
… of fields.

Changes to make this improvement:
1) All non-repeated builder fields (including maps) now have a presence bit regardless of syntax.
2) The buildPartial method is split into one method per 32-field block (aligned with bit-mask ints)
3) If a presence bit-mask int is set to 0, no fields are present, so we can skip the logic for all of those fields in the buildPartial step.

For messages with a lot of fields (> 100) that are sparsely populated this can result in a significant improvement. Not only does it potentially skip a lot of field copying logic, but also breaks the buildPartial method into chunks that should be more easily digested by the JIT compiler as discussed in this issue: #10247.

PiperOrigin-RevId: 485057663
  • Loading branch information
protobuf-github-bot authored and copybara-github committed Nov 2, 2022
1 parent 54cd586 commit c4e5996
Show file tree
Hide file tree
Showing 13 changed files with 594 additions and 495 deletions.
73 changes: 37 additions & 36 deletions src/google/protobuf/compiler/java/enum_field.cc
Original file line number Diff line number Diff line change
Expand Up @@ -91,22 +91,15 @@ void SetEnumVariables(
if (HasHasbit(descriptor)) {
// For singular messages and builders, one bit is used for the hasField bit.
(*variables)["get_has_field_bit_message"] = GenerateGetBit(messageBitIndex);
(*variables)["get_has_field_bit_builder"] = GenerateGetBit(builderBitIndex);

// Note that these have a trailing ";".
(*variables)["set_has_field_bit_message"] =
GenerateSetBit(messageBitIndex) + ";";
(*variables)["set_has_field_bit_builder"] =
GenerateSetBit(builderBitIndex) + ";";
(*variables)["clear_has_field_bit_builder"] =
GenerateClearBit(builderBitIndex) + ";";

(*variables)["set_has_field_bit_to_local"] =
GenerateSetBitToLocal(messageBitIndex);
(*variables)["is_field_present_message"] = GenerateGetBit(messageBitIndex);
} else {
(*variables)["set_has_field_bit_message"] = "";
(*variables)["set_has_field_bit_builder"] = "";
(*variables)["clear_has_field_bit_builder"] = "";

(*variables)["set_has_field_bit_to_local"] = "";
variables->insert({"is_field_present_message",
absl::StrCat((*variables)["name"], "_ != ",
(*variables)["default"], ".getNumber()")});
Expand All @@ -117,10 +110,15 @@ void SetEnumVariables(
(*variables)["set_mutable_bit_builder"] = GenerateSetBit(builderBitIndex);
(*variables)["clear_mutable_bit_builder"] = GenerateClearBit(builderBitIndex);

(*variables)["get_has_field_bit_builder"] = GenerateGetBit(builderBitIndex);

// Note that these have a trailing ";".
(*variables)["set_has_field_bit_builder"] =
GenerateSetBit(builderBitIndex) + ";";
(*variables)["clear_has_field_bit_builder"] =
GenerateClearBit(builderBitIndex) + ";";
(*variables)["get_has_field_bit_from_local"] =
GenerateGetBitFromLocal(builderBitIndex);
(*variables)["set_has_field_bit_to_local"] =
GenerateSetBitToLocal(messageBitIndex);

if (SupportUnknownEnumValue(descriptor->file())) {
variables->insert(
Expand All @@ -137,21 +135,30 @@ void SetEnumVariables(
ImmutableEnumFieldGenerator::ImmutableEnumFieldGenerator(
const FieldDescriptor* descriptor, int messageBitIndex, int builderBitIndex,
Context* context)
: descriptor_(descriptor), name_resolver_(context->GetNameResolver()) {
: descriptor_(descriptor),
message_bit_index_(messageBitIndex),
builder_bit_index_(builderBitIndex),
name_resolver_(context->GetNameResolver()) {
SetEnumVariables(descriptor, messageBitIndex, builderBitIndex,
context->GetFieldGeneratorInfo(descriptor), name_resolver_,
&variables_, context);
}

ImmutableEnumFieldGenerator::~ImmutableEnumFieldGenerator() {}

int ImmutableEnumFieldGenerator::GetMessageBitIndex() const {
return message_bit_index_;
}

int ImmutableEnumFieldGenerator::GetBuilderBitIndex() const {
return builder_bit_index_;
}

int ImmutableEnumFieldGenerator::GetNumBitsForMessage() const {
return HasHasbit(descriptor_) ? 1 : 0;
}

int ImmutableEnumFieldGenerator::GetNumBitsForBuilder() const {
return GetNumBitsForMessage();
}
int ImmutableEnumFieldGenerator::GetNumBitsForBuilder() const { return 1; }

void ImmutableEnumFieldGenerator::GenerateInterfaceMembers(
io::Printer* printer) const {
Expand All @@ -170,7 +177,7 @@ void ImmutableEnumFieldGenerator::GenerateInterfaceMembers(
}

void ImmutableEnumFieldGenerator::GenerateMembers(io::Printer* printer) const {
printer->Print(variables_, "private int $name$_;\n");
printer->Print(variables_, "private int $name$_ = $default_number$;\n");
PrintExtraFieldInfo(variables_, printer);
if (HasHazzer(descriptor_)) {
WriteFieldAccessorDocComment(printer, descriptor_, HAZZER);
Expand Down Expand Up @@ -225,8 +232,8 @@ void ImmutableEnumFieldGenerator::GenerateBuilderMembers(
printer->Print(variables_,
"$deprecation$public Builder "
"${$set$capitalized_name$Value$}$(int value) {\n"
" $set_has_field_bit_builder$\n"
" $name$_ = value;\n"
" $set_has_field_bit_builder$\n"
" onChanged();\n"
" return this;\n"
"}\n");
Expand Down Expand Up @@ -321,9 +328,7 @@ void ImmutableEnumFieldGenerator::GenerateInitializationCode(

void ImmutableEnumFieldGenerator::GenerateBuilderClearCode(
io::Printer* printer) const {
printer->Print(variables_,
"$name$_ = $default_number$;\n"
"$clear_has_field_bit_builder$\n");
printer->Print(variables_, "$name$_ = $default_number$;\n");
}

void ImmutableEnumFieldGenerator::GenerateMergingCode(
Expand All @@ -346,13 +351,13 @@ void ImmutableEnumFieldGenerator::GenerateMergingCode(

void ImmutableEnumFieldGenerator::GenerateBuildingCode(
io::Printer* printer) const {
if (HasHazzer(descriptor_)) {
printer->Print(variables_,
"if ($get_has_field_bit_from_local$) {\n"
" $set_has_field_bit_to_local$;\n"
"}\n");
printer->Print(variables_,
"if ($get_has_field_bit_from_local$) {\n"
" result.$name$_ = $name$_;\n");
if (GetNumBitsForMessage() > 0) {
printer->Print(variables_, " $set_has_field_bit_to_local$;\n");
}
printer->Print(variables_, "result.$name$_ = $name$_;\n");
printer->Print("}\n");
}

void ImmutableEnumFieldGenerator::GenerateBuilderParsingCode(
Expand Down Expand Up @@ -504,6 +509,7 @@ void ImmutableEnumOneofFieldGenerator::GenerateBuilderMembers(
" return $default$;\n"
"}\n");
printer->Annotate("{", "}", descriptor_);

WriteFieldAccessorDocComment(printer, descriptor_, SETTER,
/* builder */ true);
printer->Print(variables_,
Expand All @@ -518,6 +524,7 @@ void ImmutableEnumOneofFieldGenerator::GenerateBuilderMembers(
" return this;\n"
"}\n");
printer->Annotate("{", "}", descriptor_);

WriteFieldAccessorDocComment(printer, descriptor_, CLEARER,
/* builder */ true);
printer->Print(
Expand All @@ -540,10 +547,7 @@ void ImmutableEnumOneofFieldGenerator::GenerateBuilderClearCode(

void ImmutableEnumOneofFieldGenerator::GenerateBuildingCode(
io::Printer* printer) const {
printer->Print(variables_,
"if ($has_oneof_case_message$) {\n"
" result.$oneof_name$_ = $oneof_name$_;\n"
"}\n");
// No-Op: Handled by single statement for the oneof
}

void ImmutableEnumOneofFieldGenerator::GenerateMergingCode(
Expand Down Expand Up @@ -632,11 +636,8 @@ void ImmutableEnumOneofFieldGenerator::GenerateHashCode(
RepeatedImmutableEnumFieldGenerator::RepeatedImmutableEnumFieldGenerator(
const FieldDescriptor* descriptor, int messageBitIndex, int builderBitIndex,
Context* context)
: descriptor_(descriptor), name_resolver_(context->GetNameResolver()) {
SetEnumVariables(descriptor, messageBitIndex, builderBitIndex,
context->GetFieldGeneratorInfo(descriptor), name_resolver_,
&variables_, context);
}
: ImmutableEnumFieldGenerator(descriptor, messageBitIndex, builderBitIndex,
context) {}

RepeatedImmutableEnumFieldGenerator::~RepeatedImmutableEnumFieldGenerator() {}

Expand Down
11 changes: 5 additions & 6 deletions src/google/protobuf/compiler/java/enum_field.h
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,8 @@ class ImmutableEnumFieldGenerator : public ImmutableFieldGenerator {

// implements ImmutableFieldGenerator
// ---------------------------------------
int GetMessageBitIndex() const override;
int GetBuilderBitIndex() const override;
int GetNumBitsForMessage() const override;
int GetNumBitsForBuilder() const override;
void GenerateInterfaceMembers(io::Printer* printer) const override;
Expand All @@ -91,6 +93,8 @@ class ImmutableEnumFieldGenerator : public ImmutableFieldGenerator {

protected:
const FieldDescriptor* descriptor_;
int message_bit_index_;
int builder_bit_index_;
absl::flat_hash_map<absl::string_view, std::string> variables_;
ClassNameResolver* name_resolver_;
};
Expand Down Expand Up @@ -118,7 +122,7 @@ class ImmutableEnumOneofFieldGenerator : public ImmutableEnumFieldGenerator {
void GenerateHashCode(io::Printer* printer) const override;
};

class RepeatedImmutableEnumFieldGenerator : public ImmutableFieldGenerator {
class RepeatedImmutableEnumFieldGenerator : public ImmutableEnumFieldGenerator {
public:
explicit RepeatedImmutableEnumFieldGenerator(
const FieldDescriptor* descriptor, int messageBitIndex,
Expand Down Expand Up @@ -151,11 +155,6 @@ class RepeatedImmutableEnumFieldGenerator : public ImmutableFieldGenerator {
void GenerateKotlinDslMembers(io::Printer* printer) const override;

std::string GetBoxedType() const override;

private:
const FieldDescriptor* descriptor_;
absl::flat_hash_map<absl::string_view, std::string> variables_;
ClassNameResolver* name_resolver_;
};

} // namespace java
Expand Down
2 changes: 2 additions & 0 deletions src/google/protobuf/compiler/java/field.h
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,8 @@ class ImmutableFieldGenerator {
ImmutableFieldGenerator& operator=(const ImmutableFieldGenerator&) = delete;
virtual ~ImmutableFieldGenerator();

virtual int GetMessageBitIndex() const = 0;
virtual int GetBuilderBitIndex() const = 0;
virtual int GetNumBitsForMessage() const = 0;
virtual int GetNumBitsForBuilder() const = 0;
virtual void GenerateInterfaceMembers(io::Printer* printer) const = 0;
Expand Down
Loading

0 comments on commit c4e5996

Please sign in to comment.