golang / protobuf

Go support for Google's protocol buffers
BSD 3-Clause "New" or "Revised" License
9.64k stars 1.58k forks source link

About the suggestion for the append operation in the unmarshalList function. #1550

Closed EddieChan1993 closed 1 year ago

EddieChan1993 commented 1 year ago

What version of protobuf and what language are you using? Version: v1.5.2

What did you do? Please provide the context of your code usage.

type RoleData struct {
    Dogs []int
}
roleData:= make([]int, 3)
proto.UnmarshalOptions{Merge: true}.Unmarshal(bin, roleData)

The variable bin contains a slice variable named dogs with a length of 4. What did you expect to see? During usage, my expectation was that the length of my roleData's sliceList would still be 4 after Unmarshal, but in reality it became 7. What did you see instead? By examining the source code(proto/decode_gen.go.219), it was found that the append operation is causing the length of roleData to increase from 3. This inner situation is completely unknown to the user. In fact, when the user uses merge , they only want to merge the variable values in roleData into bin , and ultimately the bin value should be the primary data, not the append operation changing the length of the final slice. This point is very important.

Anything else we should know about your project / environment?

puellanivis commented 1 year ago

I am making a few assumptions here, as the given code sample is incomplete and would not compile.

If one is using Merge: true then Unmarshal does not reset the inner Dogs slice of your RoleData. This means that if given a proto.Message of &pb.RoleData{ Dogs: make([]int, 3) } and bin encodes four more additional items, then after the Unmarshal the Dogs slice should contain 7 elements, the original 3 from the make and the 4 additional from the encoded data in bin.

This is intended behavior.

EddieChan1993 commented 1 year ago

Yes, the reason why I used proto.Message &pb.RoleData{ Dogs: make([]int, 3) } is that when the Dogsfield in bin is nil, I want Unmarshalto initialize it to have a length of 3. However, if the Dogs field in bin is not nil, I do not want Unmarshal to append to it and change its length from 3 to 6. I still want it to have a length of 3 after Unmarshal.

puellanivis commented 1 year ago

That is not a semantic that is supported directly by the protobuf library. Instead, you would need to unmarshal, and then use:

if len(myPB.Dogs) < 3 {
  myPB.Dogs = append(myPB.Dogs, make([]int, 3 - len(myPB.Dogs))...)
}

This proto package does not attempt to reuse pre-allocated slices. Either Merge: false and it is overwriting the slice anyways, or Merge: true and we assume that slices with existing values are to be appended to.

If you want to reduce the allocations, but not worried about making sure that log(myPB.Dogs) >= 3 you could use merge, but instead use make([]int, 0, 3) which will make an int slice of length 0, but capacity 3, and then when the append() happens, it should use that existing capacity rather than allocate a whole new int slice… but then, it should be relatively efficiently allocating the backing array for the slice anyways.

EddieChan1993 commented 1 year ago

ok,3ks for your response,i got it