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

Contradictory allocation behaviour compared to documentation #680

Closed
shahn opened this issue Mar 20, 2024 · 9 comments
Closed

Contradictory allocation behaviour compared to documentation #680

shahn opened this issue Mar 20, 2024 · 9 comments

Comments

@shahn
Copy link
Contributor

shahn commented Mar 20, 2024

I hope I'm not just hopelessly confused, but I fail to see what kind of guarantees are made around allocations and using the APIs. For example, https://docs.rs/bytes/latest/bytes/buf/trait.BufMut.html#method.put_slice says that self must have enough remaining capacity to contain all of src. but the implementation will happily extend the buffer of a BytesMut beyond what's reported from a call to capacity(). Similarly, the documentation of BufMut::put_bytes() explicitly mentions a panic will happen if there is not enough capacity, yet the implementation again does not panic and instead allocates more memory - similarly for the other put* functions.

Some functions, like BytesMut::extend_from_slice(), explicitly call out this re-allocation behaviour - but what then is the point of that function, if it does the same as put_slice()?

@Darksonn
Copy link
Contributor

The documentation on this could probably be improved, but the capacity that the BufMut trait talks about is for cases where the buffer cannot be resized at all. Types such as Vec<u8> pretend that they have infinite capacity in the BufMut trait.

I agree that the wording here is confusing, but I think it is the right behavior.

@shahn
Copy link
Contributor Author

shahn commented Mar 21, 2024

Is there some API which basically forces me to make reallocations explicit? I would be happy to see if I can improve the documentation a little bit when I have understood it better.

@Darksonn
Copy link
Contributor

I don't think we provide that, no.

@shahn
Copy link
Contributor Author

shahn commented Mar 22, 2024

Hmm, OK - thank you.

I was using a BytesMut to store data received from the network, split it off and freeze it to send a Bytes over to another thread for compression and logging. I tested it first and it seemed to work, probably because there were always times when all Bytes handles had been dropped - but in the real project, I get lots of very small data packages which probably means there are always some Bytes handles alive.

One idea to deal with this was to have two BytesMut buffers, when one would get full I could switch to the other buffer while the remaining Bytes from the first buffer get dropped. Then I could move back to the first buffer once there is nothing pointing to it anymore, and vice versa. Unfortunately, there doesn't seem to be an API available to check whether a BytesMut has any live Bytes pointing into its storage. Would you be open to adding such an API, or is my usecase already achievable some other way?

@Darksonn
Copy link
Contributor

An API for getting the refcount is reasonable enough.

shahn added a commit to shahn/bytes that referenced this issue Mar 31, 2024
This is based on tokio-rs#680, where it was noted that it is hard to use
BytesMut without additional allocations in some circumstances.
shahn added a commit to shahn/bytes that referenced this issue Mar 31, 2024
This is based on tokio-rs#680, where it was noted that it is hard to use
BytesMut without additional allocations in some circumstances.
shahn added a commit to shahn/bytes that referenced this issue Mar 31, 2024
This is based on tokio-rs#680, where it was noted that it is hard to use
BytesMut without additional allocations in some circumstances.
@shahn
Copy link
Contributor Author

shahn commented Mar 31, 2024

I gave it some thought and for my usecase, an API like #686 seems more usable - it doesn't expose implementation details like refcount, and it gives a cheap way to re-use the original implementation. Do you think that approach is sensible? If so I'd be happy to bring it to the finish line. Thanks!

shahn added a commit to shahn/bytes that referenced this issue Mar 31, 2024
This is based on tokio-rs#680, where it was noted that it is hard to use
BytesMut without additional allocations in some circumstances.
@braddunbar
Copy link
Contributor

I've thought about this too. Thanks for posting @shahn! In fact, the reason I first looked into the source code for this crate was because I didn't understand the conditions that cause allocation after reading the documentation. The following snippet from the docs is accurate, but it's not a complete picture of the BytesMut allocation strategy and the constraints it assumes.

BytesMut’s BufMut implementation will implicitly grow its buffer as necessary. However, explicitly reserving the required space up-front before a series of inserts will be more efficient.

In particular, it's not clear that a BytesMut instance is most efficient when the Bytes instances it produces are short lived and, critically, all dropped before the entire buffer is filled, allowing the buffer to be reclaimed and filled again (please let me know if I've missed somewhere that's documented). It does not act as a ring buffer, filling in the empty space at the beginning of the buffer while a Bytes is still referencing the end of the buffer.

This is a subtle distinction, but it's important to understand when choosing to use BytesMut because if your data is received continuously and your Bytes instances are not dropped before more data arrives then the buffer is never reclaimed and a new one is allocated every time it's filled. To be clear, I think this is the correct behavior for BytesMut because it's making a specific trade off (i.e. less memory usage and bookkeeping in exchange for allocation when data is received continuously). It just wasn't clear to me when reading documentation.

@shahn
Copy link
Contributor Author

shahn commented Apr 10, 2024

I think what you explained about the behaviour is also what I've learned from docs/reading code, hence the idea to use two BytesMuts and alternate between them.

One thing that confused me initially was that some old docs which are however still readily found using web search explicitly say that BytesMut doesn't implicitly grow its buffer - but that went away once I looked at the right docs.

I noticed large memory usage from the OS pov, perhaps there is some fragmentation issue going on with frequent allocations of different sizes, really not sure. It could be fragmentation because the messages I receive have vastly different size. This seems to be better when switching between two buffers using the patch from #686.

shahn added a commit to shahn/bytes that referenced this issue May 13, 2024
This is based on tokio-rs#680, where it was noted that it is hard to use
BytesMut without additional allocations in some circumstances.
shahn added a commit to shahn/bytes that referenced this issue May 17, 2024
This fixes tokio-rs#680, where it was noted that it is hard to use
BytesMut without additional allocations in some circumstances.
@Darksonn
Copy link
Contributor

Closing with #686.

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

No branches or pull requests

3 participants