Re: [pbs-devel] [PATCH pxar 2/2] decoder: aio: improve performance of async file reads

["Fabian Grünbichler" <f.gruenbichler@proxmox.com>]

On July 20, 2023 7:15 pm, Max Carrara wrote:
> In order to bring `aio::Decoder` on par with its `sync` counterpart
> as well as `sync::Accessor` and `aio::Accessor`, its input is now
> buffered.
> 
> As the `tokio` docs mention themselves [0], it can be really
> inefficient to directly work with an (unbuffered) `AsyncRead`
> instance.
> 
> The other aforementioned types already buffer their reads in one way
> or another, so wrapping the input reader in `tokio::io::BufReader`
> results in a substantial performance gain. [1]
> 
> [0]: https://docs.rs/tokio/1.29.1/tokio/io/struct.BufReader.html
> [1]: Tested via a custom CLI utility that opens and traverses a
>      large (13GB) pxar archive with each decoder and accessor

would it maybe make sense to include that CLI utility as an example
binary? it might be helpful for future work in that area, to actively
look for regressions..

> Before:
>> First pass
>> With aio::Decoder:   Ok(()) (elapsed: 25.827150007s)
>> With sync::Decoder:  Ok(()) (elapsed: 3.577611655s)
>> With aio::Accessor:  Ok(()) (elapsed: 3.962754675s)
>> With sync::Accessor: Ok(()) (elapsed: 3.961245996s)
>>
>> Second pass
>> With aio::Decoder:   Ok(()) (elapsed: 21.045064325s)
>> With sync::Decoder:  Ok(()) (elapsed: 3.644003471s)
>> With aio::Accessor:  Ok(()) (elapsed: 4.054085818s)
>> With sync::Accessor: Ok(()) (elapsed: 4.036097687s)
> 
> After:
>> First pass:
>> With aio::Decoder:   Ok(()) (elapsed: 7.07321221s)
>> With sync::Decoder:  Ok(()) (elapsed: 3.431787191s)
>> With aio::Accessor:  Ok(()) (elapsed: 3.930457465s)
>> With sync::Accessor: Ok(()) (elapsed: 4.007415416s)
>>
>> Second pass:
>> With aio::Decoder:   Ok(()) (elapsed: 6.826005792s)
>> With sync::Decoder:  Ok(()) (elapsed: 3.437391887s)
>> With aio::Accessor:  Ok(()) (elapsed: 3.833275725s)
>> With sync::Accessor: Ok(()) (elapsed: 3.909827322s)

how were the numbers with the default buffer size of 8k? what was the
underlying storage like?

in general this seems like a good idea to me, not sure about the 16k vs
built-in default below, without more data to back it up ;)

in case you respin, IMHO patch 1 can be folded in, the patch here is
small enough and the Cargo.toml change only makes sense together with
it.. if you add the example binary, that can be a separate commit :)

> Signed-off-by: Max Carrara <m.carrara@proxmox.com>
> ---
>  src/decoder/aio.rs | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/src/decoder/aio.rs b/src/decoder/aio.rs
> index 200dd3d..174551b 100644
> --- a/src/decoder/aio.rs
> +++ b/src/decoder/aio.rs
> @@ -79,14 +79,20 @@ mod tok {
>      use std::pin::Pin;
>      use std::task::{Context, Poll};
> 
> -    /// Read adapter for `futures::io::AsyncRead`
> +    use tokio::io::AsyncRead;
> +
> +    /// Read adapter for `tokio::io::AsyncRead`
>      pub struct TokioReader<T> {
> -        inner: T,
> +        inner: tokio::io::BufReader<T>,
>      }
> 
>      impl<T: tokio::io::AsyncRead> TokioReader<T> {
>          pub fn new(inner: T) -> Self {
> -            Self { inner }
> +            // buffer size "sweet spot" - larger sizes don't seem to provide any benefit
> +            const BUF_SIZE: usize = 1024 * 16;
> +            Self {
> +                inner: tokio::io::BufReader::with_capacity(BUF_SIZE, inner),
> +            }
>          }
>      }
> 
> --
> 2.39.2
> 
> 
> 
> _______________________________________________
> pbs-devel mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
> 
> 
>