From 8d1a55b6183dc898395014ed56a886e58ebe7b03 Mon Sep 17 00:00:00 2001 From: Adam Cozzette Date: Wed, 5 Jan 2022 08:50:29 -0800 Subject: [PATCH] Improve performance of parsing unknown fields in Java (#9371) Credit should go to @elharo for most of these Java changes--I am just cherry-picking them from our internal codebase. The one thing I did change was to give the UTF-8 validation tests their own Bazel test target. This makes it possible to give the other tests a shorter timeout, which is important for UnknownFieldSetPerformanceTest in particular. --- Makefile.am | 1 + .../com/google/protobuf/UnknownFieldSet.java | 427 +++++++++--------- .../UnknownFieldSetPerformanceTest.java | 78 ++++ .../google/protobuf/UnknownFieldSetTest.java | 16 +- java/lite/pom.xml | 1 + 5 files changed, 310 insertions(+), 213 deletions(-) create mode 100644 java/core/src/test/java/com/google/protobuf/UnknownFieldSetPerformanceTest.java diff --git a/Makefile.am b/Makefile.am index 915184213ce9..5e07bb6749cc 100644 --- a/Makefile.am +++ b/Makefile.am @@ -490,6 +490,7 @@ java_EXTRA_DIST= java/core/src/test/java/com/google/protobuf/TypeRegistryTest.java \ java/core/src/test/java/com/google/protobuf/UnknownEnumValueTest.java \ java/core/src/test/java/com/google/protobuf/UnknownFieldSetTest.java \ + java/core/src/test/java/com/google/protobuf/UnknownFieldSetPerformanceTest.java \ java/core/src/test/java/com/google/protobuf/UnmodifiableLazyStringListTest.java \ java/core/src/test/java/com/google/protobuf/Utf8Test.java \ java/core/src/test/java/com/google/protobuf/Utf8Utils.java \ diff --git a/java/core/src/main/java/com/google/protobuf/UnknownFieldSet.java b/java/core/src/main/java/com/google/protobuf/UnknownFieldSet.java index ba2f9df08e67..5c482d62dab5 100644 --- a/java/core/src/main/java/com/google/protobuf/UnknownFieldSet.java +++ b/java/core/src/main/java/com/google/protobuf/UnknownFieldSet.java @@ -43,13 +43,13 @@ import java.util.TreeMap; /** - * {@code UnknownFieldSet} is used to keep track of fields which were seen when parsing a protocol + * {@code UnknownFieldSet} keeps track of fields which were seen when parsing a protocol * message but whose field numbers or types are unrecognized. This most frequently occurs when new * fields are added to a message type and then messages containing those fields are read by old * software that was compiled before the new types were added. * *

Every {@link Message} contains an {@code UnknownFieldSet} (and every {@link Message.Builder} - * contains an {@link Builder}). + * contains a {@link Builder}). * *

Most users will never need to use this class. * @@ -57,9 +57,13 @@ */ public final class UnknownFieldSet implements MessageLite { - private UnknownFieldSet() { - fields = null; - fieldsDescending = null; + private final TreeMap fields; + + /** + * Construct an {@code UnknownFieldSet} around the given map. + */ + UnknownFieldSet(TreeMap fields) { + this.fields = fields; } /** Create a new {@link Builder}. */ @@ -68,7 +72,7 @@ public static Builder newBuilder() { } /** Create a new {@link Builder} and initialize it to be a copy of {@code copyFrom}. */ - public static Builder newBuilder(final UnknownFieldSet copyFrom) { + public static Builder newBuilder(UnknownFieldSet copyFrom) { return newBuilder().mergeFrom(copyFrom); } @@ -83,25 +87,11 @@ public UnknownFieldSet getDefaultInstanceForType() { } private static final UnknownFieldSet defaultInstance = - new UnknownFieldSet( - Collections.emptyMap(), Collections.emptyMap()); - - /** - * Construct an {@code UnknownFieldSet} around the given map. The map is expected to be immutable. - */ - UnknownFieldSet(final Map fields, final Map fieldsDescending) { - this.fields = fields; - this.fieldsDescending = fieldsDescending; - } - - private final Map fields; - - /** A copy of {@link #fields} who's iterator order is reversed. */ - private final Map fieldsDescending; + new UnknownFieldSet(new TreeMap()); @Override - public boolean equals(final Object other) { + public boolean equals(Object other) { if (this == other) { return true; } @@ -110,29 +100,33 @@ public boolean equals(final Object other) { @Override public int hashCode() { + if (fields.isEmpty()) { // avoid allocation of iterator. + // This optimization may not be helpful but it is needed for the allocation tests to pass. + return 0; + } return fields.hashCode(); } /** Get a map of fields in the set by number. */ public Map asMap() { - return fields; + return (Map) fields.clone(); } /** Check if the given field number is present in the set. */ - public boolean hasField(final int number) { + public boolean hasField(int number) { return fields.containsKey(number); } /** Get a field by number. Returns an empty field if not present. Never returns {@code null}. */ - public Field getField(final int number) { - final Field result = fields.get(number); + public Field getField(int number) { + Field result = fields.get(number); return (result == null) ? Field.getDefaultInstance() : result; } /** Serializes the set and writes it to {@code output}. */ @Override - public void writeTo(final CodedOutputStream output) throws IOException { - for (final Map.Entry entry : fields.entrySet()) { + public void writeTo(CodedOutputStream output) throws IOException { + for (Map.Entry entry : fields.entrySet()) { Field field = entry.getValue(); field.writeTo(entry.getKey(), output); } @@ -154,10 +148,10 @@ public String toString() { @Override public ByteString toByteString() { try { - final ByteString.CodedBuilder out = ByteString.newCodedBuilder(getSerializedSize()); + ByteString.CodedBuilder out = ByteString.newCodedBuilder(getSerializedSize()); writeTo(out.getCodedOutput()); return out.build(); - } catch (final IOException e) { + } catch (IOException e) { throw new RuntimeException( "Serializing to a ByteString threw an IOException (should never happen).", e); } @@ -170,12 +164,12 @@ public ByteString toByteString() { @Override public byte[] toByteArray() { try { - final byte[] result = new byte[getSerializedSize()]; - final CodedOutputStream output = CodedOutputStream.newInstance(result); + byte[] result = new byte[getSerializedSize()]; + CodedOutputStream output = CodedOutputStream.newInstance(result); writeTo(output); output.checkNoSpaceLeft(); return result; - } catch (final IOException e) { + } catch (IOException e) { throw new RuntimeException( "Serializing to a byte array threw an IOException (should never happen).", e); } @@ -186,16 +180,16 @@ public byte[] toByteArray() { * {@link #writeTo(CodedOutputStream)}. */ @Override - public void writeTo(final OutputStream output) throws IOException { - final CodedOutputStream codedOutput = CodedOutputStream.newInstance(output); + public void writeTo(OutputStream output) throws IOException { + CodedOutputStream codedOutput = CodedOutputStream.newInstance(output); writeTo(codedOutput); codedOutput.flush(); } @Override public void writeDelimitedTo(OutputStream output) throws IOException { - final CodedOutputStream codedOutput = CodedOutputStream.newInstance(output); - codedOutput.writeRawVarint32(getSerializedSize()); + CodedOutputStream codedOutput = CodedOutputStream.newInstance(output); + codedOutput.writeUInt32NoTag(getSerializedSize()); writeTo(codedOutput); codedOutput.flush(); } @@ -204,15 +198,17 @@ public void writeDelimitedTo(OutputStream output) throws IOException { @Override public int getSerializedSize() { int result = 0; - for (final Map.Entry entry : fields.entrySet()) { - result += entry.getValue().getSerializedSize(entry.getKey()); + if (!fields.isEmpty()) { + for (Map.Entry entry : fields.entrySet()) { + result += entry.getValue().getSerializedSize(entry.getKey()); + } } return result; } /** Serializes the set and writes it to {@code output} using {@code MessageSet} wire format. */ - public void writeAsMessageSetTo(final CodedOutputStream output) throws IOException { - for (final Map.Entry entry : fields.entrySet()) { + public void writeAsMessageSetTo(CodedOutputStream output) throws IOException { + for (Map.Entry entry : fields.entrySet()) { entry.getValue().writeAsMessageSetExtensionTo(entry.getKey(), output); } } @@ -221,7 +217,7 @@ public void writeAsMessageSetTo(final CodedOutputStream output) throws IOExcepti void writeTo(Writer writer) throws IOException { if (writer.fieldOrder() == Writer.FieldOrder.DESCENDING) { // Write fields in descending order. - for (Map.Entry entry : fieldsDescending.entrySet()) { + for (Map.Entry entry : fields.descendingMap().entrySet()) { entry.getValue().writeTo(entry.getKey(), writer); } } else { @@ -233,15 +229,15 @@ void writeTo(Writer writer) throws IOException { } /** Serializes the set and writes it to {@code writer} using {@code MessageSet} wire format. */ - void writeAsMessageSetTo(final Writer writer) throws IOException { + void writeAsMessageSetTo(Writer writer) throws IOException { if (writer.fieldOrder() == Writer.FieldOrder.DESCENDING) { // Write fields in descending order. - for (final Map.Entry entry : fieldsDescending.entrySet()) { + for (Map.Entry entry : fields.descendingMap().entrySet()) { entry.getValue().writeAsMessageSetExtensionTo(entry.getKey(), writer); } } else { // Write fields in ascending order. - for (final Map.Entry entry : fields.entrySet()) { + for (Map.Entry entry : fields.entrySet()) { entry.getValue().writeAsMessageSetExtensionTo(entry.getKey(), writer); } } @@ -250,7 +246,7 @@ void writeAsMessageSetTo(final Writer writer) throws IOException { /** Get the number of bytes required to encode this set using {@code MessageSet} wire format. */ public int getSerializedSizeAsMessageSet() { int result = 0; - for (final Map.Entry entry : fields.entrySet()) { + for (Map.Entry entry : fields.entrySet()) { result += entry.getValue().getSerializedSizeAsMessageSetExtension(entry.getKey()); } return result; @@ -264,23 +260,23 @@ public boolean isInitialized() { } /** Parse an {@code UnknownFieldSet} from the given input stream. */ - public static UnknownFieldSet parseFrom(final CodedInputStream input) throws IOException { + public static UnknownFieldSet parseFrom(CodedInputStream input) throws IOException { return newBuilder().mergeFrom(input).build(); } /** Parse {@code data} as an {@code UnknownFieldSet} and return it. */ - public static UnknownFieldSet parseFrom(final ByteString data) + public static UnknownFieldSet parseFrom(ByteString data) throws InvalidProtocolBufferException { return newBuilder().mergeFrom(data).build(); } /** Parse {@code data} as an {@code UnknownFieldSet} and return it. */ - public static UnknownFieldSet parseFrom(final byte[] data) throws InvalidProtocolBufferException { + public static UnknownFieldSet parseFrom(byte[] data) throws InvalidProtocolBufferException { return newBuilder().mergeFrom(data).build(); } /** Parse an {@code UnknownFieldSet} from {@code input} and return it. */ - public static UnknownFieldSet parseFrom(final InputStream input) throws IOException { + public static UnknownFieldSet parseFrom(InputStream input) throws IOException { return newBuilder().mergeFrom(input).build(); } @@ -309,64 +305,43 @@ public static final class Builder implements MessageLite.Builder { // This constructor should never be called directly (except from 'create'). private Builder() {} - private Map fields; - - // Optimization: We keep around a builder for the last field that was - // modified so that we can efficiently add to it multiple times in a - // row (important when parsing an unknown repeated field). - private int lastFieldNumber; - private Field.Builder lastField; + private TreeMap fieldBuilders = new TreeMap<>(); private static Builder create() { - Builder builder = new Builder(); - builder.reinitialize(); - return builder; + return new Builder(); } /** * Get a field builder for the given field number which includes any values that already exist. */ - private Field.Builder getFieldBuilder(final int number) { - if (lastField != null) { - if (number == lastFieldNumber) { - return lastField; - } - // Note: addField() will reset lastField and lastFieldNumber. - addField(lastFieldNumber, lastField.build()); - } + private Field.Builder getFieldBuilder(int number) { if (number == 0) { return null; } else { - final Field existing = fields.get(number); - lastFieldNumber = number; - lastField = Field.newBuilder(); - if (existing != null) { - lastField.mergeFrom(existing); + Field.Builder builder = fieldBuilders.get(number); + if (builder == null) { + builder = Field.newBuilder(); + fieldBuilders.put(number, builder); } - return lastField; + return builder; } } /** * Build the {@link UnknownFieldSet} and return it. - * - *

Once {@code build()} has been called, the {@code Builder} will no longer be usable. - * Calling any method after {@code build()} will result in undefined behavior and can cause a - * {@code NullPointerException} to be thrown. */ @Override public UnknownFieldSet build() { - getFieldBuilder(0); // Force lastField to be built. - final UnknownFieldSet result; - if (fields.isEmpty()) { + UnknownFieldSet result; + if (fieldBuilders.isEmpty()) { result = getDefaultInstance(); } else { - Map descendingFields = null; - descendingFields = - Collections.unmodifiableMap(((TreeMap) fields).descendingMap()); - result = new UnknownFieldSet(Collections.unmodifiableMap(fields), descendingFields); + TreeMap fields = new TreeMap<>(); + for (Map.Entry entry : fieldBuilders.entrySet()) { + fields.put(entry.getKey(), entry.getValue().build()); + } + result = new UnknownFieldSet(fields); } - fields = null; return result; } @@ -378,11 +353,13 @@ public UnknownFieldSet buildPartial() { @Override public Builder clone() { - getFieldBuilder(0); // Force lastField to be built. - Map descendingFields = null; - descendingFields = - Collections.unmodifiableMap(((TreeMap) fields).descendingMap()); - return UnknownFieldSet.newBuilder().mergeFrom(new UnknownFieldSet(fields, descendingFields)); + Builder clone = UnknownFieldSet.newBuilder(); + for (Map.Entry entry : fieldBuilders.entrySet()) { + Integer key = entry.getKey(); + Field.Builder value = entry.getValue(); + clone.fieldBuilders.put(key, value.clone()); + } + return clone; } @Override @@ -390,31 +367,24 @@ public UnknownFieldSet getDefaultInstanceForType() { return UnknownFieldSet.getDefaultInstance(); } - private void reinitialize() { - fields = Collections.emptyMap(); - lastFieldNumber = 0; - lastField = null; - } - /** Reset the builder to an empty set. */ @Override public Builder clear() { - reinitialize(); + fieldBuilders = new TreeMap<>(); return this; } - /** Clear fields from the set with a given field number. */ - public Builder clearField(final int number) { - if (number == 0) { - throw new IllegalArgumentException("Zero is not a valid field number."); - } - if (lastField != null && lastFieldNumber == number) { - // Discard this. - lastField = null; - lastFieldNumber = 0; + /** + * Clear fields from the set with a given field number. + * + * @throws IllegalArgumentException if number is not positive + */ + public Builder clearField(int number) { + if (number <= 0) { + throw new IllegalArgumentException(number + " is not a valid field number."); } - if (fields.containsKey(number)) { - fields.remove(number); + if (fieldBuilders.containsKey(number)) { + fieldBuilders.remove(number); } return this; } @@ -423,9 +393,9 @@ public Builder clearField(final int number) { * Merge the fields from {@code other} into this set. If a field number exists in both sets, * {@code other}'s values for that field will be appended to the values in this set. */ - public Builder mergeFrom(final UnknownFieldSet other) { + public Builder mergeFrom(UnknownFieldSet other) { if (other != getDefaultInstance()) { - for (final Map.Entry entry : other.fields.entrySet()) { + for (Map.Entry entry : other.fields.entrySet()) { mergeField(entry.getKey(), entry.getValue()); } } @@ -435,10 +405,12 @@ public Builder mergeFrom(final UnknownFieldSet other) { /** * Add a field to the {@code UnknownFieldSet}. If a field with the same number already exists, * the two are merged. + * + * @throws IllegalArgumentException if number is not positive */ - public Builder mergeField(final int number, final Field field) { - if (number == 0) { - throw new IllegalArgumentException("Zero is not a valid field number."); + public Builder mergeField(int number, final Field field) { + if (number <= 0) { + throw new IllegalArgumentException(number + " is not a valid field number."); } if (hasField(number)) { getFieldBuilder(number).mergeFrom(field); @@ -454,10 +426,12 @@ public Builder mergeField(final int number, final Field field) { /** * Convenience method for merging a new field containing a single varint value. This is used in * particular when an unknown enum value is encountered. + * + * @throws IllegalArgumentException if number is not positive */ - public Builder mergeVarintField(final int number, final int value) { - if (number == 0) { - throw new IllegalArgumentException("Zero is not a valid field number."); + public Builder mergeVarintField(int number, int value) { + if (number <= 0) { + throw new IllegalArgumentException(number + " is not a valid field number."); } getFieldBuilder(number).addVarint(value); return this; @@ -467,40 +441,33 @@ public Builder mergeVarintField(final int number, final int value) { * Convenience method for merging a length-delimited field. * *

For use by generated code only. + * + * @throws IllegalArgumentException if number is not positive */ - public Builder mergeLengthDelimitedField(final int number, final ByteString value) { - if (number == 0) { - throw new IllegalArgumentException("Zero is not a valid field number."); + public Builder mergeLengthDelimitedField(int number, ByteString value) { + if (number <= 0) { + throw new IllegalArgumentException(number + " is not a valid field number."); } getFieldBuilder(number).addLengthDelimited(value); return this; } /** Check if the given field number is present in the set. */ - public boolean hasField(final int number) { - if (number == 0) { - throw new IllegalArgumentException("Zero is not a valid field number."); - } - return number == lastFieldNumber || fields.containsKey(number); + public boolean hasField(int number) { + return fieldBuilders.containsKey(number); } /** * Add a field to the {@code UnknownFieldSet}. If a field with the same number already exists, * it is removed. + * + * @throws IllegalArgumentException if number is not positive */ - public Builder addField(final int number, final Field field) { - if (number == 0) { - throw new IllegalArgumentException("Zero is not a valid field number."); - } - if (lastField != null && lastFieldNumber == number) { - // Discard this. - lastField = null; - lastFieldNumber = 0; + public Builder addField(int number, Field field) { + if (number <= 0) { + throw new IllegalArgumentException(number + " is not a valid field number."); } - if (fields.isEmpty()) { - fields = new TreeMap(); - } - fields.put(number, field); + fieldBuilders.put(number, Field.newBuilder(field)); return this; } @@ -509,15 +476,18 @@ public Builder addField(final int number, final Field field) { * changes may or may not be reflected in this map. */ public Map asMap() { - getFieldBuilder(0); // Force lastField to be built. + TreeMap fields = new TreeMap<>(); + for (Map.Entry entry : fieldBuilders.entrySet()) { + fields.put(entry.getKey(), entry.getValue().build()); + } return Collections.unmodifiableMap(fields); } /** Parse an entire message from {@code input} and merge its fields into this set. */ @Override - public Builder mergeFrom(final CodedInputStream input) throws IOException { + public Builder mergeFrom(CodedInputStream input) throws IOException { while (true) { - final int tag = input.readTag(); + int tag = input.readTag(); if (tag == 0 || !mergeFieldFrom(tag, input)) { break; } @@ -531,8 +501,8 @@ public Builder mergeFrom(final CodedInputStream input) throws IOException { * @param tag The field's tag number, which was already parsed. * @return {@code false} if the tag is an end group tag. */ - public boolean mergeFieldFrom(final int tag, final CodedInputStream input) throws IOException { - final int number = WireFormat.getTagFieldNumber(tag); + public boolean mergeFieldFrom(int tag, CodedInputStream input) throws IOException { + int number = WireFormat.getTagFieldNumber(tag); switch (WireFormat.getTagWireType(tag)) { case WireFormat.WIRETYPE_VARINT: getFieldBuilder(number).addVarint(input.readInt64()); @@ -544,7 +514,7 @@ public boolean mergeFieldFrom(final int tag, final CodedInputStream input) throw getFieldBuilder(number).addLengthDelimited(input.readBytes()); return true; case WireFormat.WIRETYPE_START_GROUP: - final Builder subBuilder = newBuilder(); + Builder subBuilder = newBuilder(); input.readGroup(number, subBuilder, ExtensionRegistry.getEmptyRegistry()); getFieldBuilder(number).addGroup(subBuilder.build()); return true; @@ -563,15 +533,15 @@ public boolean mergeFieldFrom(final int tag, final CodedInputStream input) throw * is just a small wrapper around {@link #mergeFrom(CodedInputStream)}. */ @Override - public Builder mergeFrom(final ByteString data) throws InvalidProtocolBufferException { + public Builder mergeFrom(ByteString data) throws InvalidProtocolBufferException { try { - final CodedInputStream input = data.newCodedInput(); + CodedInputStream input = data.newCodedInput(); mergeFrom(input); input.checkLastTagWas(0); return this; - } catch (final InvalidProtocolBufferException e) { + } catch (InvalidProtocolBufferException e) { throw e; - } catch (final IOException e) { + } catch (IOException e) { throw new RuntimeException( "Reading from a ByteString threw an IOException (should never happen).", e); } @@ -582,15 +552,15 @@ public Builder mergeFrom(final ByteString data) throws InvalidProtocolBufferExce * is just a small wrapper around {@link #mergeFrom(CodedInputStream)}. */ @Override - public Builder mergeFrom(final byte[] data) throws InvalidProtocolBufferException { + public Builder mergeFrom(byte[] data) throws InvalidProtocolBufferException { try { - final CodedInputStream input = CodedInputStream.newInstance(data); + CodedInputStream input = CodedInputStream.newInstance(data); mergeFrom(input); input.checkLastTagWas(0); return this; - } catch (final InvalidProtocolBufferException e) { + } catch (InvalidProtocolBufferException e) { throw e; - } catch (final IOException e) { + } catch (IOException e) { throw new RuntimeException( "Reading from a byte array threw an IOException (should never happen).", e); } @@ -601,8 +571,8 @@ public Builder mergeFrom(final byte[] data) throws InvalidProtocolBufferExceptio * This is just a small wrapper around {@link #mergeFrom(CodedInputStream)}. */ @Override - public Builder mergeFrom(final InputStream input) throws IOException { - final CodedInputStream codedInput = CodedInputStream.newInstance(input); + public Builder mergeFrom(InputStream input) throws IOException { + CodedInputStream codedInput = CodedInputStream.newInstance(input); mergeFrom(codedInput); codedInput.checkLastTagWas(0); return this; @@ -610,12 +580,12 @@ public Builder mergeFrom(final InputStream input) throws IOException { @Override public boolean mergeDelimitedFrom(InputStream input) throws IOException { - final int firstByte = input.read(); + int firstByte = input.read(); if (firstByte == -1) { return false; } - final int size = CodedInputStream.readRawVarint32(firstByte, input); - final InputStream limitedInput = new LimitedInputStream(input, size); + int size = CodedInputStream.readRawVarint32(firstByte, input); + InputStream limitedInput = new LimitedInputStream(input, size); mergeFrom(limitedInput); return true; } @@ -644,7 +614,7 @@ public Builder mergeFrom(ByteString data, ExtensionRegistryLite extensionRegistr @Override public Builder mergeFrom(byte[] data, int off, int len) throws InvalidProtocolBufferException { try { - final CodedInputStream input = CodedInputStream.newInstance(data, off, len); + CodedInputStream input = CodedInputStream.newInstance(data, off, len); mergeFrom(input); input.checkLastTagWas(0); return this; @@ -718,7 +688,7 @@ public static Builder newBuilder() { } /** Construct a new {@link Builder} and initialize it to a copy of {@code copyFrom}. */ - public static Builder newBuilder(final Field copyFrom) { + public static Builder newBuilder(Field copyFrom) { return newBuilder().mergeFrom(copyFrom); } @@ -758,7 +728,7 @@ public List getGroupList() { } @Override - public boolean equals(final Object other) { + public boolean equals(Object other) { if (this == other) { return true; } @@ -785,7 +755,7 @@ private Object[] getIdentityArray() { public ByteString toByteString(int fieldNumber) { try { // TODO(lukes): consider caching serialized size in a volatile long - final ByteString.CodedBuilder out = + ByteString.CodedBuilder out = ByteString.newCodedBuilder(getSerializedSize(fieldNumber)); writeTo(fieldNumber, out.getCodedOutput()); return out.build(); @@ -796,40 +766,40 @@ public ByteString toByteString(int fieldNumber) { } /** Serializes the field, including field number, and writes it to {@code output}. */ - public void writeTo(final int fieldNumber, final CodedOutputStream output) throws IOException { - for (final long value : varint) { + public void writeTo(int fieldNumber, CodedOutputStream output) throws IOException { + for (long value : varint) { output.writeUInt64(fieldNumber, value); } - for (final int value : fixed32) { + for (int value : fixed32) { output.writeFixed32(fieldNumber, value); } - for (final long value : fixed64) { + for (long value : fixed64) { output.writeFixed64(fieldNumber, value); } - for (final ByteString value : lengthDelimited) { + for (ByteString value : lengthDelimited) { output.writeBytes(fieldNumber, value); } - for (final UnknownFieldSet value : group) { + for (UnknownFieldSet value : group) { output.writeGroup(fieldNumber, value); } } /** Get the number of bytes required to encode this field, including field number. */ - public int getSerializedSize(final int fieldNumber) { + public int getSerializedSize(int fieldNumber) { int result = 0; - for (final long value : varint) { + for (long value : varint) { result += CodedOutputStream.computeUInt64Size(fieldNumber, value); } - for (final int value : fixed32) { + for (int value : fixed32) { result += CodedOutputStream.computeFixed32Size(fieldNumber, value); } - for (final long value : fixed64) { + for (long value : fixed64) { result += CodedOutputStream.computeFixed64Size(fieldNumber, value); } - for (final ByteString value : lengthDelimited) { + for (ByteString value : lengthDelimited) { result += CodedOutputStream.computeBytesSize(fieldNumber, value); } - for (final UnknownFieldSet value : group) { + for (UnknownFieldSet value : group) { result += CodedOutputStream.computeGroupSize(fieldNumber, value); } return result; @@ -839,15 +809,15 @@ public int getSerializedSize(final int fieldNumber) { * Serializes the field, including field number, and writes it to {@code output}, using {@code * MessageSet} wire format. */ - public void writeAsMessageSetExtensionTo(final int fieldNumber, final CodedOutputStream output) + public void writeAsMessageSetExtensionTo(int fieldNumber, CodedOutputStream output) throws IOException { - for (final ByteString value : lengthDelimited) { + for (ByteString value : lengthDelimited) { output.writeRawMessageSetExtension(fieldNumber, value); } } /** Serializes the field, including field number, and writes it to {@code writer}. */ - void writeTo(final int fieldNumber, final Writer writer) throws IOException { + void writeTo(int fieldNumber, Writer writer) throws IOException { writer.writeInt64List(fieldNumber, varint, false); writer.writeFixed32List(fieldNumber, fixed32, false); writer.writeFixed64List(fieldNumber, fixed64, false); @@ -872,7 +842,7 @@ void writeTo(final int fieldNumber, final Writer writer) throws IOException { * Serializes the field, including field number, and writes it to {@code writer}, using {@code * MessageSet} wire format. */ - private void writeAsMessageSetExtensionTo(final int fieldNumber, final Writer writer) + private void writeAsMessageSetExtensionTo(int fieldNumber, Writer writer) throws IOException { if (writer.fieldOrder() == Writer.FieldOrder.DESCENDING) { // Write in descending field order. @@ -882,7 +852,7 @@ private void writeAsMessageSetExtensionTo(final int fieldNumber, final Writer wr } } else { // Write in ascending field order. - for (final ByteString value : lengthDelimited) { + for (ByteString value : lengthDelimited) { writer.writeMessageSetItem(fieldNumber, value); } } @@ -892,9 +862,9 @@ private void writeAsMessageSetExtensionTo(final int fieldNumber, final Writer wr * Get the number of bytes required to encode this field, including field number, using {@code * MessageSet} wire format. */ - public int getSerializedSizeAsMessageSetExtension(final int fieldNumber) { + public int getSerializedSizeAsMessageSetExtension(int fieldNumber) { int result = 0; - for (final ByteString value : lengthDelimited) { + for (ByteString value : lengthDelimited) { result += CodedOutputStream.computeRawMessageSetExtensionSize(fieldNumber, value); } return result; @@ -912,52 +882,85 @@ public int getSerializedSizeAsMessageSetExtension(final int fieldNumber) { *

Use {@link Field#newBuilder()} to construct a {@code Builder}. */ public static final class Builder { - // This constructor should never be called directly (except from 'create'). - private Builder() {} + // This constructor should only be called directly from 'create' and 'clone'. + private Builder() { + result = new Field(); + } private static Builder create() { Builder builder = new Builder(); - builder.result = new Field(); return builder; } private Field result; + @Override + public Builder clone() { + Field copy = new Field(); + if (result.varint == null) { + copy.varint = null; + } else { + copy.varint = new ArrayList<>(result.varint); + } + if (result.fixed32 == null) { + copy.fixed32 = null; + } else { + copy.fixed32 = new ArrayList<>(result.fixed32); + } + if (result.fixed64 == null) { + copy.fixed64 = null; + } else { + copy.fixed64 = new ArrayList<>(result.fixed64); + } + if (result.lengthDelimited == null) { + copy.lengthDelimited = null; + } else { + copy.lengthDelimited = new ArrayList<>(result.lengthDelimited); + } + if (result.group == null) { + copy.group = null; + } else { + copy.group = new ArrayList<>(result.group); + } + + Builder clone = new Builder(); + clone.result = copy; + return clone; + } + /** - * Build the field. After {@code build()} has been called, the {@code Builder} is no longer - * usable. Calling any other method will result in undefined behavior and can cause a {@code - * NullPointerException} to be thrown. + * Build the field. */ public Field build() { + Field built = new Field(); if (result.varint == null) { - result.varint = Collections.emptyList(); + built.varint = Collections.emptyList(); } else { - result.varint = Collections.unmodifiableList(result.varint); + built.varint = Collections.unmodifiableList(new ArrayList<>(result.varint)); } if (result.fixed32 == null) { - result.fixed32 = Collections.emptyList(); + built.fixed32 = Collections.emptyList(); } else { - result.fixed32 = Collections.unmodifiableList(result.fixed32); + built.fixed32 = Collections.unmodifiableList(new ArrayList<>(result.fixed32)); } if (result.fixed64 == null) { - result.fixed64 = Collections.emptyList(); + built.fixed64 = Collections.emptyList(); } else { - result.fixed64 = Collections.unmodifiableList(result.fixed64); + built.fixed64 = Collections.unmodifiableList(new ArrayList<>(result.fixed64)); } if (result.lengthDelimited == null) { - result.lengthDelimited = Collections.emptyList(); + built.lengthDelimited = Collections.emptyList(); } else { - result.lengthDelimited = Collections.unmodifiableList(result.lengthDelimited); + built.lengthDelimited = Collections.unmodifiableList( + new ArrayList<>(result.lengthDelimited)); } if (result.group == null) { - result.group = Collections.emptyList(); + built.group = Collections.emptyList(); } else { - result.group = Collections.unmodifiableList(result.group); + built.group = Collections.unmodifiableList(new ArrayList<>(result.group)); } - final Field returnMe = result; - result = null; - return returnMe; + return built; } /** Discard the field's contents. */ @@ -970,7 +973,7 @@ public Builder clear() { * Merge the values in {@code other} into this field. For each list of values, {@code other}'s * values are append to the ones in this field. */ - public Builder mergeFrom(final Field other) { + public Builder mergeFrom(Field other) { if (!other.varint.isEmpty()) { if (result.varint == null) { result.varint = new ArrayList(); @@ -985,19 +988,19 @@ public Builder mergeFrom(final Field other) { } if (!other.fixed64.isEmpty()) { if (result.fixed64 == null) { - result.fixed64 = new ArrayList(); + result.fixed64 = new ArrayList<>(); } result.fixed64.addAll(other.fixed64); } if (!other.lengthDelimited.isEmpty()) { if (result.lengthDelimited == null) { - result.lengthDelimited = new ArrayList(); + result.lengthDelimited = new ArrayList<>(); } result.lengthDelimited.addAll(other.lengthDelimited); } if (!other.group.isEmpty()) { if (result.group == null) { - result.group = new ArrayList(); + result.group = new ArrayList<>(); } result.group.addAll(other.group); } @@ -1005,45 +1008,45 @@ public Builder mergeFrom(final Field other) { } /** Add a varint value. */ - public Builder addVarint(final long value) { + public Builder addVarint(long value) { if (result.varint == null) { - result.varint = new ArrayList(); + result.varint = new ArrayList<>(); } result.varint.add(value); return this; } /** Add a fixed32 value. */ - public Builder addFixed32(final int value) { + public Builder addFixed32(int value) { if (result.fixed32 == null) { - result.fixed32 = new ArrayList(); + result.fixed32 = new ArrayList<>(); } result.fixed32.add(value); return this; } /** Add a fixed64 value. */ - public Builder addFixed64(final long value) { + public Builder addFixed64(long value) { if (result.fixed64 == null) { - result.fixed64 = new ArrayList(); + result.fixed64 = new ArrayList<>(); } result.fixed64.add(value); return this; } /** Add a length-delimited value. */ - public Builder addLengthDelimited(final ByteString value) { + public Builder addLengthDelimited(ByteString value) { if (result.lengthDelimited == null) { - result.lengthDelimited = new ArrayList(); + result.lengthDelimited = new ArrayList<>(); } result.lengthDelimited.add(value); return this; } /** Add an embedded group. */ - public Builder addGroup(final UnknownFieldSet value) { + public Builder addGroup(UnknownFieldSet value) { if (result.group == null) { - result.group = new ArrayList(); + result.group = new ArrayList<>(); } result.group.add(value); return this; diff --git a/java/core/src/test/java/com/google/protobuf/UnknownFieldSetPerformanceTest.java b/java/core/src/test/java/com/google/protobuf/UnknownFieldSetPerformanceTest.java new file mode 100644 index 000000000000..6ce0fc7e348b --- /dev/null +++ b/java/core/src/test/java/com/google/protobuf/UnknownFieldSetPerformanceTest.java @@ -0,0 +1,78 @@ +// Protocol Buffers - Google's data interchange format +// Copyright 2008 Google Inc. All rights reserved. +// https://developers.google.com/protocol-buffers/ +// +// 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. + +package com.google.protobuf; + +import static com.google.common.truth.Truth.assertThat; + +import java.io.ByteArrayInputStream; +import java.io.IOException; +import java.io.InputStream; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; + +@RunWith(JUnit4.class) +public final class UnknownFieldSetPerformanceTest { + + private static byte[] generateBytes(int length) { + assertThat(length % 4).isEqualTo(0); + byte[] input = new byte[length]; + for (int i = 0; i < length; i += 4) { + input[i] = (byte) 0x08; // field 1, wiretype 0 + input[i + 1] = (byte) 0x08; // field 1, payload 8 + input[i + 2] = (byte) 0x20; // field 4, wiretype 0 + input[i + 3] = (byte) 0x20; // field 4, payload 32 + } + return input; + } + + @Test + // This is a performance test. Failure here is a timeout. + public void testAlternatingFieldNumbers() throws IOException { + byte[] input = generateBytes(800000); + InputStream in = new ByteArrayInputStream(input); + UnknownFieldSet.Builder builder = UnknownFieldSet.newBuilder(); + CodedInputStream codedInput = CodedInputStream.newInstance(in); + builder.mergeFrom(codedInput); + } + + @Test + // This is a performance test. Failure here is a timeout. + public void testAddField() { + UnknownFieldSet.Builder builder = UnknownFieldSet.newBuilder(); + for (int i = 1; i <= 100000; i++) { + UnknownFieldSet.Field field = UnknownFieldSet.Field.newBuilder().addFixed32(i).build(); + builder.addField(i, field); + } + UnknownFieldSet fieldSet = builder.build(); + assertThat(fieldSet.getField(100000).getFixed32List().get(0)).isEqualTo(100000); + } +} diff --git a/java/core/src/test/java/com/google/protobuf/UnknownFieldSetTest.java b/java/core/src/test/java/com/google/protobuf/UnknownFieldSetTest.java index c7eb57c63d4c..f022592b7403 100644 --- a/java/core/src/test/java/com/google/protobuf/UnknownFieldSetTest.java +++ b/java/core/src/test/java/com/google/protobuf/UnknownFieldSetTest.java @@ -40,8 +40,11 @@ import protobuf_unittest.UnittestProto.TestPackedTypes; import proto3_unittest.UnittestProto3; import java.util.Arrays; +import java.util.List; import java.util.Map; import junit.framework.TestCase; +import org.junit.Assert; +import org.junit.Test; /** * Tests related to unknown field handling. @@ -58,7 +61,7 @@ public void setUp() throws Exception { unknownFields = emptyMessage.getUnknownFields(); } - UnknownFieldSet.Field getField(String name) { + private UnknownFieldSet.Field getField(String name) { Descriptors.FieldDescriptor field = descriptor.findFieldByName(name); assertNotNull(field); return unknownFields.getField(field.getNumber()); @@ -173,6 +176,17 @@ public void testMergeFrom() throws Exception { assertEquals("1: 1\n2: 2\n3: 3\n3: 4\n", destination.toString()); } + @Test + public void testAsMap() throws Exception { + UnknownFieldSet.Builder builder = UnknownFieldSet.newBuilder().mergeFrom(unknownFields); + Map mapFromBuilder = builder.asMap(); + assertThat(mapFromBuilder).isNotEmpty(); + UnknownFieldSet fields = builder.build(); + Map mapFromFieldSet = fields.asMap(); + assertThat(mapFromFieldSet).containsExactlyEntriesIn(mapFromBuilder); + } + + @Test public void testClear() throws Exception { UnknownFieldSet fields = UnknownFieldSet.newBuilder().mergeFrom(unknownFields).clear().build(); assertTrue(fields.asMap().isEmpty()); diff --git a/java/lite/pom.xml b/java/lite/pom.xml index 821266a3db5e..0a7166afe232 100644 --- a/java/lite/pom.xml +++ b/java/lite/pom.xml @@ -234,6 +234,7 @@ TypeRegistryTest.java UnknownEnumValueTest.java UnknownFieldSetLiteTest.java + UnknownFieldSetPerformanceTest.java UnknownFieldSetTest.java WellKnownTypesTest.java WireFormatTest.java