Skip to content

Fix quadratic behavior of repeated vectored writes #121938

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 1 commit into from
Mar 8, 2024

Conversation

blyxxyz
Copy link
Contributor

@blyxxyz blyxxyz commented Mar 3, 2024

Some implementations of Write::write_vectored in the standard library (BufWriter, LineWriter, Stdout, Stderr) check all buffers to calculate the total length. This is O(n) over the number of buffers.

It's common that only a limited number of buffers is written at a time (e.g. 1024 for writev(2)). write_vectored_all will then call write_vectored repeatedly, leading to a runtime of O(n²) over the number of buffers.

This fix is to only calculate as much as needed if it's needed.

Here's a test program:

#![feature(write_all_vectored)]

use std::fs::File;
use std::io::{BufWriter, IoSlice, Write};
use std::time::Instant;

fn main() {
    let buf = vec![b'\0'; 100_000_000];
    let mut slices: Vec<IoSlice<'_>> = buf.chunks(100).map(IoSlice::new).collect();
    let mut writer = BufWriter::new(File::create("/dev/null").unwrap());

    let start = Instant::now();
    write_smart(&slices, &mut writer);
    println!("write_smart(): {:?}", start.elapsed());

    let start = Instant::now();
    writer.write_all_vectored(&mut slices).unwrap();
    println!("write_all_vectored(): {:?}", start.elapsed());
}

fn write_smart(mut slices: &[IoSlice<'_>], writer: &mut impl Write) {
    while !slices.is_empty() {
        // Only try to write as many slices as can be written
        let res = writer
            .write_vectored(slices.get(..1024).unwrap_or(slices))
            .unwrap();
        slices = &slices[(res / 100)..];
    }
}

Before this change:

write_smart(): 6.666952ms
write_all_vectored(): 498.437092ms

After this change:

write_smart(): 6.377158ms
write_all_vectored(): 6.923412ms

LineWriter (and by extension Stdout) isn't fully repaired by this because it looks for newlines. I could open an issue for that after this is merged, I think it's fixable but not trivially.

Some implementations of `Write::write_vectored` in the standard
library (`BufWriter`, `LineWriter`, `Stdout`, `Stderr`) check all
buffers to calculate the total length. This is O(n) over the number of
buffers.

It's common that only a limited number of buffers is written at a
time (e.g. 1024 for `writev(2)`). `write_vectored_all` will then call
`write_vectored` repeatedly, leading to a runtime of O(n²) over the
number of buffers.

The fix is to only calculate as much as needed if it's needed.
@rustbot
Copy link
Collaborator

rustbot commented Mar 3, 2024

r? @Amanieu

rustbot has assigned @Amanieu.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Mar 3, 2024
@Amanieu
Copy link
Member

Amanieu commented Mar 7, 2024

@bors r+

@bors
Copy link
Collaborator

bors commented Mar 7, 2024

📌 Commit 8212fc5 has been approved by Amanieu

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 7, 2024
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 8, 2024
…r=Amanieu

Fix quadratic behavior of repeated vectored writes

Some implementations of `Write::write_vectored` in the standard library (`BufWriter`, `LineWriter`, `Stdout`, `Stderr`) check all buffers to calculate the total length. This is O(n) over the number of buffers.

It's common that only a limited number of buffers is written at a time (e.g. 1024 for `writev(2)`). `write_vectored_all` will then call `write_vectored` repeatedly, leading to a runtime of O(n²) over the number of buffers.

This fix is to only calculate as much as needed if it's needed.

Here's a test program:
```rust
#![feature(write_all_vectored)]

use std::fs::File;
use std::io::{BufWriter, IoSlice, Write};
use std::time::Instant;

fn main() {
    let buf = vec![b'\0'; 100_000_000];
    let mut slices: Vec<IoSlice<'_>> = buf.chunks(100).map(IoSlice::new).collect();
    let mut writer = BufWriter::new(File::create("/dev/null").unwrap());

    let start = Instant::now();
    write_smart(&slices, &mut writer);
    println!("write_smart(): {:?}", start.elapsed());

    let start = Instant::now();
    writer.write_all_vectored(&mut slices).unwrap();
    println!("write_all_vectored(): {:?}", start.elapsed());
}

fn write_smart(mut slices: &[IoSlice<'_>], writer: &mut impl Write) {
    while !slices.is_empty() {
        // Only try to write as many slices as can be written
        let res = writer
            .write_vectored(slices.get(..1024).unwrap_or(slices))
            .unwrap();
        slices = &slices[(res / 100)..];
    }
}
```
Before this change:
```
write_smart(): 6.666952ms
write_all_vectored(): 498.437092ms
```
After this change:
```
write_smart(): 6.377158ms
write_all_vectored(): 6.923412ms
```

`LineWriter` (and by extension `Stdout`) isn't fully repaired by this because it looks for newlines. I could open an issue for that after this is merged, I think it's fixable but not trivially.
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 8, 2024
…iaskrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#118623 (Improve std::fs::read_to_string example)
 - rust-lang#119365 (Add asm goto support to `asm!`)
 - rust-lang#120608 (Docs for std::ptr::slice_from_raw_parts)
 - rust-lang#121885 (Move generic `NonZero` `rustc_layout_scalar_valid_range_start` attribute to inner type.)
 - rust-lang#121938 (Fix quadratic behavior of repeated vectored writes)
 - rust-lang#122099 (Add  `#[inline]` to `BTreeMap::new` constructor)
 - rust-lang#122143 (PassWrapper: update for llvm/llvm-project@a3319371970b)

Failed merges:

 - rust-lang#122076 (Tweak the way we protect in-place function arguments in interpreters)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 8, 2024
…iaskrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#118623 (Improve std::fs::read_to_string example)
 - rust-lang#119365 (Add asm goto support to `asm!`)
 - rust-lang#120608 (Docs for std::ptr::slice_from_raw_parts)
 - rust-lang#121832 (Add new Tier-3 target: `loongarch64-unknown-linux-musl`)
 - rust-lang#121938 (Fix quadratic behavior of repeated vectored writes)
 - rust-lang#122099 (Add  `#[inline]` to `BTreeMap::new` constructor)
 - rust-lang#122103 (Make TAITs and ATPITs capture late-bound lifetimes in scope)
 - rust-lang#122143 (PassWrapper: update for llvm/llvm-project@a3319371970b)

Failed merges:

 - rust-lang#122076 (Tweak the way we protect in-place function arguments in interpreters)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit f586a79 into rust-lang:master Mar 8, 2024
@rustbot rustbot added this to the 1.78.0 milestone Mar 8, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Mar 8, 2024
Rollup merge of rust-lang#121938 - blyxxyz:quadratic-vectored-write, r=Amanieu

Fix quadratic behavior of repeated vectored writes

Some implementations of `Write::write_vectored` in the standard library (`BufWriter`, `LineWriter`, `Stdout`, `Stderr`) check all buffers to calculate the total length. This is O(n) over the number of buffers.

It's common that only a limited number of buffers is written at a time (e.g. 1024 for `writev(2)`). `write_vectored_all` will then call `write_vectored` repeatedly, leading to a runtime of O(n²) over the number of buffers.

This fix is to only calculate as much as needed if it's needed.

Here's a test program:
```rust
#![feature(write_all_vectored)]

use std::fs::File;
use std::io::{BufWriter, IoSlice, Write};
use std::time::Instant;

fn main() {
    let buf = vec![b'\0'; 100_000_000];
    let mut slices: Vec<IoSlice<'_>> = buf.chunks(100).map(IoSlice::new).collect();
    let mut writer = BufWriter::new(File::create("/dev/null").unwrap());

    let start = Instant::now();
    write_smart(&slices, &mut writer);
    println!("write_smart(): {:?}", start.elapsed());

    let start = Instant::now();
    writer.write_all_vectored(&mut slices).unwrap();
    println!("write_all_vectored(): {:?}", start.elapsed());
}

fn write_smart(mut slices: &[IoSlice<'_>], writer: &mut impl Write) {
    while !slices.is_empty() {
        // Only try to write as many slices as can be written
        let res = writer
            .write_vectored(slices.get(..1024).unwrap_or(slices))
            .unwrap();
        slices = &slices[(res / 100)..];
    }
}
```
Before this change:
```
write_smart(): 6.666952ms
write_all_vectored(): 498.437092ms
```
After this change:
```
write_smart(): 6.377158ms
write_all_vectored(): 6.923412ms
```

`LineWriter` (and by extension `Stdout`) isn't fully repaired by this because it looks for newlines. I could open an issue for that after this is merged, I think it's fixable but not trivially.
@blyxxyz blyxxyz deleted the quadratic-vectored-write branch March 8, 2024 12:17
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants