Remove unnecessary lseek syscall when using std::fs::read

[chenyukang]

Fixes #106597
r? @bjorn3

[rustbot]

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

• Stabilizing library features
• Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
• Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
• Changing public documentation in ways that create new stability guarantees
• Changing observable runtime behavior of library APIs

[chenyukang]

The result from strace now is:

openat(AT_FDCWD, "./p/f.rs", O_RDONLY|O_CLOEXEC) = 3
statx(0, NULL, AT_STATX_SYNC_AS_STAT, STATX_ALL, NULL) = -1 EFAULT (Bad address)
statx(3, "", AT_STATX_SYNC_AS_STAT|AT_EMPTY_PATH, STATX_ALL, {stx_mask=STATX_ALL|0x1000, stx_attributes=0, stx_mode=S_IFREG|0644, stx_size=88, ...}) = 0
read(3, "fn main() {\n    let res = std::f"..., 88) = 88
read(3, "", 32)                         = 0
close(3)

[ShawnZhong]

buffer_capacity_required is also called by read_to_end.
If a file is already half-read, we don't really need to reserve the entire file size.

[ShawnZhong]

I'd suggest modifying std::fs::read to reserve the entire file size (since we just opened it, and the file offset is guaranteed to be 0) and call default_read_to_end there (instead of the current implementation which calls file.read_to_end)

[chenyukang]

buffer_capacity_required is also called by read_to_end. If a file is already half-read, we don't really need to reserve the entire file size.

Make sense.

[bjorn3]

r? rust-lang/libs

[bjorn3]

Thanks! I don't think I am a suitable reviewer, so I re-assigned it to someone on the libs team.

[klensy]

You can do let mut bytes = Vec::with_capacity(size as usize) instead of two lines reserve and Vec::new()

[chenyukang]

Yes, I thought on this, but I have a dig on the implementation of with_capacity and reserve, seems it's not simply share the same code path, so I'm not 100% sure is there any flaw to use with_capacity here.

I just have a simple bench test, seems with_capacity perform better:

cat@LAPTOP-V6U0QKD4:~/code/play/bench$ cargo bench
    Finished bench [optimized] target(s) in 0.00s
     Running unittests src/main.rs (target/release/deps/bench-b73bcfb176559323)

running 2 tests
test tests::bench_reserve_capacity ... bench:     677,683 ns/iter (+/- 828,000)
test tests::bench_with_capacity    ... bench:     469,924 ns/iter (+/- 319,682)

test result: ok. 0 passed; 0 failed; 0 ignored; 2 measured; 0 filtered out; finished in 10.56s

cat@LAPTOP-V6U0QKD4:~/code/play/bench$ cargo bench
    Finished bench [optimized] target(s) in 0.01s
     Running unittests src/main.rs (target/release/deps/bench-b73bcfb176559323)

running 2 tests
test tests::bench_reserve_capacity ... bench:     645,487 ns/iter (+/- 347,440)
test tests::bench_with_capacity    ... bench:     415,772 ns/iter (+/- 162,321)

test result: ok. 0 passed; 0 failed; 0 ignored; 2 measured; 0 filtered out; finished in 10.09s

with bench code:

#![feature(test)]
extern crate test;

pub fn test_with_capacity(size: usize) {
    let mut v: Vec<i32> = Vec::with_capacity(size);
    v.push(3);
    assert_eq!(v.len(), 1);
}

pub fn test_reserve_capacity(size: usize) {
    let mut v: Vec<i32> = Vec::new();
    v.reserve(size);
    v.push(3);
    assert_eq!(v.len(), 1);
}

#[cfg(test)]
mod tests {
    use super::*;
    use test::Bencher;

    #[bench]
    fn bench_with_capacity(b: &mut Bencher) {
        b.iter(|| {
            for s in 0..10000 {
                test_with_capacity(s);
            }
        });
    }

    #[bench]
    fn bench_reserve_capacity(b: &mut Bencher) {
        b.iter(|| {
            for s in 0..10000 {
                test_reserve_capacity(s);
            }
        });
    }
}

[klensy]

Same here.

[cuviper]

@bors r+

[bors]

📌 Commit f7bc68b has been approved by cuviper

It is now in the queue for this repository.

[bors]

🌲 The tree is currently closed for pull requests below priority 999. This pull request will be tested once the tree is reopened.