Suspected bug in java 2.4.0a Sub-builders

[jamesm]

I have found two things that look to me like bugs in sub-builder
handling; one is in the generated code; the second is in the base
class GeneratedMessage.

Consider the following:

message Inner {
optional int32 somefield = 1;
}

message Outer{
optional Inner inner = 1;
}

Generated code for sub builder is roughtly as follows:
public Inner.Builder getInnerBuilder() {
bitField0_ |= 0x00000004; //wrong - we've not changed it yet
onChanged(); //wrong - we've not changed it yset
return getInnerFieldBuilder().getBuilder();
}

The net effect is that calling:
outer.getInnerBuilder() marks outer as dirty; which in my view is
incorrect

The second issue is in GeneratedMessage.Builder:

protected final void onChanged() {
if (isClean && builderParent != null) {
builderParent.markDirty();

// Don't keep dispatching invalidations until build is called
again.
isClean = false;
}
}
This again seems to be incorrect; it should be as follows:
protected final void onChanged() {
if (isClean && builderParent != null) {
builderParent.markDirty();
}
// Don't keep dispatching invalidations until build is called
again.
isClean = false;
}

I can see how these two bugs together would pass a basic Junit; but
they showed up in some more extensive tests that are part of the
system that I'm building.

With these changes made we would have the desired effect that only
when a modification is made to the sub builder does that builder (and
it's parent) get marked as dirty.

Please can you confirm that the above are bugs and whether they can be
fixed before 2.4.0 final?

Thanks

[Kenton Varda]

Please reply-all so that others on the mailing list can see...

On Tue, Feb 22, 2011 at 11:51 AM, James Mulvey <james....@gmail.com> wrote:

1st issue is relatively easy to work around; I just call clear after construction.

Unfortunately that makes the second bug nasty; since then the outer message believes there is no change to the inner message when a modification is made to it.

This usage pattern is not really supposed to work.  Sub-builders do not work like sub-messages in Python; they work like sub-messages in C++.  So, setting a field on a sub-builder is not supposed to mark it present in the parent message, if it isn't present already.  Basically, when you call clear(), you're saying "I am not using the sub-builders anymore", and you have to call getInnerBuilder() again to recreate the inner message.  We should probably change the code to actually discard all sub-builders on clear().

 

Maybe explaining what I am doing will help:

I am writing transactional objects composed of an outer wrapper to two protobufs; all sets delegate to the delta message; all gets check the delta message and then the state.

Commit basically does state.mergeFrom(delta) and does dela.clear().

Rollback basically does delta.clear()

So with the second issue the outer object never records the change to the sub-object (and thus fails my unit test and use case)

I think this would work correctly if you didn't try to clear() immediately after getInnerBuilder().

 

I'm still mulling over the 1st issue.. There could be use cases where it is useful; but then having a get() mark it as dirty seems a little wrong.

Great work by the way!

Sent from my iPhone

On 22 Feb 2011, at 17:09, Kenton Varda <ken...@google.com> wrote:

The first issue is the intended behavior.  It matches C++, where calling mutable_inner() instantiates the inner message.  There needs to be a way to mark a sub-message "present" without actually setting any of its friends.

The second issue looks genuine, although it seems to me that it would cause no behavioral differences, only some wasted instructions.  Is this correct?

2.4.0a is actually a "final" release.  In fact, it is the successor to 2.4.0, which had a build problem.  If there are significant bugs we can do a 2.4.0b or 2.4.1 to fix them.

--
You received this message because you are subscribed to the Google Groups "Protocol Buffers" group.
To post to this group, send email to prot...@googlegroups.com.
To unsubscribe from this group, send email to protobuf+u...@googlegroups.com.
For more options, visit this group at http://groups.google.com/group/protobuf?hl=en.

[James Mulvey]

It would be nice if it did work; I guess this is based more on my interpretation of why the sub-builders reference the parent.

In the current implementation the builders are entirely re-usable and as such minimise gc. 

If I did as you describe then I'd need to create a delta builder for every transaction; which would hurt when trying to process my 100k msg/sec target.

With this small change it is possible to handle delta style messaging much more effectively in java (I can't comment so much for other languages aside from c# where I'd hope the port would mirror java - due to the same gc architecture)

Sent from my iPhone

[Kenton Varda]

The first issue is the intended behavior.  It matches C++, where calling mutable_inner() instantiates the inner message.  There needs to be a way to mark a sub-message "present" without actually setting any of its friends.

The second issue looks genuine, although it seems to me that it would cause no behavioral differences, only some wasted instructions.  Is this correct?

2.4.0a is actually a "final" release.  In fact, it is the successor to 2.4.0, which had a build problem.  If there are significant bugs we can do a 2.4.0b or 2.4.1 to fix them.

On Tue, Feb 22, 2011 at 6:54 AM, jamesm <james....@gmail.com> wrote: