Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make field writing order consistent between CODE_SIZE and SPEED #453

Open
jcready opened this issue Dec 24, 2022 · 0 comments · May be fixed by #577
Open

Make field writing order consistent between CODE_SIZE and SPEED #453

jcready opened this issue Dec 24, 2022 · 0 comments · May be fixed by #577

Comments

@jcready
Copy link
Contributor

jcready commented Dec 24, 2022

When optimizing for CODE_SIZE the MessageType's internalBinaryWrite is (effectively) generated at runtime and will sort the fields by their field number.

protected prepare() {
if (!this.fields) {
const fieldsInput = this.info.fields ? this.info.fields.concat() : [];
this.fields = fieldsInput.sort((a, b) => a.no - b.no);
}
}
/**
* Writes the message to binary format.
*/
write<T extends object>(message: T, writer: IBinaryWriter, options: BinaryWriteOptions): void {
this.prepare();
for (const field of this.fields!) {

However the same sorting is not applied when optimizing for SPEED. It seems like both optimization strategies could be optimized and made consistent if the fields were sorted in the plugin interpreter instead.

private buildFieldInfos(fieldDescriptors: readonly FieldDescriptorProto[]): rt.PartialFieldInfo[] {
const result: rt.PartialFieldInfo[] = [];
for (const fd of fieldDescriptors) {
if (this.registry.isGroupField(fd)) {
// We ignore groups.
// Note that groups are deprecated and not supported in proto3.
continue;
}
const fi = this.buildFieldInfo(fd);
if (fi) {
result.push(fi);
}
}
return result;
}

                 result.push(fi);
             }
         }
-        return result;
+        return result.sort((a, b) => a.no - b.no);
     }

I realize this doesn't make any difference in conforming to the protobuf spec, but it would at least keep the written binary consistent between optimization strategies within protobuf-ts. It also means that rearranging the fields of a message inside the proto file (not the field numbers) wouldn't affect the written binary of said message. e.g. these two arrangements of fields would produce identical binary output for the same field values:

message Foo {
  string baz = 1;
  string qux = 2;
}

message Bar {
  string qux = 2;
  string baz = 1;
}

Rearranging of the fields in the proto file might happen if new fields get added where perhaps it might make sense to visually group some of the new fields with the existing fields. Another case where this happens is if we wanted to group all the deprecated fields together at the top or bottom of the message.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant