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

Add support for zerocopy trait impls #738

Closed
joshlf opened this issue Oct 11, 2024 · 13 comments · Fixed by #740
Closed

Add support for zerocopy trait impls #738

joshlf opened this issue Oct 11, 2024 · 13 comments · Fixed by #740

Comments

@joshlf
Copy link

joshlf commented Oct 11, 2024

An implementation is prototyped here.

The proposal is to implement zerocopy's ByteSlice and SplitByteSlice traits for bytes' Bytes and BytesMut types, which will permit users to use Bytes and BytesMut with zerocopy features as requested in google/zerocopy#1444.

Note that it's not possible for zerocopy to add these impls itself since the traits have safety invariants which are not guaranteed by Bytes and BytesMut's public documentation - it's only possible to prove that these impls are correct inside of bytes itself.

@Darksonn
Copy link
Contributor

Darksonn commented Oct 13, 2024

The bytes crate is intended to never have a 2.0 release, so I'm reluctant to add new dependencies to the public API that don't have a similar guarantee.

What guarantees are missing? We may be able to guarantee the missing guarantees.

@Darksonn
Copy link
Contributor

As an aside, it looks like the safety requirements of SplitByteSlice are not true for the Bytes or BytesMut types, as the address may not have the right value for zero-length slices.

@seanmonstar
Copy link
Member

I have the same reservations as Alice. We've been very picky about adding any public dependencies.

@joshlf
Copy link
Author

joshlf commented Oct 13, 2024

What guarantees are missing? We may be able to guarantee the missing guarantees.

The main ones are the safety preconditions for ByteSlice and SplitByteSlice. If you were able to guarantee the safety invariants for these traits, that'd be great! I agree that we're likely to release new versions more frequently than bytes is.

As an aside, it looks like the safety requirements of SplitByteSlice are not true for the Bytes or BytesMut types, as the address may not have the right value for zero-length slices.

What do you mean by "not have the right value"?

@Darksonn
Copy link
Contributor

I mean that with the implementation in your prototype, calling split_at(0) will return Bytes::new() as first, which means that the address of the zero-length slice returned by Deref may not be the same as the base address used by the original Bytes object that got split.

@joshlf
Copy link
Author

joshlf commented Oct 14, 2024

Gotcha. That may be a non-starter then - we rely on the stability property to guarantee that if we first perform an alignment/size check and then split, the alignment and size will be the same after the split as before. It might be possible to work around (it would not be possible if you were saying that calling deref could change the address/length, but it doesn't sound like that's the case), but I'd need to confirm that we could shift the order in which we perform validation, and in any case it might pessimize things to have to unconditionally split before checking. It'd probably also result in very surprising behavior for callers if they validate alignment first and then call a method they expect to succeed (in fairness, who is doing that for ZSTs? Probably nobody).

@seanmonstar
Copy link
Member

Mostly that if a split results in a 0-length slice of Bytes, we return Bytes::new(), which will be a different address than the actual bytes:

pub const fn new() -> Self {

@Darksonn
Copy link
Contributor

We can probably change split_off to return a static Bytes at the right address.

@joshlf
Copy link
Author

joshlf commented Oct 16, 2024

That'd be great if it's on the table!

@Darksonn
Copy link
Contributor

Is there anything else you need us to guarantee?

@joshlf
Copy link
Author

joshlf commented Oct 17, 2024

It sounds like Bytes and BytesMut already guarantee what we need (when combined with #740). Take a skim and see if there's anything I've missed? I'm pretty sure these are satisfied.

ByteSlice safety requirements:

Screenshot 2024-10-16 at 5 53 25 PM

SplitByteSlice safety requirements:

Screenshot 2024-10-16 at 5 54 19 PM

@Darksonn
Copy link
Contributor

That looks okay to me.

@joshlf
Copy link
Author

joshlf commented Oct 17, 2024

Great! Then as soon as #740 is published on crates.io, we can add a feature to support bytes.

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.

3 participants