Skip to content

Commit

Permalink
Clean up TextFormat parser (#10673)
Browse files Browse the repository at this point in the history
* Fix TextFormat parser
  • Loading branch information
mkruskal-google authored Sep 29, 2022
1 parent 3b5301c commit a3888f5
Show file tree
Hide file tree
Showing 2 changed files with 59 additions and 12 deletions.
6 changes: 5 additions & 1 deletion CHANGES.txt
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,11 @@
* Change the Lite runtime to prefer merging from the wireformat into mutable
messages rather than building up a new immutable object before merging. This
way results in fewer allocations and copy operations.
* Make message-type extensions merge from wire-format instead of building up instances and merging afterwards. This has much better performance.
* Make message-type extensions merge from wire-format instead of building up
instances and merging afterwards. This has much better performance.
* Fix TextFormat parser to build up recurring (but supposedly not repeated)
sub-messages directly from text rather than building a new sub-message and
merging the fully formed message into the existing field.

2022-09-13 version 21.6 (C++/Java/Python/PHP/Objective-C/C#/Ruby)

Expand Down
65 changes: 54 additions & 11 deletions java/core/src/main/java/com/google/protobuf/MessageReflection.java
Original file line number Diff line number Diff line change
Expand Up @@ -378,6 +378,7 @@ MergeTarget newEmptyTargetForField(
static class BuilderAdapter implements MergeTarget {

private final Message.Builder builder;
private boolean hasNestedBuilders = true;

@Override
public Descriptors.Descriptor getDescriptorForType() {
Expand All @@ -393,13 +394,30 @@ public Object getField(Descriptors.FieldDescriptor field) {
return builder.getField(field);
}

private Message.Builder getFieldBuilder(Descriptors.FieldDescriptor field) {
if (hasNestedBuilders) {
try {
return builder.getFieldBuilder(field);
} catch (UnsupportedOperationException e) {
hasNestedBuilders = false;
}
}
return null;
}

@Override
public boolean hasField(Descriptors.FieldDescriptor field) {
return builder.hasField(field);
}

@Override
public MergeTarget setField(Descriptors.FieldDescriptor field, Object value) {
if (!field.isRepeated() && value instanceof MessageLite.Builder) {
if (value != getFieldBuilder(field)) {
builder.setField(field, ((MessageLite.Builder) value).buildPartial());
}
return this;
}
builder.setField(field, value);
return this;
}
Expand All @@ -413,12 +431,18 @@ public MergeTarget clearField(Descriptors.FieldDescriptor field) {
@Override
public MergeTarget setRepeatedField(
Descriptors.FieldDescriptor field, int index, Object value) {
if (value instanceof MessageLite.Builder) {
value = ((MessageLite.Builder) value).buildPartial();
}
builder.setRepeatedField(field, index, value);
return this;
}

@Override
public MergeTarget addRepeatedField(Descriptors.FieldDescriptor field, Object value) {
if (value instanceof MessageLite.Builder) {
value = ((MessageLite.Builder) value).buildPartial();
}
builder.addRepeatedField(field, value);
return this;
}
Expand Down Expand Up @@ -536,11 +560,19 @@ public void mergeGroup(
Message defaultInstance)
throws IOException {
if (!field.isRepeated()) {
Message.Builder subBuilder;
if (hasField(field)) {
input.readGroup(field.getNumber(), builder.getFieldBuilder(field), extensionRegistry);
return;
subBuilder = getFieldBuilder(field);
if (subBuilder != null) {
input.readGroup(field.getNumber(), subBuilder, extensionRegistry);
return;
} else {
subBuilder = newMessageFieldInstance(field, defaultInstance);
subBuilder.mergeFrom((Message) getField(field));
}
} else {
subBuilder = newMessageFieldInstance(field, defaultInstance);
}
Message.Builder subBuilder = newMessageFieldInstance(field, defaultInstance);
input.readGroup(field.getNumber(), subBuilder, extensionRegistry);
Object unused = setField(field, subBuilder.buildPartial());
} else {
Expand All @@ -558,11 +590,19 @@ public void mergeMessage(
Message defaultInstance)
throws IOException {
if (!field.isRepeated()) {
Message.Builder subBuilder;
if (hasField(field)) {
input.readMessage(builder.getFieldBuilder(field), extensionRegistry);
return;
subBuilder = getFieldBuilder(field);
if (subBuilder != null) {
input.readMessage(subBuilder, extensionRegistry);
return;
} else {
subBuilder = newMessageFieldInstance(field, defaultInstance);
subBuilder.mergeFrom((Message) getField(field));
}
} else {
subBuilder = newMessageFieldInstance(field, defaultInstance);
}
Message.Builder subBuilder = newMessageFieldInstance(field, defaultInstance);
input.readMessage(subBuilder, extensionRegistry);
Object unused = setField(field, subBuilder.buildPartial());
} else {
Expand All @@ -586,11 +626,14 @@ private Message.Builder newMessageFieldInstance(
public MergeTarget newMergeTargetForField(
Descriptors.FieldDescriptor field, Message defaultInstance) {
Message.Builder subBuilder;
if (defaultInstance != null) {
subBuilder = defaultInstance.newBuilderForType();
} else {
subBuilder = builder.newBuilderForField(field);
if (!field.isRepeated() && hasField(field)) {
subBuilder = getFieldBuilder(field);
if (subBuilder != null) {
return new BuilderAdapter(subBuilder);
}
}

subBuilder = newMessageFieldInstance(field, defaultInstance);
if (!field.isRepeated()) {
Message originalMessage = (Message) getField(field);
if (originalMessage != null) {
Expand Down Expand Up @@ -626,7 +669,7 @@ public WireFormat.Utf8Validation getUtf8Validation(Descriptors.FieldDescriptor d

@Override
public Object finish() {
return builder.buildPartial();
return builder;
}
}

Expand Down

1 comment on commit a3888f5

@eslerm
Copy link

@eslerm eslerm commented on a3888f5 Nov 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please sign in to comment.