planetscale / vtprotobuf

A Protocol Buffers compiler that generates optimized marshaling & unmarshaling Go code for ProtoBuf APIv2
BSD 3-Clause "New" or "Revised" License
868 stars 74 forks source link

Support pooling repeated fields #63

Open joe-elliott opened 1 year ago

joe-elliott commented 1 year ago

As far as I can tell single fields are pooled correctly but repeated fields are not. Using the following proto:

message Parent {
  option (vtproto.mempool) = true;
  repeated Child children = 1;
  Child one = 2;
}

message Child {
  option (vtproto.mempool) = true;
  uint32 field = 1;
}

I can see the the ResetVT and UnmarshalVT methods correctly handle the "one" field but not the "children" field.

func (m *Parent) ResetVT() {
    for _, mm := range m.Children {
        mm.ResetVT()  // does not return the slice pointers to the pool
    }
    m.One.ReturnToVTPool()   // correctly returns this pointer to the pool
    m.Reset()
}
func (m *Parent) UnmarshalVT(dAtA []byte) error {
...
        switch fieldNum {
        case 1:
...
            if len(m.Children) == cap(m.Children) {
                m.Children = append(m.Children, &Child{}) // allocates new object for slice
            } else {
                m.Children = m.Children[:len(m.Children)+1]
                if m.Children[len(m.Children)-1] == nil {
                    m.Children[len(m.Children)-1] = &Child{} // allocates new object for slice
                }
            }
...
        case 2:
...
            if m.One == nil {
                m.One = ChildFromVTPool() // correctly pulls from pool
            }
...

Is there a way to do this that I'm not seeing?

joe-elliott commented 1 year ago

ok, i'm seeing the description here of expected behavior and understand now what is being done. https://github.com/planetscale/vtprotobuf/issues/8

We are looking at using this project with https://github.com/open-telemetry/opentelemetry-proto. I do believe for complex hierarchies of objects with highly variable slice lengths it's possible that individually pooling objects would out perform the current approach, but I do understand now what it's doing. If time permits I may attempt some benchmarks on the linked proto.

nockty commented 1 year ago

^ Adding this label to remind us that we need further information to tackle this issue. Namely, a benchmark showing that individually pooling objects can be more performant in certain scenarios.

Thank you for raising this!