cpu load by prepare_uninitialized_buffer in async_read of tokio-io

[gin66]

The current implementation zeros out a buffer, which consumes a lot of time. In my application the CPU actually spends 86% in this routine, which only listens to a tls web socket stream. The traffic is not high (10-100 kB/s), but this buffer zeroing has a huge hit on the application performance.

The application makes use of the websocket crate, which imports tokio-io. It uses the framed interface, so the application has no direct access. Possibly there could be some (complex) solution be implemented to solve this problem. Best would be to add a feature-flag, which decides about zero out the buffer or not.

Any other solution ?

[carllerche]

Do you know which type’s AsyncRead implementation is doing this?

[gin66]

Actually I am not sure, which of those is used via the websocket crate. I am using websocket with TlsStream aka a wss:// address. Independent from this, manually zeroing should be avoided in general. Perhaps using bytes::BytesMut in tokio-io would avoid this problem totally. As I understand, zeroing is exclusively used to avoid lack of sensitive data (which I doubt makes sense in this context).

In the meantime I have solved it by avoiding tokio and the websocket-future. Instead I am using the sync-version of the websocket. Now the CPU load of the application is OK.

[carllerche]

The issue is most likely that some type that implements AsyncRead is not correctly forwarding the prepare_uninitialized_buffer implementation.

[olix0r]

i just did a quick search through the websocket crate and found:

https://github.com/cyderize/rust-websocket/blob/2253a0aee965e95290c0426b56d621f964e8dd04/src/stream.rs#L80-L84

perhaps this needs to be updated to proxy calls onto the reader?

[gin66]

If this would be sufficient, then IMHO overriding prepare_uninitialized() like below should help

impl<R, W> AsyncRead for ReadWritePair<R, W>
		where R: AsyncRead,
		      W: Write
{
    unsafe fn prepare_uninitialized_buffer(&self, _buf: &mut [u8]) -> bool {
      false
   }
}

But it doesn't. I have tried without effect:

      - 91,19% <tokio_io::framed::Framed<T, U> as futures::stream::Stream>::poll                                                      ▒
         - 91,16% <tokio_io::framed_read::FramedRead2<T> as futures::stream::Stream>::poll                                            ▒
            - 87,97% tokio_io::async_read::AsyncRead::read_buf                                                                        ▒
               - 87,88% <&'a mut T as tokio_io::async_read::AsyncRead>::prepare_uninitialized_buffer                                  ▒
                    <tokio_io::framed_write::FramedWrite2<T> as tokio_io::async_read::AsyncRead>::prepare_uninitialized_buffer        ▒
                  - <tokio_io::framed::Fuse<T, U> as tokio_io::async_read::AsyncRead>::prepare_uninitialized_buffer                   ▒
                     - 87,20% tokio_io::async_read::AsyncRead::prepare_uninitialized_buffer                                           ▒
                        - 67,59% core::iter::range::<impl core::iter::iterator::Iterator for core::ops::range::Range<A>>::next        ▒
                           + 32,56% <usize as core::iter::range::Step>::add_usize                                                     ▒
                           + 25,53% core::mem::swap                                                                                   ▒
                             1,23% core::cmp::impls::<impl core::cmp::PartialOrd for usize>::lt                                       ▒
                             0,81% core::num::<impl usize>::checked_add                                                               ▒
                          0,75% core::cmp::impls::<impl core::cmp::PartialOrd for usize>::lt                                          ▒
                       0,68% core::iter::range::<impl core::iter::iterator::Iterator for core::ops::range::Range<A>>::next            ▒
            + 2,12% <tokio_io::framed_write::FramedWrite2<T> as tokio_io::codec::decoder::Decoder>::decode                            ▒
            + 0,77% <fern::log_impl::Dispatch as log::Log>::log                                                                       ▒

If the CPU hogging can be fixed in websocket, then I can raise an issue there. But I think, the zeroing is not good in tokio-io in the first place.

[carllerche]

Zeroing out is required for safety. It would be unsafe to pass in uninitialized memory. This is why implementations of AsyncRead must opt-in to the unsafety.

If all your AsyncRead implementations opt-into uninitialized buffers, then there will be no zeroing out.

[kpp]

Zeroing out is required for safety. It would be unsafe to pass in uninitialized memory.

Why? AsyncRead actually makes a write op into the given region of memory inside the buffer. So there is no need to set it to 0 before writeing actually a "good" data inside it.

It is forbidden to read uninitialized data, but nobody reads it before the AsyncRead operation completes (if your BufMut data struct correctly handles this situation).

[carllerche]

@kpp std::io::Ready has the same problem. You can see here: rust-lang/rust#42002

The problem is that it is technically possible to read the data in a read fn, which is silly, but still possible. Because of this, you enter unsafe territory.

[carllerche]

It is forbidden to read uninitialized data

It is not forbidden by the compiler. This is the problem.

[gin66]

Why not use one of these alternatives:

• Provide an interface to hand in a buffer factory like the pool crate, which even helps to avoid allocations (comes close to tokio's bare metal performance claim). Leave the decision making to the tokio user
• Give documentation hints, how to opt out the performance hog of buffer zeroing out
• Mandatory opt-in for tokio-io user to use slow safe or fast unsafe code
• Rely on BufMut, which may be used anyway at other places in the application unsafely

Anyway I do not understand the problem in this case. The malicious user is the application tokio-io is linked in. If it cannot be trusted, then what else to trust ?

BTW: If the tokio-io user (aka websocket crate) has a means to avoid the zeroing, then I miss totally the point of safety. If the application cannot be trusted, then for sure the application will not use tokio-io's zeroing. If it is about safety for the application, then application should not use unsafe ways to read out data outside of the filled in data in the BufMut

[kpp]

then application should not use unsafe ways to read out data outside of the filled in data in the BufMut

👍

[carllerche]

@gin66 The malicious user is not the application using tokio-io

When safety is discussed here, it is in terms of Rust's memory model. Informally, when writing Rust code, it should be impossible to be able to read uninitialized memory without writing unsafe. So, Tokio will never pass uninitialized memory into a read fn w/o that implementation opting in to it w/ an unsafe block.

However, this discussion isn't limited to Tokio, and if you wish to continue it, I'd recommend rust-lang/rust#42788. Tokio will end up following what the Rust team goes with.

[gin66]

@carllerche Thanks for clarification. Before I have thought, that BufMut prohibits access to uninitialized area except unsafe is used. Need to read the code more detailed and learn rust better.

My current solution is to use the synchronous version of websocket and enjoy single digit CPU load. For me is OK to wait for that rust-discussion and close this issue here.

[carllerche]

@gin66 To be clear, if large amounts of CPU is being used to zero out memory, it is a bug, but it would be in the library that has a type that incorrectly implements AsyncRead.