Re: Fix for Protobuf issue 136: memoized serialized size of packed fields was invalid (issue157169)

[Kenton Varda]

I guess it's technically the case that a packed field always has non-zero size, but maybe we should initialize the cached sizes to -1 anyway to make it more clear when they haven't been initialized?

On Tue, Nov 24, 2009 at 7:04 PM, <jas...@google.com> wrote:

Reviewers: kenton,

Description:
Fix Issue 136: the memoized serialized size for packed fields may not
be properly set. writeTo() may be invoked without a call to
getSerializedSize(), so the generated serialization methods would
write a length of 0 for non-empty packed fields. Since the object
is immutable, we know that the memoized size is 0 or set to the
correct size. In the generated code, check to see if the memoized
size is 0, and if so, call getSerializedSize() before actually
emitting the field.

Tested: new unittest case in WireFormatTest.java now passes



Please review this at http://codereview.appspot.com/157169

Affected files:
 M     java/src/test/java/com/google/protobuf/WireFormatTest.java
 M     src/google/protobuf/compiler/java/java_primitive_field.cc


Index: java/src/test/java/com/google/protobuf/WireFormatTest.java
===================================================================
--- java/src/test/java/com/google/protobuf/WireFormatTest.java  (revision 246)
+++ java/src/test/java/com/google/protobuf/WireFormatTest.java  (working copy)
@@ -102,6 +102,28 @@
    assertEquals(rawBytes, rawBytes2);
  }

+  public void testSerializationPackedWithoutGetSerializedSize()
+      throws Exception {
+    // Write directly to an OutputStream, without invoking getSerializedSize()
+    // This used to be a bug where the size of a packed field was incorrect,
+    // since getSerializedSize() was never invoked.
+    TestPackedTypes message = TestUtil.getPackedSet();
+
+    // Directly construct a CodedOutputStream around the actual OutputStream,
+    // because writeTo() now invokes getSerializedSize();
+    ByteArrayOutputStream outputStream = new ByteArrayOutputStream();
+    CodedOutputStream codedOutput = CodedOutputStream.newInstance(outputStream);
+
+    message.writeTo(codedOutput);
+
+    codedOutput.flush();
+
+    TestPackedTypes message2 = TestPackedTypes.parseFrom(
+        outputStream.toByteArray());
+
+    TestUtil.assertPackedFieldsSet(message2);
+  }
+
  public void testSerializeExtensionsLite() throws Exception {
    // TestAllTypes and TestAllExtensions should have compatible wire formats,
    // so if we serialize a TestAllExtensions then parse it as TestAllTypes
Index: src/google/protobuf/compiler/java/java_primitive_field.cc
===================================================================
--- src/google/protobuf/compiler/java/java_primitive_field.cc   (revision 246)
+++ src/google/protobuf/compiler/java/java_primitive_field.cc   (working copy)
@@ -384,8 +384,17 @@
 void RepeatedPrimitiveFieldGenerator::
 GenerateSerializationCode(io::Printer* printer) const {
  if (descriptor_->options().packed()) {
+    // writeTo() may be called without getSerializedSize() ever having been
+    // called, so we may need to compute the length of the packed data. Since
+    // the object is immutable, we know that the memoized size is either set to
+    // 0 (via object initialization) or else it has the correct size from a
+    // previous getSerializedSize() call. If the field is not empty and the size
+    // has not yet been computed, just call getSerializedSize()
    printer->Print(variables_,
      "if (get$capitalized_name$List().size() > 0) {\n"
+      "  if ($name$MemoizedSerializedSize == 0) {\n"
+      "    getSerializedSize();\n"
+      "  }\n"
      "  output.writeRawVarint32($tag$);\n"
      "  output.writeRawVarint32($name$MemoizedSerializedSize);\n"
      "}\n"

[Kenton Varda]

On Tue, Nov 24, 2009 at 7:40 PM, Kenton Varda <ken...@google.com> wrote:

I guess it's technically the case that a packed field always has non-zero size, but maybe we should initialize the cached sizes to -1 anyway to make it more clear when they haven't been initialized?

That was a confusing sentence.  Change "when they haven't been initialized" to "when getSerializedSize() hasn't been called".

[jas...@google.com]

Done. But it occurs to me that this isn't all that much nicer a solution
than just calling getSerializedSize() in writeTo(CodedOutputStream
output), except that it only happens for messages with packed fields,
rather than all messages. Should this compute only the serialized size
of the packed field, so that it doesn't needlessly compute the size of
all the unpacked fields? Or does this not really matter? I think all of
the wrappers now call getSerializedSize() thanks to r240 (I was tearing
my hair out trying to figure out why I could not get a new test which
just did
ByteArrayOutputStream output = new ByteArrayOutputStream();
message.writeTo(output);
would not fail from the open source code, but would if I added the same
test to our internal version, until I finally looked at the source).
Anyway point being that in most cases getSerializedSize() will have
already been called anyway so maybe it doesn't matter too much if we
compute the size of the entire message rather than just the packed
fields.

http://codereview.appspot.com/157169

[Kenton Varda]

You're right, we might as well always call it.

[jas...@google.com]

Done. I kept the change to initialize the memoized size of packed fields
to -1, just for consistency with the message's size.

On 2009/11/25 20:51:52, kenton wrote:
> You're right, we might as well always call it.

http://codereview.appspot.com/157169

[ken...@google.com]

LGTM, just a minor nitpick:


http://codereview.appspot.com/157169/diff/2007/1012
File src/google/protobuf/compiler/java/java_message.cc (right):

http://codereview.appspot.com/157169/diff/2007/1012#newcode404
src/google/protobuf/compiler/java/java_message.cc:404:
"getSerializedSize();");
Missing \n.

http://codereview.appspot.com/157169