Re: [protobuf] Suspected bug in java 2.4.0a Sub-builders
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
On 22 Feb 2011, at 20:12, Kenton Varda wrote:
> Please reply-all so that others on the mailing list can see...
>
> On Tue, Feb 22, 2011 at 11:51 AM, James Mulvey 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 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.
>>
>> On Tue, Feb 22, 2011 at 6:54 AM, jamesm wrote:
>> 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_ |= 0x0004; //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
>>
>>
>>
>>
>> --
>> You received
Re: [protobuf] Suspected bug in java 2.4.0a Sub-builders
Please reply-all so that others on the mailing list can see...
On Tue, Feb 22, 2011 at 11:51 AM, James Mulvey 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 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.
>
> On Tue, Feb 22, 2011 at 6:54 AM, jamesm <
> james.mul...@gmail.com> wrote:
>
>> 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_ |= 0x0004; //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
>>
>>
>>
>>
>> --
>> You received this message because you are subscribed to the Google Groups
>> "Protocol Buffers" group.
>> To post to this group, send email to
>> protobuf@googlegroups.com.
>> To unsubscribe from this group, send email to
>>
>> protobuf+unsubscr...@googlegroups.com.
>> For more options, visit this group at
>>
>> http://groups.google.com/group/protobuf?hl=en.
>>
>>
>
--
You received this message because you are subscribed to the Google Groups
"Protocol Buffers" group.
To post to this group, send email to protobuf@googlegroups.com.
To unsubscribe from this group, send email to
protobuf+unsubscr...@googlegroups.com.
For more options, visit this group at
http://groups.google.com/group/protobuf?
Re: [protobuf] Suspected bug in java 2.4.0a Sub-builders
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 wrote:
> 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_ |= 0x0004; //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
>
>
>
>
> --
> You received this message because you are subscribed to the Google Groups
> "Protocol Buffers" group.
> To post to this group, send email to protobuf@googlegroups.com.
> To unsubscribe from this group, send email to
> protobuf+unsubscr...@googlegroups.com.
> For more options, visit this group at
> http://groups.google.com/group/protobuf?hl=en.
>
>
--
You received this message because you are subscribed to the Google Groups
"Protocol Buffers" group.
To post to this group, send email to protobuf@googlegroups.com.
To unsubscribe from this group, send email to
protobuf+unsubscr...@googlegroups.com.
For more options, visit this group at
http://groups.google.com/group/protobuf?hl=en.
[protobuf] Suspected bug in java 2.4.0a Sub-builders
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_ |= 0x0004; //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
--
You received this message because you are subscribed to the Google Groups
"Protocol Buffers" group.
To post to this group, send email to protobuf@googlegroups.com.
To unsubscribe from this group, send email to
protobuf+unsubscr...@googlegroups.com.
For more options, visit this group at
http://groups.google.com/group/protobuf?hl=en.
