From fea3fd5cb6ff88b51da60b1f33004944d93a9fce Mon Sep 17 00:00:00 2001 From: Max Cai Date: Wed, 13 Nov 2013 18:21:28 +0000 Subject: Align with main: two ways of parsing repeated packable fields. It is a requirement for parsing code to handle packed and unpacked forms on the wire for repeated packable fields. This change aligns the javanano's behavior with the java's. Bonus: optimize array length calculation when parsing repeated fixed-size-element-type fields. Bonus 2: lose "xMemoizedSerializedSize" for repeated enum fields, and make the serialized size calculation match that for repeated int32 fields. Change-Id: I8a06103d9290234adb46b0971b5ed155544fe86a --- .../compiler/javanano/javanano_enum_field.cc | 132 ++++++++++----------- .../compiler/javanano/javanano_enum_field.h | 3 + .../protobuf/compiler/javanano/javanano_field.cc | 10 ++ .../protobuf/compiler/javanano/javanano_field.h | 6 + .../protobuf/compiler/javanano/javanano_message.cc | 20 +++- .../compiler/javanano/javanano_primitive_field.cc | 91 ++++++++------ .../compiler/javanano/javanano_primitive_field.h | 1 + .../unittest_repeated_packables_nano.proto | 95 +++++++++++++++ 8 files changed, 252 insertions(+), 106 deletions(-) create mode 100644 src/google/protobuf/unittest_repeated_packables_nano.proto (limited to 'src') diff --git a/src/google/protobuf/compiler/javanano/javanano_enum_field.cc b/src/google/protobuf/compiler/javanano/javanano_enum_field.cc index b9f4fd0..17f0e26 100644 --- a/src/google/protobuf/compiler/javanano/javanano_enum_field.cc +++ b/src/google/protobuf/compiler/javanano/javanano_enum_field.cc @@ -294,10 +294,6 @@ void RepeatedEnumFieldGenerator:: GenerateMembers(io::Printer* printer) const { printer->Print(variables_, "public $type$[] $name$;\n"); - if (descriptor_->options().packed()) { - printer->Print(variables_, - "private int $name$MemoizedSerializedSize;\n"); - } } void RepeatedEnumFieldGenerator:: @@ -309,45 +305,58 @@ GenerateClearCode(io::Printer* printer) const { void RepeatedEnumFieldGenerator:: GenerateMergingCode(io::Printer* printer) const { // First, figure out the length of the array, then parse. - if (descriptor_->options().packed()) { - printer->Print(variables_, - "int length = input.readRawVarint32();\n" - "int limit = input.pushLimit(length);\n" - "// First pass to compute array length.\n" - "int arrayLength = 0;\n" - "int startPos = input.getPosition();\n" - "while (input.getBytesUntilLimit() > 0) {\n" - " input.readInt32();\n" - " arrayLength++;\n" - "}\n" - "input.rewindToPosition(startPos);\n" - "int i = this.$name$ == null ? 0 : this.$name$.length;\n" - "int[] newArray = new int[i + arrayLength];\n" - "if (i != 0) {\n" - " java.lang.System.arraycopy(this.$name$, 0, newArray, 0, i);\n" - "}\n" - "for (; i < newArray.length; i++) {\n" - " newArray[i] = input.readInt32();\n" - "}\n" - "this.$name$ = newArray;\n" - "input.popLimit(limit);\n"); - } else { - printer->Print(variables_, - "int arrayLength = com.google.protobuf.nano.WireFormatNano\n" - " .getRepeatedFieldArrayLength(input, $tag$);\n" - "int i = this.$name$ == null ? 0 : this.$name$.length;\n" - "int[] newArray = new int[i + arrayLength];\n" - "if (i != 0) {\n" - " java.lang.System.arraycopy(this.$name$, 0, newArray, 0, i);\n" - "}\n" - "for (; i < newArray.length - 1; i++) {\n" - " newArray[i] = input.readInt32();\n" - " input.readTag();\n" - "}\n" - "// Last one without readTag.\n" - "newArray[i] = input.readInt32();\n" - "this.$name$ = newArray;\n"); - } + printer->Print(variables_, + "int arrayLength = com.google.protobuf.nano.WireFormatNano\n" + " .getRepeatedFieldArrayLength(input, $tag$);\n" + "int i = this.$name$ == null ? 0 : this.$name$.length;\n" + "int[] newArray = new int[i + arrayLength];\n" + "if (i != 0) {\n" + " java.lang.System.arraycopy(this.$name$, 0, newArray, 0, i);\n" + "}\n" + "for (; i < newArray.length - 1; i++) {\n" + " newArray[i] = input.readInt32();\n" + " input.readTag();\n" + "}\n" + "// Last one without readTag.\n" + "newArray[i] = input.readInt32();\n" + "this.$name$ = newArray;\n"); +} + +void RepeatedEnumFieldGenerator:: +GenerateMergingCodeFromPacked(io::Printer* printer) const { + printer->Print(variables_, + "int length = input.readRawVarint32();\n" + "int limit = input.pushLimit(length);\n" + "// First pass to compute array length.\n" + "int arrayLength = 0;\n" + "int startPos = input.getPosition();\n" + "while (input.getBytesUntilLimit() > 0) {\n" + " input.readInt32();\n" + " arrayLength++;\n" + "}\n" + "input.rewindToPosition(startPos);\n" + "int i = this.$name$ == null ? 0 : this.$name$.length;\n" + "int[] newArray = new int[i + arrayLength];\n" + "if (i != 0) {\n" + " java.lang.System.arraycopy(this.$name$, 0, newArray, 0, i);\n" + "}\n" + "for (; i < newArray.length; i++) {\n" + " newArray[i] = input.readInt32();\n" + "}\n" + "this.$name$ = newArray;\n" + "input.popLimit(limit);\n"); +} + +void RepeatedEnumFieldGenerator:: +GenerateRepeatedDataSizeCode(io::Printer* printer) const { + // Creates a variable dataSize and puts the serialized size in there. + printer->Print(variables_, + "int dataSize = 0;\n" + "for (int i = 0; i < this.$name$.length; i++) {\n" + " int element = this.$name$[i];\n" + " dataSize += com.google.protobuf.nano.CodedOutputByteBufferNano\n" + " .computeInt32SizeNoTag(element);\n" + "}\n"); } void RepeatedEnumFieldGenerator:: @@ -357,18 +366,20 @@ GenerateSerializationCode(io::Printer* printer) const { printer->Indent(); if (descriptor_->options().packed()) { + GenerateRepeatedDataSizeCode(printer); printer->Print(variables_, "output.writeRawVarint32($tag$);\n" - "output.writeRawVarint32($name$MemoizedSerializedSize);\n" - "for (int element : this.$name$) {\n" - " output.writeRawVarint32(element);\n" + "output.writeRawVarint32(dataSize);\n" + "for (int i = 0; i < this.$name$.length; i++) {\n" + " output.writeRawVarint32(this.$name$[i]);\n" "}\n"); } else { printer->Print(variables_, - "for (int element : this.$name$) {\n" - " output.writeInt32($number$, element);\n" + "for (int i = 0; i < this.$name$.length; i++) {\n" + " output.writeInt32($number$, this.$name$[i]);\n" "}\n"); } + printer->Outdent(); printer->Print(variables_, "}\n"); @@ -380,39 +391,24 @@ GenerateSerializedSizeCode(io::Printer* printer) const { "if (this.$name$ != null && this.$name$.length > 0) {\n"); printer->Indent(); - printer->Print(variables_, - "int dataSize = 0;\n" - "for (int element : this.$name$) {\n" - " dataSize += com.google.protobuf.nano.CodedOutputByteBufferNano\n" - " .computeInt32SizeNoTag(element);\n" - "}\n"); + GenerateRepeatedDataSizeCode(printer); printer->Print( "size += dataSize;\n"); if (descriptor_->options().packed()) { - // cache the data size for packed fields. printer->Print(variables_, "size += $tag_size$;\n" "size += com.google.protobuf.nano.CodedOutputByteBufferNano\n" - " .computeRawVarint32Size(dataSize);\n" - "$name$MemoizedSerializedSize = dataSize;\n"); + " .computeRawVarint32Size(dataSize);\n"); } else { printer->Print(variables_, - "size += $tag_size$ * this.$name$.length;\n"); + "size += $tag_size$ * this.$name$.length;\n"); } printer->Outdent(); - // set cached size to 0 for empty packed fields. - if (descriptor_->options().packed()) { - printer->Print(variables_, - "} else {\n" - " $name$MemoizedSerializedSize = 0;\n" - "}\n"); - } else { - printer->Print( - "}\n"); - } + printer->Print( + "}\n"); } void RepeatedEnumFieldGenerator:: diff --git a/src/google/protobuf/compiler/javanano/javanano_enum_field.h b/src/google/protobuf/compiler/javanano/javanano_enum_field.h index af317d8..9000d20 100644 --- a/src/google/protobuf/compiler/javanano/javanano_enum_field.h +++ b/src/google/protobuf/compiler/javanano/javanano_enum_field.h @@ -96,12 +96,15 @@ class RepeatedEnumFieldGenerator : public FieldGenerator { void GenerateMembers(io::Printer* printer) const; void GenerateClearCode(io::Printer* printer) const; void GenerateMergingCode(io::Printer* printer) const; + void GenerateMergingCodeFromPacked(io::Printer* printer) const; void GenerateSerializationCode(io::Printer* printer) const; void GenerateSerializedSizeCode(io::Printer* printer) const; void GenerateEqualsCode(io::Printer* printer) const; void GenerateHashCodeCode(io::Printer* printer) const; private: + void GenerateRepeatedDataSizeCode(io::Printer* printer) const; + const FieldDescriptor* descriptor_; map variables_; diff --git a/src/google/protobuf/compiler/javanano/javanano_field.cc b/src/google/protobuf/compiler/javanano/javanano_field.cc index 6629f96..62e133e 100644 --- a/src/google/protobuf/compiler/javanano/javanano_field.cc +++ b/src/google/protobuf/compiler/javanano/javanano_field.cc @@ -46,6 +46,16 @@ namespace javanano { FieldGenerator::~FieldGenerator() {} +void FieldGenerator::GenerateMergingCodeFromPacked(io::Printer* printer) const { + // Reaching here indicates a bug. Cases are: + // - This FieldGenerator should support packing, but this method should be + // overridden. + // - This FieldGenerator doesn't support packing, and this method should + // never have been called. + GOOGLE_LOG(FATAL) << "GenerateParsingCodeFromPacked() " + << "called on field generator that does not support packing."; +} + FieldGeneratorMap::FieldGeneratorMap(const Descriptor* descriptor, const Params ¶ms) : descriptor_(descriptor), field_generators_( diff --git a/src/google/protobuf/compiler/javanano/javanano_field.h b/src/google/protobuf/compiler/javanano/javanano_field.h index 150ec19..14b6489 100644 --- a/src/google/protobuf/compiler/javanano/javanano_field.h +++ b/src/google/protobuf/compiler/javanano/javanano_field.h @@ -60,6 +60,12 @@ class FieldGenerator { virtual void GenerateMembers(io::Printer* printer) const = 0; virtual void GenerateClearCode(io::Printer* printer) const = 0; virtual void GenerateMergingCode(io::Printer* printer) const = 0; + + // Generates code to merge from packed serialized form. The default + // implementation will fail; subclasses which can handle packed serialized + // forms will override this and print appropriate code to the printer. + virtual void GenerateMergingCodeFromPacked(io::Printer* printer) const; + virtual void GenerateSerializationCode(io::Printer* printer) const = 0; virtual void GenerateSerializedSizeCode(io::Printer* printer) const = 0; virtual void GenerateEqualsCode(io::Printer* printer) const = 0; diff --git a/src/google/protobuf/compiler/javanano/javanano_message.cc b/src/google/protobuf/compiler/javanano/javanano_message.cc index 901870f..f7ab62c 100644 --- a/src/google/protobuf/compiler/javanano/javanano_message.cc +++ b/src/google/protobuf/compiler/javanano/javanano_message.cc @@ -320,7 +320,7 @@ void MessageGenerator::GenerateMergeFromMethods(io::Printer* printer) { for (int i = 0; i < descriptor_->field_count(); i++) { const FieldDescriptor* field = sorted_fields[i]; uint32 tag = WireFormatLite::MakeTag(field->number(), - WireFormat::WireTypeForField(field)); + WireFormat::WireTypeForFieldType(field->type())); printer->Print( "case $tag$: {\n", @@ -333,6 +333,24 @@ void MessageGenerator::GenerateMergeFromMethods(io::Printer* printer) { printer->Print( " break;\n" "}\n"); + + if (field->is_packable()) { + // To make packed = true wire compatible, we generate parsing code from a + // packed version of this field regardless of field->options().packed(). + uint32 packed_tag = WireFormatLite::MakeTag(field->number(), + WireFormatLite::WIRETYPE_LENGTH_DELIMITED); + printer->Print( + "case $tag$: {\n", + "tag", SimpleItoa(packed_tag)); + printer->Indent(); + + field_generators_.get(field).GenerateMergingCodeFromPacked(printer); + + printer->Outdent(); + printer->Print( + " break;\n" + "}\n"); + } } printer->Outdent(); diff --git a/src/google/protobuf/compiler/javanano/javanano_primitive_field.cc b/src/google/protobuf/compiler/javanano/javanano_primitive_field.cc index 5887a7e..c0717e6 100644 --- a/src/google/protobuf/compiler/javanano/javanano_primitive_field.cc +++ b/src/google/protobuf/compiler/javanano/javanano_primitive_field.cc @@ -685,10 +685,46 @@ GenerateClearCode(io::Printer* printer) const { void RepeatedPrimitiveFieldGenerator:: GenerateMergingCode(io::Printer* printer) const { // First, figure out the length of the array, then parse. - if (descriptor_->is_packable() && descriptor_->options().packed()) { + printer->Print(variables_, + "int arrayLength = com.google.protobuf.nano.WireFormatNano\n" + " .getRepeatedFieldArrayLength(input, $tag$);\n" + "int i = this.$name$ == null ? 0 : this.$name$.length;\n"); + + if (GetJavaType(descriptor_) == JAVATYPE_BYTES) { + printer->Print(variables_, + "byte[][] newArray = new byte[i + arrayLength][];\n"); + } else { + printer->Print(variables_, + "$type$[] newArray = new $type$[i + arrayLength];\n"); + } + printer->Print(variables_, + "if (i != 0) {\n" + " java.lang.System.arraycopy(this.$name$, 0, newArray, 0, i);\n" + "}\n" + "for (; i < newArray.length - 1; i++) {\n" + " newArray[i] = input.read$capitalized_type$();\n" + " input.readTag();\n" + "}\n" + "// Last one without readTag.\n" + "newArray[i] = input.read$capitalized_type$();\n" + "this.$name$ = newArray;\n"); +} + +void RepeatedPrimitiveFieldGenerator:: +GenerateMergingCodeFromPacked(io::Printer* printer) const { + printer->Print( + "int length = input.readRawVarint32();\n" + "int limit = input.pushLimit(length);\n"); + + // If we know the elements will all be of the same size, the arrayLength + // can be calculated much more easily. However, FixedSize() returns 1 for + // repeated bool fields, which are guaranteed to have the fixed size of + // 1 byte per value only if we control the output. On the wire they can + // legally appear as variable-size integers, so we need to use the slow + // way for repeated bool fields. + if (descriptor_->type() == FieldDescriptor::TYPE_BOOL + || FixedSize(descriptor_->type()) == -1) { printer->Print(variables_, - "int length = input.readRawVarint32();\n" - "int limit = input.pushLimit(length);\n" "// First pass to compute array length.\n" "int arrayLength = 0;\n" "int startPos = input.getPosition();\n" @@ -696,42 +732,23 @@ GenerateMergingCode(io::Printer* printer) const { " input.read$capitalized_type$();\n" " arrayLength++;\n" "}\n" - "input.rewindToPosition(startPos);\n" - "int i = this.$name$ == null ? 0 : this.$name$.length;\n" - "$type$[] newArray = new $type$[i + arrayLength];\n" - "if (i != 0) {\n" - " java.lang.System.arraycopy(this.$name$, 0, newArray, 0, i);\n" - "}\n" - "for (; i < newArray.length; i++) {\n" - " newArray[i] = input.read$capitalized_type$();\n" - "}\n" - "this.$name$ = newArray;\n" - "input.popLimit(limit);\n"); + "input.rewindToPosition(startPos);\n"); } else { printer->Print(variables_, - "int arrayLength = com.google.protobuf.nano.WireFormatNano\n" - " .getRepeatedFieldArrayLength(input, $tag$);\n" - "int i = this.$name$ == null ? 0 : this.$name$.length;\n"); - - if (GetJavaType(descriptor_) == JAVATYPE_BYTES) { - printer->Print(variables_, - "byte[][] newArray = new byte[i + arrayLength][];\n"); - } else { - printer->Print(variables_, - "$type$[] newArray = new $type$[i + arrayLength];\n"); - } - printer->Print(variables_, - "if (i != 0) {\n" - " java.lang.System.arraycopy(this.$name$, 0, newArray, 0, i);\n" - "}\n" - "for (; i < newArray.length - 1; i++) {\n" - " newArray[i] = input.read$capitalized_type$();\n" - " input.readTag();\n" - "}\n" - "// Last one without readTag.\n" - "newArray[i] = input.read$capitalized_type$();\n" - "this.$name$ = newArray;\n"); + "int arrayLength = length / $fixed_size$;\n"); } + + printer->Print(variables_, + "int i = this.$name$ == null ? 0 : this.$name$.length;\n" + "$type$[] newArray = new $type$[i + arrayLength];\n" + "if (i != 0) {\n" + " java.lang.System.arraycopy(this.$name$, 0, newArray, 0, i);\n" + "}\n" + "for (; i < newArray.length; i++) {\n" + " newArray[i] = input.read$capitalized_type$();\n" + "}\n" + "this.$name$ = newArray;\n" + "input.popLimit(limit);\n"); } void RepeatedPrimitiveFieldGenerator:: @@ -812,7 +829,7 @@ GenerateSerializedSizeCode(io::Printer* printer) const { printer->Print(variables_, "size += $tag_size$;\n" "size += com.google.protobuf.nano.CodedOutputByteBufferNano\n" - " .computeRawVarint32Size(dataSize);\n"); + " .computeRawVarint32Size(dataSize);\n"); } else if (IsReferenceType(GetJavaType(descriptor_))) { printer->Print(variables_, "size += $tag_size$ * dataCount;\n"); diff --git a/src/google/protobuf/compiler/javanano/javanano_primitive_field.h b/src/google/protobuf/compiler/javanano/javanano_primitive_field.h index 2d2b268..d207535 100644 --- a/src/google/protobuf/compiler/javanano/javanano_primitive_field.h +++ b/src/google/protobuf/compiler/javanano/javanano_primitive_field.h @@ -98,6 +98,7 @@ class RepeatedPrimitiveFieldGenerator : public FieldGenerator { void GenerateMembers(io::Printer* printer) const; void GenerateClearCode(io::Printer* printer) const; void GenerateMergingCode(io::Printer* printer) const; + void GenerateMergingCodeFromPacked(io::Printer* printer) const; void GenerateSerializationCode(io::Printer* printer) const; void GenerateSerializedSizeCode(io::Printer* printer) const; void GenerateEqualsCode(io::Printer* printer) const; diff --git a/src/google/protobuf/unittest_repeated_packables_nano.proto b/src/google/protobuf/unittest_repeated_packables_nano.proto new file mode 100644 index 0000000..1c78918 --- /dev/null +++ b/src/google/protobuf/unittest_repeated_packables_nano.proto @@ -0,0 +1,95 @@ +// Protocol Buffers - Google's data interchange format +// Copyright 2008 Google Inc. All rights reserved. +// http://code.google.com/p/protobuf/ +// +// Redistribution and use in source and binary forms, with or without +// modification, are permitted provided that the following conditions are +// met: +// +// * Redistributions of source code must retain the above copyright +// notice, this list of conditions and the following disclaimer. +// * Redistributions in binary form must reproduce the above +// copyright notice, this list of conditions and the following disclaimer +// in the documentation and/or other materials provided with the +// distribution. +// * Neither the name of Google Inc. nor the names of its +// contributors may be used to endorse or promote products derived from +// this software without specific prior written permission. +// +// THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS +// "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT +// LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR +// A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT +// OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, +// SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT +// LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, +// DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY +// THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT +// (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE +// OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. + +// Author: maxtroy@google.com (Max Cai) + +package protobuf_unittest; + +option java_package = "com.google.protobuf.nano"; +option java_outer_classname = "NanoRepeatedPackables"; + +enum Enum { + OPTION_ONE = 1; + OPTION_TWO = 2; +} + +// Two almost identical messages with all packable repeated field types. +// One with none marked as packed and the other all packed. For +// compatibility, they should be able to parse each other's serialized +// forms. + +message NonPacked { + + // All packable types, none marked as packed. + + repeated int32 int32s = 1; + repeated int64 int64s = 2; + repeated uint32 uint32s = 3; + repeated uint64 uint64s = 4; + repeated sint32 sint32s = 5; + repeated sint64 sint64s = 6; + repeated fixed32 fixed32s = 7; + repeated fixed64 fixed64s = 8; + repeated sfixed32 sfixed32s = 9; + repeated sfixed64 sfixed64s = 10; + repeated float floats = 11; + repeated double doubles = 12; + repeated bool bools = 13; + repeated Enum enums = 14; + + // Noise for testing merged deserialization. + optional int32 noise = 15; + +} + +message Packed { + + // All packable types, all matching the field numbers in NonPacked, + // all marked as packed. + + repeated int32 int32s = 1 [ packed = true ]; + repeated int64 int64s = 2 [ packed = true ]; + repeated uint32 uint32s = 3 [ packed = true ]; + repeated uint64 uint64s = 4 [ packed = true ]; + repeated sint32 sint32s = 5 [ packed = true ]; + repeated sint64 sint64s = 6 [ packed = true ]; + repeated fixed32 fixed32s = 7 [ packed = true ]; + repeated fixed64 fixed64s = 8 [ packed = true ]; + repeated sfixed32 sfixed32s = 9 [ packed = true ]; + repeated sfixed64 sfixed64s = 10 [ packed = true ]; + repeated float floats = 11 [ packed = true ]; + repeated double doubles = 12 [ packed = true ]; + repeated bool bools = 13 [ packed = true ]; + repeated Enum enums = 14 [ packed = true ]; + + // Noise for testing merged deserialization. + optional int32 noise = 15; + +} -- cgit v1.1