Enhancement: order fields by tag when serializing structs

[GoogleCodeExporter]

What steps will reproduce the problem?

1. Define a Protobuf message with two required fields with tags 2 and 1 
respectively, e.g.

    message msg {
        repeated bool foo = 2;
        repeated bool bar = 1;
    }

2. use proto.Marshal to serialize an instance of the correspondent Go struct

What is the expected output? What do you see instead?

Serialized fields should be ordered by tag according to the Protocol Buffers 
specification 
(http://code.google.com/apis/protocolbuffers/docs/encoding.html#order), i.e. 
field "bar" (tag 1) followed by field "foo" (tag 2).

Instead, fields are written in the order they were defined in the .proto 
specification, i.e. field "foo" (tag 2) followed by field "bar" (tag 1).

What version of the product are you using? On what operating system?

tip, all systems

Please provide any additional information below.

Attached is a patch that implements ordering of serialized fields by tag. The 
tests are updated accordingly by reordering the precomputed test output.

Original issue reported on code.google.com by alav...@piqi.org on 2 Oct 2011 at 9:22

Attachments:

• order_fields_by_tag.patch

[GoogleCodeExporter]

This is not a defect. It's simply a difference between the C++ and Go 
implementations of protocol buffers.

Original comment by dsymo...@golang.org on 2 Oct 2011 at 9:30

• Changed state: WorkingAsIntended
• Added labels: Type-Enhancement
• Removed labels: Type-Defect

[GoogleCodeExporter]

The issue submission interface doesn't provide the way to choose between issue 
types (at least I couldn't find that option). This is why I marked it as 
"enhancement" in the header.

Speaking of the enhancement itself, the Protocol Buffers specification I 
referred to says "known fields should be written sequentially by field number, 
as in the provided C++, Java, and Python serialization code. This allows 
parsing code to use optimizations that rely on field numbers being in sequence".

It sounds like a recommendation to me, not just a difference between 
implementations.

If not for optimization reason, sometimes (e.g. in my use case) it is useful to 
be able to compare Protobuf messages serialized by different implementations by 
comparing them byte-by-byte.

Also, it is worth mentioning that the proposed enhancement has almost zero 
runtime overhead.

Original comment by alav...@piqi.org on 2 Oct 2011 at 10:18

[GoogleCodeExporter]

Protocol buffer equality should be done with proto.Equal; there are myriad ways 
for two equal protocol buffers to have different encodings.

Your proposed enhancement may have almost zero runtime overhead, but it's 
adding a non-trivial amount of code that would be nice to avoid.

All that's left is that this is an optimisation. I'm not against optimisations, 
but I wonder how big an impact it actually has. A benchmark would be a 
compelling reason.

Original comment by dsymo...@golang.org on 2 Oct 2011 at 10:23

• Added labels: Performance

[GoogleCodeExporter]

The proposed change amounts to 5 lines of trivial code. It adds 3 lines of code 
and 1 field definition to "properties.go" and alters 1 line in "encode.go"

The rest of the changes include Go's boilerplate for making a slice of 
properties sortable by tag and quite straightforward reordering of precomputed 
test data by field tag.

As for the optimization and its impact on the performance of various decoders, 
I haven't run any substantial benchmarks, but I can clearly see how C++ code 
generated by "protoc --cpp_out" takes advantage of fields being ordered by tag. 
Basically, if the fields are ordered, for each field, it avoids going through 
"switch(field tag)" and performs a single "goto" jump instead.

Original comment by alav...@piqi.org on 2 Oct 2011 at 11:38

[GoogleCodeExporter]

After some discussion, we're going to do this, but in a different way.

Original comment by dsymo...@golang.org on 3 Oct 2011 at 7:43

• Changed state: Started

[GoogleCodeExporter]

I'm glad to hear that!

Original comment by alav...@piqi.org on 3 Oct 2011 at 9:08

[GoogleCodeExporter]

This issue was closed by revision f0f9983b6de6.

Original comment by dsymo...@golang.org on 3 Oct 2011 at 9:31

• Changed state: Fixed