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 no_std support, by adding an std feature #281

Merged
merged 5 commits into from Sep 5, 2019

Conversation

cbeck88
Copy link
Contributor

@cbeck88 cbeck88 commented Aug 13, 2019

I was really hopeful initially that this could be done without
having an std feature, but it turns out that bytes has a bunch
of special additional integration with std::io stuff, e.g.
std::io::Reader and std::io::IoSlice.

To make the library work as no_std we add an std feature which
is on by default. When it is off, we compile as no_std and make
parts of the API that require std::io conditional on the std
feature.

@cbeck88
Copy link
Contributor Author

cbeck88 commented Aug 13, 2019

Hi -- I'm interested in getting bytes to work and build as a no_std library relying on the newly-stabilized alloc crate to get Vec etc.

My motivation for doing this is that we want to use prost for serialization inside of an SGX enclave. Inside SGX you may have an allocator but you can't talk to the OS directly. bytes is a dependency of prost.

I think it's pretty unlikely that all of tokio could be ported inside of sgx, but if you are interested in no_std support in some of these low-hanging dependencies like bytes that are really fundamental, it will make them maximally portable and help the ecosystem.

LMK if you are interested in a patch like this! Thanks

@tarcieri
Copy link

@cbeck88 you should probably check out:

#269
#256
#255
#153
#135

@cbeck88
Copy link
Contributor Author

cbeck88 commented Aug 15, 2019

@tarcieri My PR is very simple -- alloc has been standardized, and master is already on edition 2018. So the only thing we have to do is get things from the right namespace, and feature-gate the use of std-only trait bounds. This is a simple and mechanical change. This is not new functionality or code change at all, so it's hopefully not controversial.

#269 seems orthogonal and way beyond my goals. there's no reason we can't land this and then that one later.
#256 and #255 are both stale since iovec was dropped. They also do some complex stuff with

#[cfg(feature = "std")]
use prelude::*;

that seems totally unnecessary.

#153 and #135 are both from 2 years ago. Is there anything from these threads in particular that you want to draw attention to? They look moot to me.

@tarcieri
Copy link

I pointed it out to note that there are other open PRs for this same general issue, for the ample historical discussion if nothing else. #269 describes solutions which can work for heapless no_std users (which this PR doesn’t help)

re: a crate-local prelude

that seems totally unnecessary

It’s necessary for compatibility with Rust versions prior to 1.35. It’s certainly much cleaner to support 1.35+, and I’m not sure what MSRV plans are for bytes 0.5.

@cbeck88
Copy link
Contributor Author

cbeck88 commented Aug 15, 2019

@tarcieri I see -- I had mistakenly thought that edition: 2018 already put us past 1.35
Yeah if MSRV for bytes 0.5 is 1.35 or less then my patch needs more work

@tarcieri
Copy link

Perhaps @carllerche can tell us what he's thinking re: bytes 0.5 MSRV?

@taiki-e
Copy link
Member

taiki-e commented Aug 15, 2019

MSRV of Bytes 0.5 is 1.36.

# This represents the minimum Rust version supported by
# Bytes. Updating this should be done in a dedicated PR.
#
# Tests are not run as tests may require newer versions of
# rust.
- template: ci/azure-test-stable.yml
parameters:
name: minrust
rust_version: 1.36.0

@DoumanAsh
Copy link
Contributor

@cbeck88 Could you please resolve current conflicts?

Master branch targets 2018 so it is perfectly fine as PR.

I'm still not sure what sort of no_std support we should have, but I believe support for global allocator would be good first step

src/lib.rs Outdated Show resolved Hide resolved
src/bytes.rs Outdated Show resolved Hide resolved
I was really hopeful initially that this could be done without
having an `std` feature, but it turns out that `bytes` has a bunch
of special additional integration with `std::io` stuff, e.g.
`std::io::Reader` and `std::io::IoSlice`.

To make the library work as `no_std` we add an `std` feature which
is on by default. When it is off, we compile as `no_std` and make
parts of the API that require `std::io` conditional on the `std`
feature.
@DoumanAsh
Copy link
Contributor

@cbeck88 I made some changes to your PR.
I believe we don't need to link against alloc when std is already present

@cbeck88
Copy link
Contributor Author

cbeck88 commented Aug 31, 2019

@DoumanAsh

I believe we don't need to link against alloc when std is already present

okay, but to my understanding there is no separate target to "link against". the symbols exported in std are simply re-exports of what is in alloc. so i think it's simpler to just always take the symbols from alloc and not do if std use std::..., because that just adds boilerplate that literally doesn't do anything.

that said, i'm happy with these changes if they make this PR acceptable to reviewers

also apparently there's more going on here than I realize per @tarcieri playground example so maybe this has some subtle effect

@DoumanAsh
Copy link
Contributor

Hm maybe you're right, I usually do not link to alloc because it is a bit unclear about it's status when building in std enviornment

@cbeck88
Copy link
Contributor Author

cbeck88 commented Sep 3, 2019

@tarcieri @DoumanAsh let me know if you guys see anything else to improve here, i don't have plans to make further changes at this time

src/buf/vec_deque.rs Outdated Show resolved Hide resolved
src/bytes.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
Copy link

@tarcieri tarcieri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great now!

@cbeck88
Copy link
Contributor Author

cbeck88 commented Sep 7, 2019

@tarcieri i realized that my idea about moving Buf and BufMut to a smaller crate will hit a problem after this recent commit:

b6cb346#diff-c423debf79de47ec6dd3f075e0fc9cdcR901

    /// Consumes remaining bytes inside self and returns new instance of `Bytes`
    ///
    /// # Examples
    ///
    /// ```
    /// use bytes::{Buf};
    ///
    /// let bytes = "hello world".to_bytes();
    /// assert_eq!(&bytes[..], &b"hello world"[..]);
    /// ```
    fn to_bytes(&mut self) -> crate::Bytes {
        use super::BufMut;
        let mut ret = crate::BytesMut::with_capacity(self.remaining());
        ret.put(self);
        ret.freeze()
    }

because now trait Buf has a dependency back on the Bytes type, which would be circular if they were two crates

I've been trying to brainstorm a simple solution to this, because I don't want to sabotage long-term goals of core-only heapless version of this crate.

I think another approach might be, instead of buf::to_bytes() something like

impl Bytes {
   ...
   fn from_buf(arg: Buf) -> Bytes {
       let mut ret = BytesMut::with_capacity(arg.remaining());
       ret.put(arg);
       ret.freeze();
       ret
    }
}

or some such, because then Buf trait doesn't need to know about bytes

This has the downside, though, that you don't get the benefit described in commit message:

The advantage of Buf::into_bytes is that it can be optimized in cases where converting a T: Buf into a Bytes instance is efficient.

A different direction is, maybe bytes_core would contain simple versions of Buf and BufMut that don't have knowledge of Bytes type.

Then, bytes makes its own versions of Buf and BufMut which are defined as

pub trait Buf: bytes_core::Buf + Into<Bytes> {}

and this is what bytes exports as Buf and BufMut, to users of bytes crate. The implementors of buf can implement Into<Bytes> how they want, and Bytes can do the efficient thing. There might be a way to make it so that the default implementation is there but the user can specialize it...

But in heapless context you wouldn't try to have Bytes or Into<Bytes> at all, you would just use the bytes_core versions of traits and probably do some kind of allocator-aware thing as described in earlier PR's

IDK I don't have personal need of heapless bytes in my own projects right now but I'm interested to keep brainstorming, and interested what others think: @DoumanAsh ^

My feeling is that bytes crate Buf and BufMut objects are much nicer than the std::io stuff. I worked in embedded c++ development at tesla in their autopilot project for a year, and I ended up writing some very similar stuff to Buf and BufMut, but in C++, to help them do zero-copy logging and serialization in the critical path throughout their codebase. This part of bytes documentation speaks volumes to me:

At first glance, it may seem that Buf and BufMut overlap in functionality with std::io::Read and std::io::Write. However, they serve different purposes. A buffer is the value that is provided as an argument to Read::read and Write::write. Read and Write may then perform a syscall, which has the potential of failing. Operations on Buf and BufMut are infallible.

If I were ever to write a serialization lib in rust I would probably try to use bytes rather than the std::io stuff, just as prost did. And if I wanted it to not make dynamic memory allocations (which can be forbidden in safety-critical programming contexts), and use pre-sized arenas etc., then I guess I would probably want a core-only version of bytes::Buf and BufMut. So anyways I'm really sympathetic to this as a development goal. There should be a way to make it possible, I'm just not sure what the best way is.

@tarcieri
Copy link

tarcieri commented Sep 7, 2019

@cbeck88 right now the main solution we have for this particular problem in an embedded context is to used fixed-sized fields for everything in the protos, so e.g. fixed32 for all integers, and fixed-sized string/bytes, then more or less handrolling the parser/serializer to slice the data up or write it out to a fixed-sized buffer with constant protobuf framing. It'd sure be nice to find some way to leverage heapless in conjunction with prost though, so we could have prost autogenerate the parser/serializer, even for embedded.

@DoumanAsh
Copy link
Contributor

DoumanAsh commented Sep 7, 2019

@cbeck88 I'm not sure in particular what will be benefit of moving traits themself, but this particular memthod could be made through trait extension rather than moved. i.e. BufExt should be in the same crate as Bytes

@cbeck88
Copy link
Contributor Author

cbeck88 commented Sep 7, 2019

@DoumanAsh it's only to try to separate out the ideas and reduce the need for features to configure the package

it might be simpler to just say, there will be an std feature, and an alloc feature, and when neither are on we try to provide Buf and BufMut without Bytes type or related api

if we think that sounds better, it might be a good idea to add an alloc feature, even if it doesn't do anything right now, so that people can select it and it won't be a breaking change later if we make the core-only config work

@DoumanAsh
Copy link
Contributor

@cbeck88 I believe right now it should be quite simple to make the crate no_std compatible.
I don't really think we'll need a need for alloc feature because even if we link with it, it is not necessary to use it as long as you're not touch allocations.

I don't really see much harm in splitting but I'm myself unsure of whether we should fragment such small crate, I'd suggest to hop onto tokio gitter and discuss it

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 this pull request may close these issues.

None yet

5 participants