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

Support pooling repeated fields #63

Open
joe-elliott opened this issue Oct 5, 2022 · 2 comments
Open

Support pooling repeated fields #63

joe-elliott opened this issue Oct 5, 2022 · 2 comments
Labels
question Further information is requested

Comments

@joe-elliott
Copy link

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
Copy link
Author

ok, i'm seeing the description here of expected behavior and understand now what is being done.
#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 nockty added the question Further information is requested label Sep 8, 2023
@nockty
Copy link
Contributor

nockty commented Sep 8, 2023

^ 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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

No branches or pull requests

2 participants