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

BytesMut should implicitly grow the internal storage in its BufMut implementation. #170

Closed
carllerche opened this issue Dec 14, 2017 · 10 comments · Fixed by #316
Closed
Milestone

Comments

@carllerche
Copy link
Member

This will bring the implementation in line with that of Vec. This is a breaking change.

Relates to #131, #77.

@cramertj
Copy link

cramertj commented Dec 14, 2017

Can you explain why this is a breaking change? Are users relying on the fact that the buffer does not grow implicitly?

@carllerche
Copy link
Member Author

Yes, users (well, I am at the very least) are relying on the fact that the buffer does not grow implicitly.

@stbuehler
Copy link

I'm very much in favor of this; or in providing another type (BytesExt) or a newtype wrapper (see below) that defaults to extend the allocation by default.

Even with #199 fixed there are still some places where not extending is imho neither expected nor useful:

Maybe ButMut could also solve this by adding another trait BufMutReserve that provides an abstraction to reserve more space if the BufMut implementation for the type doesn't allocate itself (i.e. the implementation for Vec<u8> would for now be a noop), and a newtype wrapper for any BufMut+BufMutReserve, for which the BufMut implementation would automatically reserve the needed space.

@carllerche
Copy link
Member Author

If this is functionality that you require immediately, could you define a wrapper type for now?

@BusyJay
Copy link

BusyJay commented Sep 29, 2018

I'm in favor with the reserve function, which leave more flexibility for users to control how the capacity grows.

@mzabaluev
Copy link
Contributor

It looks as though there may be a need for two traits serving the extending and non-extending use cases. If there is really just one use case, the preferred behavior should be fixed in the contract of BufMut and all implementations should be made conformant.

I'm in favor with the reserve function, which leave more flexibility for users to control how the capacity grows.

If slices, Cursor, and other non-extendable types are to keep implementing BufMut, such reserve method needs to be fallible, right?

@mzabaluev
Copy link
Contributor

For the implicitly allocating case, what I would like to have is a type that (to largely reuse the current code) wraps BytesMut, implements std::fmt::Write and all the put* facilities of BufMut but in a grow-as-needed fashion, internally forming a sequence of Bytes chunks with take() from the underlying buffer as it gets filled up, in order to avoid copying reallocations. The chunks can then be handed off in a Buf for iovec output or iterated over otherwise.

This could allow simple implementations of non-reallocating structured/framed/serde sinks in a way that current Tokio interfaces do not provide.

benesch added a commit to MaterializeInc/materialize that referenced this issue Apr 26, 2019
BytesMut does not automatically grow its capacity.

See: tokio-rs/bytes#170
@BusyJay
Copy link

BusyJay commented Sep 20, 2019

Any updates on this?

@carllerche
Copy link
Member Author

Still scheduled for 0.5 :)

@mzabaluev
Copy link
Contributor

For the implicitly allocating case, what I would like to have is a type that (to largely reuse the current code) wraps BytesMut, implements std::fmt::Write and all the put* facilities of BufMut but in a grow-as-needed fashion, internally forming a sequence of Bytes chunks with take() from the underlying buffer as it gets filled up, in order to avoid copying reallocations. The chunks can then be handed off in a Buf for iovec output or iterated over otherwise.

To follow up on this, here is ChunkedBytes that I wrote as part of a larger project: https://github.com/mzabaluev/tokio-text/blob/master/src/chunked_bytes.rs

I think something like this would be more efficient than serializing into a contiguous growing BytesMut.

carllerche added a commit that referenced this issue Nov 20, 2019
This brings `BytesMut` in line with `Vec<u8>` behavior.

In order to fix a test, `BufMutExt::chain_mut` is provided. Withou this,
it is not possible to chain two `&mut [u8]`.

Closes #170
carllerche added a commit that referenced this issue Nov 20, 2019
This brings `BytesMut` in line with `Vec<u8>` behavior.

This also fixes an existing bug in BytesMut::bytes_mut that exposes
invalid slices. The bug was recently introduced and was only on master
and never released to `crates.io`.

In order to fix a test, `BufMutExt::chain_mut` is provided. Withou this,
it is not possible to chain two `&mut [u8]`.

Closes #170
benesch added a commit to benesch/bytes that referenced this issue Dec 24, 2019
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

Successfully merging a pull request may close this issue.

5 participants