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

Any plans to use MemoryPool when deserializing ByteString? #8238

Closed
wolf8196 opened this issue Jan 28, 2021 · 1 comment
Closed

Any plans to use MemoryPool when deserializing ByteString? #8238

wolf8196 opened this issue Jan 28, 2021 · 1 comment
Assignees
Labels

Comments

@wolf8196
Copy link

wolf8196 commented Jan 28, 2021

This is a question in regards to .NET implementation of protocol buffers.
I started using protobuf together with gRPC, so sorry, if I'm missing the bigger picture or don't take other use cases into account.

Considering the big number of guides & tutorials (including MSDN) recommending to use ArrayPool or MemoryPool to minimize memory allocations and improve performance, I was wondering why ByteString doesn't use the memory from MemoryPool?

I saw that recently there was a PR that changed ByteString to use memory and added unsafe create without copy.
This allows to rent the memory from pool, create ByteString, send it, and release the memory.
This should be beneficial for client side, but on server side, it's the protobuf's code that does the allocation, and it currently creates an array for the whole field.

So back to the question: are there any plans to rent the memory from the pool for ByteStrings, and let the users dispose of it once they are done? Or is there a reason why it's a bad idea? Or will the benefits actually be so small, that it's not worth the effort?

@jtattermusch
Copy link
Contributor

The fundamental problem is that currently ByteString (or any other objects for representing protobuf fields / messages) doesn't provide an API for disposing the underlying values, which makes the scenario of renting memory from a pool a returning it non-intuitive and it's easy to get things wrong and either leak memory or erroneously read from memory that was already returned to the pool.

#7645 was mostly introduced for advanced and "use at your own risk" scenarios, not as a general purpose API that everyone should use. Using the new API you can achieve renting the byteStrings for some scenarios when you really need to (and after you've measured it's worth it), but as was pointed out in that PR's thread already, using it in your day-to-day usage of protobufs is not recommended (because there's no way of controlling the lifecycle of the rented memory).

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

No branches or pull requests

3 participants