* Re: [pbs-devel] [PATCH pxar 2/2] decoder: aio: improve performance of async file reads
2023-07-20 17:15 ` [pbs-devel] [PATCH pxar 2/2] decoder: aio: improve performance of async file reads Max Carrara
@ 2023-07-27 8:50 ` Fabian Grünbichler
2023-08-04 11:27 ` Wolfgang Bumiller
1 sibling, 0 replies; 4+ messages in thread
From: Fabian Grünbichler @ 2023-07-27 8:50 UTC (permalink / raw)
To: Proxmox Backup Server development discussion
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
>
>
>
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [pbs-devel] [PATCH pxar 2/2] decoder: aio: improve performance of async file reads
2023-07-20 17:15 ` [pbs-devel] [PATCH pxar 2/2] decoder: aio: improve performance of async file reads Max Carrara
2023-07-27 8:50 ` Fabian Grünbichler
@ 2023-08-04 11:27 ` Wolfgang Bumiller
1 sibling, 0 replies; 4+ messages in thread
From: Wolfgang Bumiller @ 2023-08-04 11:27 UTC (permalink / raw)
To: Max Carrara; +Cc: pbs-devel
On Thu, Jul 20, 2023 at 07:15:05PM +0200, 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.
Sure, but the question is *where* does it truly make sense to do the
buffering, more below...
(...)
> ---
> 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> {
^ This is a very generic interface here...
> - inner: T,
> + inner: tokio::io::BufReader<T>,
> }
>
> impl<T: tokio::io::AsyncRead> TokioReader<T> {
> pub fn new(inner: T) -> Self {
Note that `tokio`'s `BufReader` itself also implements `AsyncRead`, and
the user may already have a buffered reader here.
A better choice for us here would be to perform this change with the
`tokio-fs` feature and replace the
impl Decoder<TokioReader<tokio::fs::File>> {
fn open(...) -> io::Result<Self> { ... }
}
(which exists only so that `Decoder::open` can be used by the crate
consumer easily, automatically producing a `Decoder` for "some file
type"...)
with:
impl Decoder<TokioReader<BufReader<tokio::fs::File>>> {
fn open(...) -> io::Result<Self> { ... }
}
Since this is the place where we *actually* should be creating the
buffered reader.
> - Self { inner }
> + // buffer size "sweet spot" - larger sizes don't seem to provide any benefit
> + const BUF_SIZE: usize = 1024 * 16;
And we also wouldn't have to decide on what would be a sane size here
with the assumption that it is the right size for any possible T we
instantiate the decoder with.
There's a bit of a danger with sprinkling `BufReaders` in generic `T:
Read` APIs, as this may lead to multiple of those getting chained
together.
Eg. a consumer of the crate may instantiate a
`Decoder<SomeNetworkFile<TlsStreamThing>>`.
Then reads that buffering for such things can improve performance and
turn that into: `Decoder<SomeNetworkFile<BufReader<TlsStreamThing>>>`.
Little do they know that `Decoder` buffers, the creator of
`SomeNetworkFile` also thought the same thing and buffers as well, and
`TlsStreamThing` might also need buffering for a sane implementation, and
suddenly you're just chaining memcpys across 4 buffers before they end
up at the destination ;-)
> + Self {
> + inner: tokio::io::BufReader::with_capacity(BUF_SIZE, inner),
> + }
> }
> }
>
> --
> 2.39.2
^ permalink raw reply [flat|nested] 4+ messages in thread