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

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

On July 31, 2023 3:34 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]
> 
> `tokio/io-util` is added as dependency in order to use
> `tokio::io::BufReader`.
> 
> [0]: https://docs.rs/tokio/1.29.1/tokio/io/struct.BufReader.html
> [1]: Tested via examples/compare-read.rs on a large (13GB) pxar archive
> 
> Before:
>> PXAR Read Performance Comparison
>> Running in mode: release
>>
>> First pass:
>> With aio::Decoder:   Ok(()) (elapsed: 20.532270177s)
>> With sync::Decoder:  Ok(()) (elapsed: 3.498566141s)
>> With aio::Accessor:  Ok(()) (elapsed: 3.978160609s)
>> With sync::Accessor: Ok(()) (elapsed: 3.885640895s)
>>
>> Second pass:
>> With aio::Decoder:   Ok(()) (elapsed: 18.648986266s)
>> With sync::Decoder:  Ok(()) (elapsed: 3.617167922s)
>> With aio::Accessor:  Ok(()) (elapsed: 4.083678211s)
>> With sync::Accessor: Ok(()) (elapsed: 4.103763507s)
> 
> After:
>> PXAR Read Performance Comparison
>> Running in mode: release
>>
>> First pass:
>> With aio::Decoder:   Ok(()) (elapsed: 9.546522171s)
>> With sync::Decoder:  Ok(()) (elapsed: 3.535062119s)
>> With aio::Accessor:  Ok(()) (elapsed: 3.926439101s)
>> With sync::Accessor: Ok(()) (elapsed: 3.905232916s)
>>
>> Second pass:
>> With aio::Decoder:   Ok(()) (elapsed: 10.633561678s)
>> With sync::Decoder:  Ok(()) (elapsed: 3.528989778s)
>> With aio::Accessor:  Ok(()) (elapsed: 3.831093917s)
>> With sync::Accessor: Ok(()) (elapsed: 3.848684845s)

this does look good to me in general, do you have more details about
your test pxar file?

because for me with a big archive with lots of hardlinks (POM-created
mirror):

buffered:
Time (mean ± σ):     17.360 s ±  0.769 s    [User: 2.460 s, System: 14.345 s]
  Range (min … max):   16.004 s … 18.225 s    10 runs

stock:
Time (mean ± σ):     20.512 s ±  1.248 s    [User: 3.158 s, System: 16.510 s]
  Range (min … max):   19.045 s … 22.176 s    10 runs

Summary
  buffered ran
    1.18 ± 0.09 times faster than stock

and for another even bigger (~40G) archive consisting of a PBS .chunks
dir:

buffered:
Time (mean ± σ):     138.329 s ±  3.627 s    [User: 19.407 s, System: 114.824 s]
  Range (min … max):   134.266 s … 146.754 s    10 runs

stock:
  Time (mean ± σ):     179.822 s ±  3.679 s    [User: 26.894 s, System: 144.526 s]
  Range (min … max):   173.166 s … 186.505 s    10 runs

Summary
  buffered ran
    1.30 ± 0.04 times faster than stock

which, while an obvious improvement, is far from your almost 2x speedup
;)

> 
> Signed-off-by: Max Carrara <m.carrara@proxmox.com>
> ---
>  Changes v1 --> v2:
>   * Include addition of `tokio/io-util` as dependency
>   * Use new examples/compare-read.rs instead of old custom tool to
>     measure performance impact
>   * Use default buffer size (8K) instead of 16K
>     (I wasn't able to reproduce the performance gains, so ...)
> 
>  Cargo.toml         |  2 +-
>  src/decoder/aio.rs | 10 +++++++---
>  2 files changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/Cargo.toml b/Cargo.toml
> index 8669e30..08c0973 100644
> --- a/Cargo.toml
> +++ b/Cargo.toml
> @@ -63,7 +63,7 @@ libc = "0.2"
> 
>  [features]
>  default = [ "tokio-io" ]
> -tokio-io = [ "tokio" ]
> +tokio-io = [ "tokio", "tokio/io-util" ]
>  tokio-fs = [ "tokio-io", "tokio/fs" ]
> 
>  full = [ "tokio-fs"]
> diff --git a/src/decoder/aio.rs b/src/decoder/aio.rs
> index 200dd3d..7cb9c12 100644
> --- a/src/decoder/aio.rs
> +++ b/src/decoder/aio.rs
> @@ -79,14 +79,18 @@ 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 }
> +            Self {
> +                inner: tokio::io::BufReader::new(inner),
> +            }
>          }
>      }
> 
> --
> 2.39.2
> 
> 
> 
> _______________________________________________
> pbs-devel mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
> 
> 
>