From b0b926a141e261ab1e4d41715c1d51cc31db46dd Mon Sep 17 00:00:00 2001 From: Protobuf Team Bot Date: Wed, 21 Jun 2023 13:52:55 -0700 Subject: [PATCH] Use ProtobufArrayList for repeated bytes field. Presize primitive arrays for fixed-length primitives. PiperOrigin-RevId: 542353392 --- .../protobuf/compiler/java/message_builder.cc | 1 - .../protobuf/compiler/java/primitive_field.cc | 185 +++++++----------- 2 files changed, 71 insertions(+), 115 deletions(-) diff --git a/src/google/protobuf/compiler/java/message_builder.cc b/src/google/protobuf/compiler/java/message_builder.cc index 753add4c70..fe93feed5c 100644 --- a/src/google/protobuf/compiler/java/message_builder.cc +++ b/src/google/protobuf/compiler/java/message_builder.cc @@ -87,7 +87,6 @@ bool BitfieldTracksMutability(const FieldDescriptor* const descriptor) { case FieldDescriptor::TYPE_GROUP: case FieldDescriptor::TYPE_MESSAGE: case FieldDescriptor::TYPE_ENUM: - case FieldDescriptor::TYPE_BYTES: return true; default: return false; diff --git a/src/google/protobuf/compiler/java/primitive_field.cc b/src/google/protobuf/compiler/java/primitive_field.cc index 1c0f994b49..35f0571647 100644 --- a/src/google/protobuf/compiler/java/primitive_field.cc +++ b/src/google/protobuf/compiler/java/primitive_field.cc @@ -44,6 +44,7 @@ #include "google/protobuf/compiler/java/doc_comment.h" #include "google/protobuf/compiler/java/helpers.h" #include "google/protobuf/compiler/java/name_resolver.h" +#include "google/protobuf/descriptor.h" #include "google/protobuf/io/printer.h" #include "google/protobuf/wire_format.h" @@ -71,6 +72,7 @@ void SetPrimitiveVariables( variables->insert({"field_type", (*variables)["type"]}); std::string name = (*variables)["name"]; + (*variables)["name_make_immutable"] = absl::StrCat(name, "_.makeImmutable()"); if (javaType == JAVATYPE_BOOLEAN || javaType == JAVATYPE_DOUBLE || javaType == JAVATYPE_FLOAT || javaType == JAVATYPE_INT || javaType == JAVATYPE_LONG) { @@ -80,12 +82,6 @@ void SetPrimitiveVariables( absl::StrCat("com.google.protobuf.Internal.", capitalized_type, "List"); (*variables)["empty_list"] = absl::StrCat("empty", capitalized_type, "List()"); - (*variables)["create_list"] = - absl::StrCat("new", capitalized_type, "List()"); - (*variables)["mutable_copy_list"] = - absl::StrCat("mutableCopy(", name, "_)"); - (*variables)["name_make_immutable"] = - absl::StrCat(name, "_.makeImmutable()"); (*variables)["repeated_get"] = absl::StrCat(name, "_.get", capitalized_type); (*variables)["repeated_add"] = @@ -93,16 +89,11 @@ void SetPrimitiveVariables( (*variables)["repeated_set"] = absl::StrCat(name, "_.set", capitalized_type); } else { - std::string boxed_type = (*variables)["boxed_type"]; (*variables)["field_list_type"] = - absl::StrCat("java.util.List<", boxed_type, ">"); - (*variables)["create_list"] = - absl::StrCat("new java.util.ArrayList<", boxed_type, ">()"); - (*variables)["mutable_copy_list"] = - absl::StrCat("new java.util.ArrayList<", boxed_type, ">(", name, "_)"); - (*variables)["empty_list"] = "java.util.Collections.emptyList()"; - (*variables)["name_make_immutable"] = absl::StrCat( - name, "_ = java.util.Collections.unmodifiableList(", name, "_)"); + "com.google.protobuf.Internal.ProtobufList"; + (*variables)["empty_list"] = + "emptyList(com.google.protobuf.ByteString.class)"; (*variables)["repeated_get"] = absl::StrCat(name, "_.get"); (*variables)["repeated_add"] = absl::StrCat(name, "_.add"); (*variables)["repeated_set"] = absl::StrCat(name, "_.set"); @@ -171,13 +162,6 @@ void SetPrimitiveVariables( break; } } - // For repeated bytes builders, one bit is used for whether the array is - // mutable. - // TODO(b/255468704): migrate bytes field to ProtobufList so that we can - // use the bit field to track their presence instead of mutability. - (*variables)["get_mutable_bit_builder"] = GenerateGetBit(builderBitIndex); - (*variables)["set_mutable_bit_builder"] = GenerateSetBit(builderBitIndex); - (*variables)["clear_mutable_bit_builder"] = GenerateClearBit(builderBitIndex); // Always track the presence of a field explicitly in the builder, regardless // of syntax. @@ -731,34 +715,29 @@ void RepeatedImmutablePrimitiveFieldGenerator::GenerateMembers( void RepeatedImmutablePrimitiveFieldGenerator::GenerateBuilderMembers( io::Printer* printer) const { - // One field is the list and the bit field keeps track of whether the - // list is immutable. If it's immutable, the invariant is that it must - // either an instance of Collections.emptyList() or it's an ArrayList - // wrapped in a Collections.unmodifiableList() wrapper and nobody else has - // a reference to the underlying ArrayList. This invariant allows us to - // share instances of lists between protocol buffers avoiding expensive - // memory allocations. Note, immutable is a strong guarantee here -- not - // just that the list cannot be modified via the reference but that the - // list can never be modified. + // We use a ProtobufArrayList because it starts as a mutable list that can be + // switched to immutable when references are handed out. This allows copy-free + // sharing. A bit in the bitfield tracks whether there are any items in the + // list. The presence bit allows us to skip work on blocks of 32 fields by + // by checking if the entire bit-field int == 0 (none of the fields are + // present). printer->Print(variables_, - "private $field_list_type$ $name$_ = $empty_list$;\n"); - - if (descriptor_->type() == FieldDescriptor::TYPE_BYTES) { - printer->Print(variables_, - "private void ensure$capitalized_name$IsMutable() {\n" - " if (!$get_mutable_bit_builder$) {\n" - " $name$_ = $mutable_copy_list$;\n" - " $set_mutable_bit_builder$;\n" - " }\n" - "}\n"); - } else { - printer->Print(variables_, - "private void ensure$capitalized_name$IsMutable() {\n" - " if (!$name$_.isModifiable()) {\n" - " $name$_ = $mutable_copy_list$;\n" - " }\n" - " $set_has_field_bit_builder$\n" - "}\n"); + "private $field_list_type$ $name$_ = $empty_list$;\n" + "private void ensure$capitalized_name$IsMutable() {\n" + " if (!$name$_.isModifiable()) {\n" + " $name$_ = makeMutableCopy($name$_);\n" + " }\n" + " $set_has_field_bit_builder$\n" + "}\n"); + if (FixedSize(GetType(descriptor_)) != -1) { + printer->Print( + variables_, + "private void ensure$capitalized_name$IsMutable(int capacity) {\n" + " if (!$name$_.isModifiable()) {\n" + " $name$_ = makeMutableCopy($name$_, capacity);\n" + " }\n" + " $set_has_field_bit_builder$\n" + "}\n"); } // Note: We return an unmodifiable list because otherwise the caller @@ -766,22 +745,12 @@ void RepeatedImmutablePrimitiveFieldGenerator::GenerateBuilderMembers( // has been built, thus mutating the message which is supposed to be // immutable. WriteFieldAccessorDocComment(printer, descriptor_, LIST_GETTER); - if (descriptor_->type() == FieldDescriptor::TYPE_BYTES) { - printer->Print(variables_, - "$deprecation$public java.util.List<$boxed_type$>\n" - " ${$get$capitalized_name$List$}$() {\n" - " return $get_mutable_bit_builder$ ?\n" - " java.util.Collections.unmodifiableList($name$_) " - ": $name$_;\n" - "}\n"); - } else { - printer->Print(variables_, - "$deprecation$public java.util.List<$boxed_type$>\n" - " ${$get$capitalized_name$List$}$() {\n" - " $name$_.makeImmutable();\n" - " return $name$_;\n" - "}\n"); - } + printer->Print(variables_, + "$deprecation$public java.util.List<$boxed_type$>\n" + " ${$get$capitalized_name$List$}$() {\n" + " $name$_.makeImmutable();\n" + " return $name$_;\n" + "}\n"); printer->Annotate("{", "}", descriptor_); WriteFieldAccessorDocComment(printer, descriptor_, LIST_COUNT); printer->Print( @@ -804,15 +773,12 @@ void RepeatedImmutablePrimitiveFieldGenerator::GenerateBuilderMembers( " int index, $type$ value) {\n" " $null_check$\n" " ensure$capitalized_name$IsMutable();\n" - " $repeated_set$(index, value);\n"); - printer->Annotate("{", "}", descriptor_, Semantic::kSet); - if (descriptor_->type() != FieldDescriptor::TYPE_BYTES) { - printer->Print(variables_, " $set_has_field_bit_builder$\n"); - } - printer->Print(variables_, + " $repeated_set$(index, value);\n" + " $set_has_field_bit_builder$\n" " $on_changed$\n" " return this;\n" "}\n"); + printer->Annotate("{", "}", descriptor_, Semantic::kSet); WriteFieldAccessorDocComment(printer, descriptor_, LIST_ADDER, /* builder */ true); printer->Print(variables_, @@ -820,15 +786,12 @@ void RepeatedImmutablePrimitiveFieldGenerator::GenerateBuilderMembers( "${$add$capitalized_name$$}$($type$ value) {\n" " $null_check$\n" " ensure$capitalized_name$IsMutable();\n" - " $repeated_add$(value);\n"); - printer->Annotate("{", "}", descriptor_, Semantic::kSet); - if (descriptor_->type() != FieldDescriptor::TYPE_BYTES) { - printer->Print(variables_, " $set_has_field_bit_builder$\n"); - } - printer->Print(variables_, + " $repeated_add$(value);\n" + " $set_has_field_bit_builder$\n" " $on_changed$\n" " return this;\n" "}\n"); + printer->Annotate("{", "}", descriptor_, Semantic::kSet); WriteFieldAccessorDocComment(printer, descriptor_, LIST_MULTI_ADDER, /* builder */ true); printer->Print(variables_, @@ -836,22 +799,19 @@ void RepeatedImmutablePrimitiveFieldGenerator::GenerateBuilderMembers( " java.lang.Iterable values) {\n" " ensure$capitalized_name$IsMutable();\n" " com.google.protobuf.AbstractMessageLite.Builder.addAll(\n" - " values, $name$_);\n"); - printer->Annotate("{", "}", descriptor_, Semantic::kSet); - if (descriptor_->type() != FieldDescriptor::TYPE_BYTES) { - printer->Print(variables_, " $set_has_field_bit_builder$\n"); - } - printer->Print(variables_, + " values, $name$_);\n" + " $set_has_field_bit_builder$\n" " $on_changed$\n" " return this;\n" "}\n"); + printer->Annotate("{", "}", descriptor_, Semantic::kSet); WriteFieldAccessorDocComment(printer, descriptor_, CLEARER, /* builder */ true); printer->Print( variables_, "$deprecation$public Builder ${$clear$capitalized_name$$}$() {\n" " $name$_ = $empty_list$;\n" - " $clear_mutable_bit_builder$;\n" + " $clear_has_field_bit_builder$\n" " $on_changed$\n" " return this;\n" "}\n"); @@ -977,14 +937,9 @@ void RepeatedImmutablePrimitiveFieldGenerator::GenerateMergingCode( printer->Print(variables_, "if (!other.$name$_.isEmpty()) {\n" " if ($name$_.isEmpty()) {\n" - " $name$_ = other.$name$_;\n"); - if (descriptor_->type() == FieldDescriptor::TYPE_BYTES) { - printer->Print(variables_, " $clear_mutable_bit_builder$;\n"); - } else { - printer->Print(variables_, - " $name$_.makeImmutable();\n" - " $set_has_field_bit_builder$\n"); - } + " $name$_ = other.$name$_;\n" + " $name_make_immutable$;\n" + " $set_has_field_bit_builder$\n"); printer->Print(variables_, " } else {\n" " ensure$capitalized_name$IsMutable();\n" @@ -998,20 +953,11 @@ void RepeatedImmutablePrimitiveFieldGenerator::GenerateBuildingCode( io::Printer* printer) const { // The code below ensures that the result has an immutable list. If our // list is immutable, we can just reuse it. If not, we make it immutable. - if (descriptor_->type() == FieldDescriptor::TYPE_BYTES) { - printer->Print(variables_, - "if ($get_mutable_bit_builder$) {\n" - " $name_make_immutable$;\n" - " $clear_mutable_bit_builder$;\n" - "}\n" - "result.$name$_ = $name$_;\n"); - } else { - printer->Print(variables_, - "if ($get_has_field_bit_from_local$) {\n" - " $name_make_immutable$;\n" - " result.$name$_ = $name$_;\n" - "}\n"); - } + printer->Print(variables_, + "if ($get_has_field_bit_from_local$) {\n" + " $name_make_immutable$;\n" + " result.$name$_ = $name$_;\n" + "}\n"); } void RepeatedImmutablePrimitiveFieldGenerator::GenerateBuilderParsingCode( @@ -1024,14 +970,25 @@ void RepeatedImmutablePrimitiveFieldGenerator::GenerateBuilderParsingCode( void RepeatedImmutablePrimitiveFieldGenerator:: GenerateBuilderParsingCodeFromPacked(io::Printer* printer) const { - printer->Print(variables_, - "int length = input.readRawVarint32();\n" - "int limit = input.pushLimit(length);\n" - "ensure$capitalized_name$IsMutable();\n" - "while (input.getBytesUntilLimit() > 0) {\n" - " $repeated_add$(input.read$capitalized_type$());\n" - "}\n" - "input.popLimit(limit);\n"); + if (FixedSize(GetType(descriptor_)) != -1) { + printer->Print(variables_, + "int length = input.readRawVarint32();\n" + "int limit = input.pushLimit(length);\n" + "ensure$capitalized_name$IsMutable(length / $fixed_size$);\n" + "while (input.getBytesUntilLimit() > 0) {\n" + " $repeated_add$(input.read$capitalized_type$());\n" + "}\n" + "input.popLimit(limit);\n"); + } else { + printer->Print(variables_, + "int length = input.readRawVarint32();\n" + "int limit = input.pushLimit(length);\n" + "ensure$capitalized_name$IsMutable();\n" + "while (input.getBytesUntilLimit() > 0) {\n" + " $repeated_add$(input.read$capitalized_type$());\n" + "}\n" + "input.popLimit(limit);\n"); + } } void RepeatedImmutablePrimitiveFieldGenerator::GenerateSerializationCode(