[pbs-devel] [PATCH v2 pxar 1/2] Add `compare-read.rs` to examples and drop feature `async-examples`

[pbs-devel] [PATCH v2 pxar 1/2] Add `compare-read.rs` to examples and drop feature `async-examples`

[Max Carrara]

`compare-read.rs` may be used to compare read speeds between pxar
accessors and decoders, both synchronous and asynchronous versions.

The `async-examples` feature is dropped in favour of declaring
`[dev-dependencies]` in `Cargo.toml`.

Signed-off-by: Max Carrara <m.carrara@proxmox.com>
---
 Changes v1 --> v2:
  * Remove addition of `tokio/io-util` as dependency
  * Add example `compare-read`
  * Remove feature `async-example` in favour of `[dev-dependencies]`

 Cargo.toml               |  20 +++---
 examples/compare-read.rs | 150 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 162 insertions(+), 8 deletions(-)
 create mode 100644 examples/compare-read.rs

diff --git a/Cargo.toml b/Cargo.toml
index d120e70..8669e30 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -15,7 +15,10 @@ exclude = [
 [[example]]
 name = "apxar"
 path = "examples/apxar.rs"
-required-features = [ "async-example" ]
+
+[[example]]
+name = "compare-read"
+path = "examples/compare-read.rs"

 [[example]]
 name = "pxarcmd"
@@ -47,6 +50,14 @@ siphasher = "0.3"

 tokio = { version = "1.0", optional = true, default-features = false }

+[dev-dependencies]
+anyhow = "1.0"
+tokio = { version = "1.0", default-features = false, features = [
+    "fs",
+    "macros",
+    "rt-multi-thread"
+] }
+
 [target.'cfg(target_os = "linux")'.dependencies]
 libc = "0.2"

@@ -57,11 +68,4 @@ tokio-fs = [ "tokio-io", "tokio/fs" ]

 full = [ "tokio-fs"]

-async-example = [
-    "tokio-io",
-    "tokio-fs",
-    "tokio/rt-multi-thread",
-    "tokio/macros",
-]
-
 test-harness = []
diff --git a/examples/compare-read.rs b/examples/compare-read.rs
new file mode 100644
index 0000000..c908077
--- /dev/null
+++ b/examples/compare-read.rs
@@ -0,0 +1,150 @@
+//! # Compare Read Speeds of Accessors and Decoders
+//!
+//! This example is used to compare read speeds between:
+//!
+//! * [`aio::Accessor`][pxar::accessor::aio::Accessor]
+//! * [`sync::Accessor`][pxar::accessor::sync::Accessor]
+//! * [`aio::Decoder`][pxar::decoder::aio::Decoder]
+//! * [`sync::Decoder`][pxar::decoder::sync::Decoder]
+//!
+//! ## Usage
+//!
+//! You may run this example directly on a PXAR archive:
+//!
+//! ```bash
+//! cargo run -q --example compare-read [FILE_PATH]
+//! cargo run -q --release --example compare-read [FILE_PATH]
+//! ```
+
+use std::ffi::OsStr;
+use std::future::Future;
+use std::time::Duration;
+
+use anyhow::{Context, Result};
+use pxar::accessor::aio::Accessor as AioAccessor;
+use pxar::accessor::sync::Accessor as SyncAccessor;
+use pxar::decoder::aio::Decoder as AioDecoder;
+use pxar::decoder::sync::Decoder as SyncDecoder;
+
+async fn read_with_decoder(file: &OsStr) -> Result<()> {
+    let file = tokio::fs::File::open(file)
+        .await
+        .context("failed to open file")?;
+
+    let mut reader = AioDecoder::from_tokio(file)
+        .await
+        .context("failed to open pxar archive contents")?;
+
+    let mut entries = vec![];
+
+    while let Some(entry) = reader.next().await {
+        let entry = entry.context("failed to parse entry")?;
+
+        entries.push(entry);
+    }
+
+    Ok(())
+}
+
+async fn read_with_decoder_sync(file: &OsStr) -> Result<()> {
+    let file = std::fs::File::open(file).context("failed to open file")?;
+
+    let reader = SyncDecoder::from_std(file).context("failed to open pxar archive contents")?;
+
+    let mut entries = vec![];
+
+    for entry in reader {
+        let entry = entry.context("failed to parse entry")?;
+        entries.push(entry);
+    }
+
+    Ok(())
+}
+
+async fn read_with_accessor(file: &OsStr) -> Result<()> {
+    let accessor = AioAccessor::open(file)
+        .await
+        .context("failed to open pxar archive contents")?;
+
+    let dir = accessor.open_root_ref().await?;
+    let mut decode_full = dir.decode_full().await?;
+
+    let mut entries = vec![];
+
+    while let Some(entry) = decode_full.next().await {
+        let entry = entry.context("failed to parse entry")?;
+
+        entries.push(entry);
+    }
+
+    Ok(())
+}
+
+async fn read_with_accessor_sync(file: &OsStr) -> Result<()> {
+    let accessor = SyncAccessor::open(file).context("failed to open pxar archive contents")?;
+
+    let dir = accessor.open_root_ref()?;
+    let decode_full = dir.decode_full()?;
+
+    let mut entries = vec![];
+
+    for entry in decode_full {
+        let entry = entry.context("failed to parse entry")?;
+
+        entries.push(entry);
+    }
+
+    Ok(())
+}
+
+async fn measure_duration<F, R>(future: F) -> (R, Duration)
+where
+    F: Future<Output = R>,
+    R: Send + 'static,
+{
+    use std::time::Instant;
+    let start = Instant::now();
+    let return_value = future.await;
+    let elapsed = start.elapsed();
+
+    (return_value, elapsed)
+}
+
+async fn run_reads(file: &OsStr) -> Result<()> {
+    let (result, elapsed) = measure_duration(read_with_decoder(&file)).await;
+    println!("With aio::Decoder:   {result:?} (elapsed: {elapsed:#?})");
+
+    let (result, elapsed) = measure_duration(read_with_decoder_sync(&file)).await;
+    println!("With sync::Decoder:  {result:?} (elapsed: {elapsed:#?})");
+
+    let (result, elapsed) = measure_duration(read_with_accessor(&file)).await;
+    println!("With aio::Accessor:  {result:?} (elapsed: {elapsed:#?})");
+
+    let (result, elapsed) = measure_duration(read_with_accessor_sync(&file)).await;
+    println!("With sync::Accessor: {result:?} (elapsed: {elapsed:#?})");
+
+    Ok(())
+}
+
+#[tokio::main]
+async fn main() -> Result<()> {
+    let mode = if cfg!(debug_assertions) {
+        "debug"
+    } else {
+        "release"
+    };
+
+    println!("PXAR Read Performance Comparison");
+    println!("Running in mode: {mode}\n");
+
+    let mut args = std::env::args_os().skip(1);
+
+    let file = args.next().context("expected file name")?;
+    println!("First pass:");
+    run_reads(&file).await?;
+
+    println!("\nSecond pass:");
+    run_reads(&file).await?;
+
+    Ok(())
+}
--
2.39.2

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

[Max Carrara]

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)

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

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

[Fabian Grünbichler]

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
> 
> 
> 

Re: [pbs-devel] [PATCH v2 pxar 1/2] Add `compare-read.rs` to examples and drop feature `async-examples`

[Fabian Grünbichler]

On July 31, 2023 3:34 pm, Max Carrara wrote:
> `compare-read.rs` may be used to compare read speeds between pxar
> accessors and decoders, both synchronous and asynchronous versions.
> 
> The `async-examples` feature is dropped in favour of declaring
> `[dev-dependencies]` in `Cargo.toml`.
> 
> Signed-off-by: Max Carrara <m.carrara@proxmox.com>
> ---

just a note for when this gets applied - both patches obviously change
d/control, so that needs a follow-up commit with the updated version

>  Changes v1 --> v2:
>   * Remove addition of `tokio/io-util` as dependency
>   * Add example `compare-read`
>   * Remove feature `async-example` in favour of `[dev-dependencies]`
> 
>  Cargo.toml               |  20 +++---
>  examples/compare-read.rs | 150 +++++++++++++++++++++++++++++++++++++++
>  2 files changed, 162 insertions(+), 8 deletions(-)
>  create mode 100644 examples/compare-read.rs
> 
> diff --git a/Cargo.toml b/Cargo.toml
> index d120e70..8669e30 100644
> --- a/Cargo.toml
> +++ b/Cargo.toml
> @@ -15,7 +15,10 @@ exclude = [
>  [[example]]
>  name = "apxar"
>  path = "examples/apxar.rs"
> -required-features = [ "async-example" ]
> +
> +[[example]]
> +name = "compare-read"
> +path = "examples/compare-read.rs"
> 
>  [[example]]
>  name = "pxarcmd"
> @@ -47,6 +50,14 @@ siphasher = "0.3"
> 
>  tokio = { version = "1.0", optional = true, default-features = false }
> 
> +[dev-dependencies]
> +anyhow = "1.0"
> +tokio = { version = "1.0", default-features = false, features = [
> +    "fs",
> +    "macros",
> +    "rt-multi-thread"
> +] }
> +
>  [target.'cfg(target_os = "linux")'.dependencies]
>  libc = "0.2"
> 
> @@ -57,11 +68,4 @@ tokio-fs = [ "tokio-io", "tokio/fs" ]
> 
>  full = [ "tokio-fs"]
> 
> -async-example = [
> -    "tokio-io",
> -    "tokio-fs",
> -    "tokio/rt-multi-thread",
> -    "tokio/macros",
> -]
> -
>  test-harness = []
> diff --git a/examples/compare-read.rs b/examples/compare-read.rs
> new file mode 100644
> index 0000000..c908077
> --- /dev/null
> +++ b/examples/compare-read.rs
> @@ -0,0 +1,150 @@
> +//! # Compare Read Speeds of Accessors and Decoders
> +//!
> +//! This example is used to compare read speeds between:
> +//!
> +//! * [`aio::Accessor`][pxar::accessor::aio::Accessor]
> +//! * [`sync::Accessor`][pxar::accessor::sync::Accessor]
> +//! * [`aio::Decoder`][pxar::decoder::aio::Decoder]
> +//! * [`sync::Decoder`][pxar::decoder::sync::Decoder]
> +//!
> +//! ## Usage
> +//!
> +//! You may run this example directly on a PXAR archive:
> +//!
> +//! ```bash
> +//! cargo run -q --example compare-read [FILE_PATH]
> +//! cargo run -q --release --example compare-read [FILE_PATH]
> +//! ```
> +
> +use std::ffi::OsStr;
> +use std::future::Future;
> +use std::time::Duration;
> +
> +use anyhow::{Context, Result};
> +use pxar::accessor::aio::Accessor as AioAccessor;
> +use pxar::accessor::sync::Accessor as SyncAccessor;
> +use pxar::decoder::aio::Decoder as AioDecoder;
> +use pxar::decoder::sync::Decoder as SyncDecoder;
> +
> +async fn read_with_decoder(file: &OsStr) -> Result<()> {
> +    let file = tokio::fs::File::open(file)
> +        .await
> +        .context("failed to open file")?;
> +
> +    let mut reader = AioDecoder::from_tokio(file)
> +        .await
> +        .context("failed to open pxar archive contents")?;
> +
> +    let mut entries = vec![];
> +
> +    while let Some(entry) = reader.next().await {
> +        let entry = entry.context("failed to parse entry")?;
> +
> +        entries.push(entry);
> +    }
> +
> +    Ok(())
> +}
> +
> +async fn read_with_decoder_sync(file: &OsStr) -> Result<()> {
> +    let file = std::fs::File::open(file).context("failed to open file")?;
> +
> +    let reader = SyncDecoder::from_std(file).context("failed to open pxar archive contents")?;
> +
> +    let mut entries = vec![];
> +
> +    for entry in reader {
> +        let entry = entry.context("failed to parse entry")?;
> +        entries.push(entry);
> +    }
> +
> +    Ok(())
> +}
> +
> +async fn read_with_accessor(file: &OsStr) -> Result<()> {
> +    let accessor = AioAccessor::open(file)
> +        .await
> +        .context("failed to open pxar archive contents")?;
> +
> +    let dir = accessor.open_root_ref().await?;
> +    let mut decode_full = dir.decode_full().await?;
> +
> +    let mut entries = vec![];
> +
> +    while let Some(entry) = decode_full.next().await {
> +        let entry = entry.context("failed to parse entry")?;
> +
> +        entries.push(entry);
> +    }
> +
> +    Ok(())
> +}
> +
> +async fn read_with_accessor_sync(file: &OsStr) -> Result<()> {
> +    let accessor = SyncAccessor::open(file).context("failed to open pxar archive contents")?;
> +
> +    let dir = accessor.open_root_ref()?;
> +    let decode_full = dir.decode_full()?;
> +
> +    let mut entries = vec![];
> +
> +    for entry in decode_full {
> +        let entry = entry.context("failed to parse entry")?;
> +
> +        entries.push(entry);
> +    }
> +
> +    Ok(())
> +}
> +
> +async fn measure_duration<F, R>(future: F) -> (R, Duration)
> +where
> +    F: Future<Output = R>,
> +    R: Send + 'static,
> +{
> +    use std::time::Instant;
> +    let start = Instant::now();
> +    let return_value = future.await;
> +    let elapsed = start.elapsed();
> +
> +    (return_value, elapsed)
> +}
> +
> +async fn run_reads(file: &OsStr) -> Result<()> {
> +    let (result, elapsed) = measure_duration(read_with_decoder(&file)).await;
> +    println!("With aio::Decoder:   {result:?} (elapsed: {elapsed:#?})");
> +
> +    let (result, elapsed) = measure_duration(read_with_decoder_sync(&file)).await;
> +    println!("With sync::Decoder:  {result:?} (elapsed: {elapsed:#?})");
> +
> +    let (result, elapsed) = measure_duration(read_with_accessor(&file)).await;
> +    println!("With aio::Accessor:  {result:?} (elapsed: {elapsed:#?})");
> +
> +    let (result, elapsed) = measure_duration(read_with_accessor_sync(&file)).await;
> +    println!("With sync::Accessor: {result:?} (elapsed: {elapsed:#?})");
> +
> +    Ok(())
> +}
> +
> +#[tokio::main]
> +async fn main() -> Result<()> {
> +    let mode = if cfg!(debug_assertions) {
> +        "debug"
> +    } else {
> +        "release"
> +    };
> +
> +    println!("PXAR Read Performance Comparison");
> +    println!("Running in mode: {mode}\n");
> +
> +    let mut args = std::env::args_os().skip(1);
> +
> +    let file = args.next().context("expected file name")?;
> +    println!("First pass:");
> +    run_reads(&file).await?;
> +
> +    println!("\nSecond pass:");
> +    run_reads(&file).await?;
> +
> +    Ok(())
> +}
> --
> 2.39.2
> 
> 
> 
> _______________________________________________
> pbs-devel mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
> 
> 
> 

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

[Max Carrara]

On 7/31/23 16:57, Fabian Grünbichler wrote:
> 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
> ;)
> 

The file I've used contains a couple test files from the sparse copy
bug (essentially files with random data and some holes) and a .tar of
the /var/log/mysql directory of a MariaDB instance I had used in an
attempt to create yet another test file.

Maybe we can exchange files? ;)

>>
>> 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
>>
>>
>>
> 
> 
> _______________________________________________
> pbs-devel mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

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

[Wolfgang Bumiller]

NAK

see comment for the previous v1

TLDR: buffering should be inserted on the consumer side (and our
`Decoder::open` impl should be changed to be for
`Decoder<TokioReader<BufReader<File>>` instead of
`Decoder<TokioReader<File>>`)
(which btw. is a breaking change API-wise)