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.
diff --git a/java/core/BUILD b/java/core/BUILD
index 20bf589..aa7ba62 100644
--- a/java/core/BUILD
+++ b/java/core/BUILD
@@ -239,6 +239,7 @@
"conformance_test",
"core_build_test",
"core_tests",
+ "utf8_tests",
],
)
@@ -258,10 +259,12 @@
junit_tests(
name = "core_tests",
- size = "large",
+ size = "small",
srcs = glob(
["src/test/java/**/*.java"],
exclude = [
+ "src/test/java/com/google/protobuf/DecodeUtf8Test.java",
+ "src/test/java/com/google/protobuf/IsValidUtf8Test.java",
"src/test/java/com/google/protobuf/TestUtil.java",
"src/test/java/com/google/protobuf/TestUtilLite.java",
],
@@ -279,6 +282,24 @@
],
)
+# The UTF-8 validation tests are much slower than the other tests, so they get
+# their own test target with a longer timeout.
+junit_tests(
+ name = "utf8_tests",
+ size = "large",
+ srcs = [
+ "src/test/java/com/google/protobuf/DecodeUtf8Test.java",
+ "src/test/java/com/google/protobuf/IsValidUtf8Test.java",
+ "src/test/java/com/google/protobuf/IsValidUtf8TestUtil.java",
+ ],
+ deps = [
+ ":core",
+ "@maven//:com_google_guava_guava",
+ "@maven//:com_google_truth_truth",
+ "@maven//:junit_junit",
+ ],
+)
+
java_lite_proto_library(
name = "generic_test_protos_java_proto_lite",
visibility = [
@@ -361,6 +382,7 @@
"src/test/java/com/google/protobuf/TypeRegistryTest.java",
"src/test/java/com/google/protobuf/UnknownEnumValueTest.java",
"src/test/java/com/google/protobuf/UnknownFieldSetLiteTest.java",
+ "src/test/java/com/google/protobuf/UnknownFieldSetPerformanceTest.java",
"src/test/java/com/google/protobuf/UnknownFieldSetTest.java",
"src/test/java/com/google/protobuf/WellKnownTypesTest.java",
"src/test/java/com/google/protobuf/WireFormatTest.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 ba2f9df..5c482d6 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.
*
* <p>Every {@link Message} contains an {@code UnknownFieldSet} (and every {@link Message.Builder}
- * contains an {@link Builder}).
+ * contains a {@link Builder}).
*
* <p>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<Integer, Field> fields;
+
+ /**
+ * Construct an {@code UnknownFieldSet} around the given map.
+ */
+ UnknownFieldSet(TreeMap<Integer, Field> fields) {
+ this.fields = fields;
}
/** Create a new {@link Builder}. */
@@ -68,7 +72,7 @@
}
/** 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 @@
}
private static final UnknownFieldSet defaultInstance =
- new UnknownFieldSet(
- Collections.<Integer, Field>emptyMap(), Collections.<Integer, Field>emptyMap());
-
- /**
- * Construct an {@code UnknownFieldSet} around the given map. The map is expected to be immutable.
- */
- UnknownFieldSet(final Map<Integer, Field> fields, final Map<Integer, Field> fieldsDescending) {
- this.fields = fields;
- this.fieldsDescending = fieldsDescending;
- }
-
- private final Map<Integer, Field> fields;
-
- /** A copy of {@link #fields} who's iterator order is reversed. */
- private final Map<Integer, Field> fieldsDescending;
+ new UnknownFieldSet(new TreeMap<Integer, Field>());
@Override
- public boolean equals(final Object other) {
+ public boolean equals(Object other) {
if (this == other) {
return true;
}
@@ -110,29 +100,33 @@
@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<Integer, Field> asMap() {
- return fields;
+ return (Map<Integer, Field>) 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<Integer, Field> entry : fields.entrySet()) {
+ public void writeTo(CodedOutputStream output) throws IOException {
+ for (Map.Entry<Integer, Field> entry : fields.entrySet()) {
Field field = entry.getValue();
field.writeTo(entry.getKey(), output);
}
@@ -154,10 +148,10 @@
@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 @@
@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 @@
* {@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 @@
@Override
public int getSerializedSize() {
int result = 0;
- for (final Map.Entry<Integer, Field> entry : fields.entrySet()) {
- result += entry.getValue().getSerializedSize(entry.getKey());
+ if (!fields.isEmpty()) {
+ for (Map.Entry<Integer, Field> 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<Integer, Field> entry : fields.entrySet()) {
+ public void writeAsMessageSetTo(CodedOutputStream output) throws IOException {
+ for (Map.Entry<Integer, Field> entry : fields.entrySet()) {
entry.getValue().writeAsMessageSetExtensionTo(entry.getKey(), output);
}
}
@@ -221,7 +217,7 @@
void writeTo(Writer writer) throws IOException {
if (writer.fieldOrder() == Writer.FieldOrder.DESCENDING) {
// Write fields in descending order.
- for (Map.Entry<Integer, Field> entry : fieldsDescending.entrySet()) {
+ for (Map.Entry<Integer, Field> entry : fields.descendingMap().entrySet()) {
entry.getValue().writeTo(entry.getKey(), writer);
}
} else {
@@ -233,15 +229,15 @@
}
/** 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<Integer, Field> entry : fieldsDescending.entrySet()) {
+ for (Map.Entry<Integer, Field> entry : fields.descendingMap().entrySet()) {
entry.getValue().writeAsMessageSetExtensionTo(entry.getKey(), writer);
}
} else {
// Write fields in ascending order.
- for (final Map.Entry<Integer, Field> entry : fields.entrySet()) {
+ for (Map.Entry<Integer, Field> entry : fields.entrySet()) {
entry.getValue().writeAsMessageSetExtensionTo(entry.getKey(), writer);
}
}
@@ -250,7 +246,7 @@
/** 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<Integer, Field> entry : fields.entrySet()) {
+ for (Map.Entry<Integer, Field> entry : fields.entrySet()) {
result += entry.getValue().getSerializedSizeAsMessageSetExtension(entry.getKey());
}
return result;
@@ -264,23 +260,23 @@
}
/** 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 @@
// This constructor should never be called directly (except from 'create').
private Builder() {}
- private Map<Integer, Field> 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<Integer, Field.Builder> 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.
- *
- * <p>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<Integer, Field> descendingFields = null;
- descendingFields =
- Collections.unmodifiableMap(((TreeMap<Integer, Field>) fields).descendingMap());
- result = new UnknownFieldSet(Collections.unmodifiableMap(fields), descendingFields);
+ TreeMap<Integer, Field> fields = new TreeMap<>();
+ for (Map.Entry<Integer, Field.Builder> entry : fieldBuilders.entrySet()) {
+ fields.put(entry.getKey(), entry.getValue().build());
+ }
+ result = new UnknownFieldSet(fields);
}
- fields = null;
return result;
}
@@ -378,11 +353,13 @@
@Override
public Builder clone() {
- getFieldBuilder(0); // Force lastField to be built.
- Map<Integer, Field> descendingFields = null;
- descendingFields =
- Collections.unmodifiableMap(((TreeMap<Integer, Field>) fields).descendingMap());
- return UnknownFieldSet.newBuilder().mergeFrom(new UnknownFieldSet(fields, descendingFields));
+ Builder clone = UnknownFieldSet.newBuilder();
+ for (Map.Entry<Integer, Field.Builder> 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 @@
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.");
+ /**
+ * 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 (lastField != null && lastFieldNumber == number) {
- // Discard this.
- lastField = null;
- lastFieldNumber = 0;
- }
- if (fields.containsKey(number)) {
- fields.remove(number);
+ if (fieldBuilders.containsKey(number)) {
+ fieldBuilders.remove(number);
}
return this;
}
@@ -423,9 +393,9 @@
* 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<Integer, Field> entry : other.fields.entrySet()) {
+ for (Map.Entry<Integer, Field> entry : other.fields.entrySet()) {
mergeField(entry.getKey(), entry.getValue());
}
}
@@ -435,10 +405,12 @@
/**
* 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 @@
/**
* 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 @@
* Convenience method for merging a length-delimited field.
*
* <p>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.");
+ public Builder addField(int number, Field field) {
+ if (number <= 0) {
+ throw new IllegalArgumentException(number + " is not a valid field number.");
}
- if (lastField != null && lastFieldNumber == number) {
- // Discard this.
- lastField = null;
- lastFieldNumber = 0;
- }
- if (fields.isEmpty()) {
- fields = new TreeMap<Integer, Field>();
- }
- fields.put(number, field);
+ fieldBuilders.put(number, Field.newBuilder(field));
return this;
}
@@ -509,15 +476,18 @@
* changes may or may not be reflected in this map.
*/
public Map<Integer, Field> asMap() {
- getFieldBuilder(0); // Force lastField to be built.
+ TreeMap<Integer, Field> fields = new TreeMap<>();
+ for (Map.Entry<Integer, Field.Builder> 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 @@
* @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 @@
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 @@
* 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 @@
* 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 @@
* 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 @@
@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 @@
@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 @@
}
/** 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 @@
}
@Override
- public boolean equals(final Object other) {
+ public boolean equals(Object other) {
if (this == other) {
return true;
}
@@ -785,7 +755,7 @@
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 @@
}
/** 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 @@
* 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 @@
* 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 @@
}
} else {
// Write in ascending field order.
- for (final ByteString value : lengthDelimited) {
+ for (ByteString value : lengthDelimited) {
writer.writeMessageSetItem(fieldNumber, value);
}
}
@@ -892,9 +862,9 @@
* 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 @@
* <p>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;
- /**
- * 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.
- */
- public Field build() {
+ @Override
+ public Builder clone() {
+ Field copy = new Field();
if (result.varint == null) {
- result.varint = Collections.emptyList();
+ copy.varint = null;
} else {
- result.varint = Collections.unmodifiableList(result.varint);
+ copy.varint = new ArrayList<>(result.varint);
}
if (result.fixed32 == null) {
- result.fixed32 = Collections.emptyList();
+ copy.fixed32 = null;
} else {
- result.fixed32 = Collections.unmodifiableList(result.fixed32);
+ copy.fixed32 = new ArrayList<>(result.fixed32);
}
if (result.fixed64 == null) {
- result.fixed64 = Collections.emptyList();
+ copy.fixed64 = null;
} else {
- result.fixed64 = Collections.unmodifiableList(result.fixed64);
+ copy.fixed64 = new ArrayList<>(result.fixed64);
}
if (result.lengthDelimited == null) {
- result.lengthDelimited = Collections.emptyList();
+ copy.lengthDelimited = null;
} else {
- result.lengthDelimited = Collections.unmodifiableList(result.lengthDelimited);
+ copy.lengthDelimited = new ArrayList<>(result.lengthDelimited);
}
if (result.group == null) {
- result.group = Collections.emptyList();
+ copy.group = null;
} else {
- result.group = Collections.unmodifiableList(result.group);
+ copy.group = new ArrayList<>(result.group);
}
- final Field returnMe = result;
- result = null;
- return returnMe;
+ Builder clone = new Builder();
+ clone.result = copy;
+ return clone;
+ }
+
+ /**
+ * Build the field.
+ */
+ public Field build() {
+ Field built = new Field();
+ if (result.varint == null) {
+ built.varint = Collections.emptyList();
+ } else {
+ built.varint = Collections.unmodifiableList(new ArrayList<>(result.varint));
+ }
+ if (result.fixed32 == null) {
+ built.fixed32 = Collections.emptyList();
+ } else {
+ built.fixed32 = Collections.unmodifiableList(new ArrayList<>(result.fixed32));
+ }
+ if (result.fixed64 == null) {
+ built.fixed64 = Collections.emptyList();
+ } else {
+ built.fixed64 = Collections.unmodifiableList(new ArrayList<>(result.fixed64));
+ }
+ if (result.lengthDelimited == null) {
+ built.lengthDelimited = Collections.emptyList();
+ } else {
+ built.lengthDelimited = Collections.unmodifiableList(
+ new ArrayList<>(result.lengthDelimited));
+ }
+ if (result.group == null) {
+ built.group = Collections.emptyList();
+ } else {
+ built.group = Collections.unmodifiableList(new ArrayList<>(result.group));
+ }
+
+ return built;
}
/** Discard the field's contents. */
@@ -970,7 +973,7 @@
* 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<Long>();
@@ -985,19 +988,19 @@
}
if (!other.fixed64.isEmpty()) {
if (result.fixed64 == null) {
- result.fixed64 = new ArrayList<Long>();
+ result.fixed64 = new ArrayList<>();
}
result.fixed64.addAll(other.fixed64);
}
if (!other.lengthDelimited.isEmpty()) {
if (result.lengthDelimited == null) {
- result.lengthDelimited = new ArrayList<ByteString>();
+ result.lengthDelimited = new ArrayList<>();
}
result.lengthDelimited.addAll(other.lengthDelimited);
}
if (!other.group.isEmpty()) {
if (result.group == null) {
- result.group = new ArrayList<UnknownFieldSet>();
+ result.group = new ArrayList<>();
}
result.group.addAll(other.group);
}
@@ -1005,45 +1008,45 @@
}
/** Add a varint value. */
- public Builder addVarint(final long value) {
+ public Builder addVarint(long value) {
if (result.varint == null) {
- result.varint = new ArrayList<Long>();
+ 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<Integer>();
+ 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<Long>();
+ 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<ByteString>();
+ 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<UnknownFieldSet>();
+ 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 0000000..6ce0fc7
--- /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 1e5bc96..fbc3bb8 100644
--- a/java/core/src/test/java/com/google/protobuf/UnknownFieldSetTest.java
+++ b/java/core/src/test/java/com/google/protobuf/UnknownFieldSetTest.java
@@ -42,7 +42,9 @@
import protobuf_unittest.UnittestProto.TestPackedExtensions;
import protobuf_unittest.UnittestProto.TestPackedTypes;
import proto3_unittest.UnittestProto3;
+import java.util.List;
import java.util.Map;
+import org.junit.Assert;
import org.junit.Before;
import org.junit.Test;
import org.junit.runner.RunWith;
@@ -61,7 +63,7 @@
unknownFields = emptyMessage.getUnknownFields();
}
- UnknownFieldSet.Field getField(String name) {
+ private UnknownFieldSet.Field getField(String name) {
Descriptors.FieldDescriptor field = descriptor.findFieldByName(name);
assertThat(field).isNotNull();
return unknownFields.getField(field.getNumber());
@@ -101,6 +103,174 @@
// =================================================================
@Test
+ public void testFieldBuildersAreReusable() {
+ UnknownFieldSet.Field.Builder fieldBuilder = UnknownFieldSet.Field.newBuilder();
+ fieldBuilder.addFixed32(10);
+ UnknownFieldSet.Field first = fieldBuilder.build();
+ UnknownFieldSet.Field second = fieldBuilder.build();
+ fieldBuilder.addFixed32(11);
+ UnknownFieldSet.Field third = fieldBuilder.build();
+
+ assertThat(first).isEqualTo(second);
+ assertThat(first).isNotEqualTo(third);
+ }
+
+ @Test
+ public void testClone() {
+ UnknownFieldSet.Builder unknownSetBuilder = UnknownFieldSet.newBuilder();
+ UnknownFieldSet.Field.Builder fieldBuilder = UnknownFieldSet.Field.newBuilder();
+ fieldBuilder.addFixed32(10);
+ unknownSetBuilder.addField(8, fieldBuilder.build());
+ // necessary to call clone twice to expose the bug
+ UnknownFieldSet.Builder clone1 = unknownSetBuilder.clone();
+ UnknownFieldSet.Builder clone2 = unknownSetBuilder.clone(); // failure is a NullPointerException
+ assertThat(clone1).isNotSameInstanceAs(clone2);
+ }
+
+ @Test
+ public void testClone_lengthDelimited() {
+ UnknownFieldSet.Builder destUnknownFieldSet =
+ UnknownFieldSet.newBuilder()
+ .addField(997, UnknownFieldSet.Field.newBuilder().addVarint(99).build())
+ .addField(
+ 999,
+ UnknownFieldSet.Field.newBuilder()
+ .addLengthDelimited(ByteString.copyFromUtf8("some data"))
+ .addLengthDelimited(ByteString.copyFromUtf8("some more data"))
+ .build());
+ UnknownFieldSet clone = destUnknownFieldSet.clone().build();
+ assertThat(clone.getField(997)).isNotNull();
+ UnknownFieldSet.Field field999 = clone.getField(999);
+ List<ByteString> lengthDelimited = field999.getLengthDelimitedList();
+ assertThat(lengthDelimited.get(0).toStringUtf8()).isEqualTo("some data");
+ assertThat(lengthDelimited.get(1).toStringUtf8()).isEqualTo("some more data");
+
+ UnknownFieldSet clone2 = destUnknownFieldSet.clone().build();
+ assertThat(clone2.getField(997)).isNotNull();
+ UnknownFieldSet.Field secondField = clone2.getField(999);
+ List<ByteString> lengthDelimited2 = secondField.getLengthDelimitedList();
+ assertThat(lengthDelimited2.get(0).toStringUtf8()).isEqualTo("some data");
+ assertThat(lengthDelimited2.get(1).toStringUtf8()).isEqualTo("some more data");
+ }
+
+ @Test
+ public void testReuse() {
+ UnknownFieldSet.Builder builder =
+ UnknownFieldSet.newBuilder()
+ .addField(997, UnknownFieldSet.Field.newBuilder().addVarint(99).build())
+ .addField(
+ 999,
+ UnknownFieldSet.Field.newBuilder()
+ .addLengthDelimited(ByteString.copyFromUtf8("some data"))
+ .addLengthDelimited(ByteString.copyFromUtf8("some more data"))
+ .build());
+
+ UnknownFieldSet fieldSet1 = builder.build();
+ UnknownFieldSet fieldSet2 = builder.build();
+ builder.addField(1000, UnknownFieldSet.Field.newBuilder().addVarint(-90).build());
+ UnknownFieldSet fieldSet3 = builder.build();
+
+ assertThat(fieldSet1).isEqualTo(fieldSet2);
+ assertThat(fieldSet1).isNotEqualTo(fieldSet3);
+ }
+
+ @Test
+ @SuppressWarnings("ModifiedButNotUsed")
+ public void testAddField_zero() {
+ UnknownFieldSet.Field field = getField("optional_int32");
+ try {
+ UnknownFieldSet.newBuilder().addField(0, field);
+ Assert.fail();
+ } catch (IllegalArgumentException expected) {
+ assertThat(expected).hasMessageThat().isEqualTo("0 is not a valid field number.");
+ }
+ }
+
+ @Test
+ @SuppressWarnings("ModifiedButNotUsed")
+ public void testAddField_negative() {
+ UnknownFieldSet.Field field = getField("optional_int32");
+ try {
+ UnknownFieldSet.newBuilder().addField(-2, field);
+ Assert.fail();
+ } catch (IllegalArgumentException expected) {
+ assertThat(expected).hasMessageThat().isEqualTo("-2 is not a valid field number.");
+ }
+ }
+
+ @Test
+ @SuppressWarnings("ModifiedButNotUsed")
+ public void testClearField_negative() {
+ try {
+ UnknownFieldSet.newBuilder().clearField(-28);
+ Assert.fail();
+ } catch (IllegalArgumentException expected) {
+ assertThat(expected).hasMessageThat().isEqualTo("-28 is not a valid field number.");
+ }
+ }
+
+ @Test
+ @SuppressWarnings("ModifiedButNotUsed")
+ public void testMergeField_negative() {
+ UnknownFieldSet.Field field = getField("optional_int32");
+ try {
+ UnknownFieldSet.newBuilder().mergeField(-2, field);
+ Assert.fail();
+ } catch (IllegalArgumentException expected) {
+ assertThat(expected).hasMessageThat().isEqualTo("-2 is not a valid field number.");
+ }
+ }
+
+ @Test
+ @SuppressWarnings("ModifiedButNotUsed")
+ public void testMergeVarintField_negative() {
+ try {
+ UnknownFieldSet.newBuilder().mergeVarintField(-2, 78);
+ Assert.fail();
+ } catch (IllegalArgumentException expected) {
+ assertThat(expected).hasMessageThat().isEqualTo("-2 is not a valid field number.");
+ }
+ }
+
+ @Test
+ @SuppressWarnings("ModifiedButNotUsed")
+ public void testHasField_negative() {
+ assertThat(UnknownFieldSet.newBuilder().hasField(-2)).isFalse();
+ }
+
+ @Test
+ @SuppressWarnings("ModifiedButNotUsed")
+ public void testMergeLengthDelimitedField_negative() {
+ ByteString byteString = ByteString.copyFromUtf8("some data");
+ try {
+ UnknownFieldSet.newBuilder().mergeLengthDelimitedField(-2, byteString);
+ Assert.fail();
+ } catch (IllegalArgumentException expected) {
+ assertThat(expected).hasMessageThat().isEqualTo("-2 is not a valid field number.");
+ }
+ }
+
+ @Test
+ public void testAddField() {
+ UnknownFieldSet.Field field = getField("optional_int32");
+ UnknownFieldSet fieldSet = UnknownFieldSet.newBuilder().addField(1, field).build();
+ assertThat(fieldSet.getField(1)).isEqualTo(field);
+ }
+
+ @Test
+ public void testAddField_withReplacement() {
+ UnknownFieldSet.Field first = UnknownFieldSet.Field.newBuilder().addFixed32(56).build();
+ UnknownFieldSet.Field second = UnknownFieldSet.Field.newBuilder().addFixed32(25).build();
+ UnknownFieldSet fieldSet = UnknownFieldSet.newBuilder()
+ .addField(1, first)
+ .addField(1, second)
+ .build();
+ List<Integer> list = fieldSet.getField(1).getFixed32List();
+ assertThat(list).hasSize(1);
+ assertThat(list.get(0)).isEqualTo(25);
+ }
+
+ @Test
public void testVarint() throws Exception {
UnknownFieldSet.Field field = getField("optional_int32");
assertThat(field.getVarintList()).hasSize(1);
@@ -186,6 +356,16 @@
}
@Test
+ public void testAsMap() throws Exception {
+ UnknownFieldSet.Builder builder = UnknownFieldSet.newBuilder().mergeFrom(unknownFields);
+ Map<Integer, UnknownFieldSet.Field> mapFromBuilder = builder.asMap();
+ assertThat(mapFromBuilder).isNotEmpty();
+ UnknownFieldSet fields = builder.build();
+ Map<Integer, UnknownFieldSet.Field> mapFromFieldSet = fields.asMap();
+ assertThat(mapFromFieldSet).containsExactlyEntriesIn(mapFromBuilder);
+ }
+
+ @Test
public void testClear() throws Exception {
UnknownFieldSet fields = UnknownFieldSet.newBuilder().mergeFrom(unknownFields).clear().build();
assertThat(fields.asMap()).isEmpty();
