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

fixes #121: java serialization breaks serializedSize #122

Merged
merged 1 commit into from
Jun 1, 2016

Conversation

eiennohito
Copy link
Contributor

Previously -1 was used as a sentinel value.
However, if a generated case class to be serialized using java serialization,
all backing fields would be reset to 0 because java serialization does not call
constructors.

This fix changes serializedSize default (sentinel) value to 0.
Serialized size of 0 is valid value and it will not be cached if the message
is actually empty.
Still, for such messages computing size should be cheap.

 Previously -1 was used as a sentinel value.
 However, if a generated case class to be serialized using java serialization,
 all backing fields would be reset to 0 because java serialization does not call
 constructors.

 This fix changes serializedSize default (sentinel) value to 0.
 Serialized size of 0 is valid value and it will not be cached if the message
 is actually empty.
 Still, for such messages computing size should be cheap.
@@ -382,8 +382,14 @@ class ProtobufGenerator(val params: GeneratorParams) extends DescriptorPimps {
.add("}")
.add("final override def serializedSize: Int = {")
.indent
.add("if (__serializedSizeCachedValue == -1) { __serializedSizeCachedValue = __computeSerializedValue() }")
.add("__serializedSizeCachedValue")
.add("var read = __serializedSizeCachedValue")
Copy link
Contributor

Choose a reason for hiding this comment

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

can you get rid of 'read' and use '__serializedSizeCachedValue` directly instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's two field accesses vs one.

JDK's toString hashCode and protobuf's ByteString use similar style:
https://github.com/google/protobuf/blob/master/java/core/src/main/java/com/google/protobuf/ByteString.java#L842

But I can make it back if you still want.

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it, no need to change. Will merge in.

On Wed, Jun 1, 2016 at 6:32 AM, Arseny Tolmachev notifications@github.com
wrote:

In
compiler-plugin/src/main/scala/com/trueaccord/scalapb/compiler/ProtobufGenerator.scala
#122 (comment):

@@ -382,8 +382,14 @@ class ProtobufGenerator(val params: GeneratorParams) extends DescriptorPimps {
.add("}")
.add("final override def serializedSize: Int = {")
.indent

  •  .add("if (__serializedSizeCachedValue == -1) { __serializedSizeCachedValue = __computeSerializedValue() }")
    
  •  .add("__serializedSizeCachedValue")
    
  •  .add("var read = __serializedSizeCachedValue")
    

It's two field accesses vs one.

JDK's toString hashCode and protobuf's ByteString use similar style:

https://github.com/google/protobuf/blob/master/java/core/src/main/java/com/google/protobuf/ByteString.java#L842

But I can make it back if you still want.


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
https://github.com/trueaccord/ScalaPB/pull/122/files/b8d6a4371ba8ce91dee94067ec51feb798b00275#r65359890,
or mute the thread
https://github.com/notifications/unsubscribe/AASwrfRPDoFIScpVxSw_2Wp41EZEaqzCks5qHYoHgaJpZM4Irdnj
.

-Nadav

@thesamet thesamet merged commit 56a48bd into scalapb:master Jun 1, 2016
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 this pull request may close these issues.

2 participants