Specialize Bytes<R>::next when R is a BufReader.

[nnethercote]

This reduces the runtime for a simple program using Bytes::next to iterate through a file from 220ms to 70ms on my Linux box.

r? @the8472

[nnethercote]

cc @Shnatsel

One weird thing: this effectively undoes #116775, because it removes the #[inline] from Bytes::next.

• With the #[inline] my test program took 140ms and 724M instructions.
• Without the #[inline] it took 70ms and 644M instructions.

Cachegrind tells me that in both cases that Bytes::next is being inlined. (Runtime of the test program jumps dramatically if it's not inlined.) But somehow the presence/absence of that #[inline] makes a slight difference to the codegen of Buffer::buffer, which is enough to shift the performance in this way. Weird, and disconcerting. I would prefer to have #[inline] on Bytes::next, even though it only contains a single function call.

[the8472]

Looks fine overall, just a few style questions

[nnethercote]

New code is up. It adds comments, and moves the impl block like you suggested. That also let me remove the #[cold] out-of-line function. I still get bad results with #[inline] on Bytes::next so I added a comment about that.

For posterity, here is the test program I was using:

use std::io::Read;

fn main() -> Result<(), Box<dyn std::error::Error>> {
    let test_file_path = std::env::args_os().nth(1).expect("No input file specified!");
    let test_file = std::fs::File::open(test_file_path)?;
    let reader = std::io::BufReader::new(test_file);

    let sum: u64 = reader.bytes().map(|i| i.unwrap() as u64).sum();
    println!("{sum}");
    Ok(())
}

And here is an alternative using try_fold that had exactly the same performance:

use std::io::Read;

fn main() -> Result<(), Box<dyn std::error::Error>> {
    let test_file_path = std::env::args_os().nth(1).expect("No input file specified!");
    let test_file = std::fs::File::open(test_file_path)?;
    let reader = std::io::BufReader::new(test_file);

    fn f(sum: u64, i: Result<u8, std::io::Error>) -> Option<u64> {
        Some(sum + (i.unwrap() as u64))
    }
    let sum = reader.bytes().try_fold(0u64, f).unwrap();
    println!("{sum}");
    Ok(())
}

My test file was the output of running git log on the rust-lang/rust repo, which was 80 MB.

[nnethercote]

New code is up, addressing all review comments.

[the8472]

@bors r+ rollup=never

Looks good.

It might be possible to extend the specialization to some other Read impls like StdinLock.

Do people use Bytes a lot in the wild? The Result-per-byte approach is quite awkward.

[bors]

📌 Commit 181ce63 has been approved by the8472

It is now in the queue for this repository.

[nnethercote]

It might be possible to extend the specialization to some other Read impls like StdinLock.

Do people use Bytes a lot in the wild? The Result-per-byte approach is quite awkward.

I don't know. I didn't know it existed until @Shnatsel complained in nnethercote/perf-book#69. I think it's reasonable to not do anything further until someone complains.

[Shnatsel]

A quick search on Github shows 5.4k hits. The number can be both higher and lower because this query excludes imports of std::io::{Read, Write} and other such imports, but also it seems not all hits contain call to this function named .bytes() - the name is actually rather common.

I recalled the method from my first Rust project, which was a tr clone in Rust. It was multiple times slower than GNU tr implementation, but at the time I couldn't figure out why. Now I know!

Result-per-byte is actually very convenient, you just write a for loop and use the ? operator to deal with the result. Not great for performance though 😅

Specializing for StdinLock also sounds like a good idea to me. It's unfortunate that this cannot be specialized for any BufRead impl.

[Shnatsel]

To put it in perspective, there are 23k uses of read_to_end() and 31k uses of .read_to_string() for the same query. So at an eighth of the usage of read_to_end() (assuming half the hits are from other functions named .bytes()), it is not super popular but is not exceedingly rare either.

[bors]

⌛ Testing commit 181ce63 with merge e092502...

[rust-log-analyzer]

The job aarch64-gnu failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

[RUSTC-TIMING] process_spawning test:true 1.304
[RUSTC-TIMING] env test:true 1.468
[RUSTC-TIMING] stdbenches test:true 3.041
[RUSTC-TIMING] test test:true 15.895
##[error]The runner has received a shutdown signal. This can happen when the runner service is stopped, or a manually started runner is canceled.
##[group]Clock drift check
  local time: Fri Oct 20 00:01:50 UTC 2023
  local time: Fri Oct 20 00:01:50 UTC 2023
Session terminated, killing shell... ...killed.
##[error]The operation was canceled.
Cleaning up orphan processes

[bors]

💔 Test failed - checks-actions

[nnethercote]

@bors retry

[bors]

⌛ Testing commit 181ce63 with merge 029d00c...

[bors]

☀️ Test successful - checks-actions
Approved by: the8472
Pushing 029d00c to master...

[rust-timer]

Finished benchmarking commit (029d00c): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.6%	[-0.6%, -0.6%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.6%	[-0.6%, -0.6%]	1

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	4.4%	[4.4%, 4.4%]	1
Improvements ✅ (primary)	-6.1%	[-6.6%, -5.5%]	2
Improvements ✅ (secondary)	-3.9%	[-3.9%, -3.9%]	1
All ❌✅ (primary)	-6.1%	[-6.6%, -5.5%]	2

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	0.5%	[0.5%, 0.5%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-4.3%	[-10.4%, -1.8%]	6
All ❌✅ (primary)	-	-	0

Binary size

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	0.0%	[0.0%, 0.1%]	4
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.1%	[-0.1%, -0.1%]	2
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.0%	[-0.1%, 0.1%]	6

Bootstrap: 630.044s -> 631.512s (0.23%)
Artifact size: 304.02 MiB -> 304.04 MiB (0.01%)