Pre-allocate in fs::read and fs::read_string #47324

mbrubeck · 2018-01-10T16:14:28Z

This is a simpler alternative to #46340 and #45928, as requested by the libs team.

sfackler · 2018-01-10T17:12:01Z

src/libstd/fs.rs

+    // Allocate one extra byte so the buffer doesn't need to grow before the final `read` call at
+    // the end of the file.  Don't worry about `usize` overflow because this will fail anyway if
+    // the file doesn't fit into memory.
+    let mut bytes = Vec::with_capacity(metadata(&path)?.len() as usize + 1);


Are there contexts/weird files where the metadata lookup won't work but the file read will?

I'm not totally certain. I based this on #45928 (comment):

You can always return 0 if something went wrong, but it seems like it's nice to not throw away those errors. I kind of doubt that reads would succeed if you can't stat the file anymore.

but I do think it would be reasonable to just start with an empty buffer if the stat call fails.

I agree that starting with an empty buffer if the stat call fails is probably the right thing to do. It's potentially worth pointing out that it seems like this may not be faster (#35844 (comment)) based on past benchmarks so maybe we shouldn't do this.

I think those numbers may no longer be accurate - we no longer zero the buffer before reading into it for Files, and read_to_end has been tweaked a bit to behave differently. #45928 has some benchmarks.

mbrubeck · 2018-01-10T18:35:32Z

Here are before/after benchmark results from my Linux x86_64 laptop with SSD with a hot cache. The alloc versions are using the new code from this PR.

test read_1kb        ... bench:       2,487 ns/iter (+/- 473)
test read_1kb_alloc  ... bench:       1,799 ns/iter (+/- 641)
test read_32kb       ... bench:       6,454 ns/iter (+/- 978)
test read_32kb_alloc ... bench:       3,171 ns/iter (+/- 438)
test read_1mb        ... bench:     431,048 ns/iter (+/- 20,617)
test read_1mb_alloc  ... bench:      55,331 ns/iter (+/- 7,460)
test read_32mb       ... bench:   7,643,615 ns/iter (+/- 161,601)
test read_32mb_alloc ... bench:   7,104,146 ns/iter (+/- 237,000)

sfackler · 2018-01-10T18:47:41Z

@bors r+

bors · 2018-01-10T18:47:41Z

📌 Commit 1745f43 has been approved by sfackler

Mark-Simulacrum · 2018-01-10T18:49:23Z

@bors r=sfackler

bors · 2018-01-10T18:49:23Z

📌 Commit 44912bf has been approved by sfackler

mbrubeck · 2018-01-10T18:49:28Z

Updated to fall back to an empty buffer if metadata returns an error.

sfackler · 2018-01-10T18:50:58Z

@bors r+

bors · 2018-01-10T18:50:59Z

💡 This pull request was already approved, no need to approve it again.

There's another pull request that is currently being tested, blocking this pull request: Use DefIndex encoding that works better with on-disk variable length integer representations. #47181

bors · 2018-01-10T18:50:59Z

📌 Commit 44912bf has been approved by sfackler

petrochenkov · 2018-01-10T20:56:55Z

src/libstd/fs.rs

+fn initial_buffer_size<P: AsRef<Path>>(path: P) -> usize {
+    // Allocate one extra byte so the buffer doesn't need to grow before the
+    // final `read` call at the end of the file.  Don't worry about `usize`
+    // overflow because reading will fail regardless in that case.


Does "fail regardless" here mean Result::Err or panic?
m.len() as usize + 1 can panic.

It will panic in RawVec code when requested capacity exceeds isize::MAX.

(or more likely, abort due to out-of-memory sometime before that)

Pre-allocate in fs::read and fs::read_string This is a simpler alternative to rust-lang#46340 and rust-lang#45928, as requested by the libs team.

Use the new fs_read_write functions in rustc internals Uses `fs::read` and `fs::write` (added by rust-lang#45837) where appropriate, to simplify code and dog-food these new APIs. This also improves performance, when combined with rust-lang#47324.

Pre-allocate in fs::read and fs::read_string This is a simpler alternative to rust-lang#46340 and rust-lang#45928, as requested by the libs team.

Use the new fs_read_write functions in rustc internals Uses `fs::read` and `fs::write` (added by rust-lang#45837) where appropriate, to simplify code and dog-food these new APIs. This also improves performance, when combined with rust-lang#47324.

Rollup of 12 pull requests - Successful merges: #47283, #47288, #47289, #47298, #47305, #47307, #47310, #47322, #47324, #47328, #47340, #47344 - Failed merges:

Pre-allocate in fs::read and fs::read_string This is a simpler alternative to rust-lang#46340 and rust-lang#45928, as requested by the libs team.

Use the new fs_read_write functions in rustc internals Uses `fs::read` and `fs::write` (added by rust-lang#45837) where appropriate, to simplify code and dog-food these new APIs. This also improves performance, when combined with rust-lang#47324.

Rollup of 24 pull requests - Successful merges: #46985, #47069, #47081, #47185, #47282, #47283, #47288, #47289, #47298, #47305, #47306, #47307, #47310, #47324, #47328, #47331, #47340, #47343, #47344, #47352, #47357, #47365, #47375, #47382 - Failed merges: #47334

alex · 2018-01-17T13:13:48Z

Should this use a sequence of calls to do open -> fstat instead of stat -> open? In contrived local measurements on macOS, the fstat version is faster (presumably because it avoids kernel string processing and needing to trace more FS metadata).

Should I file a bug to track this?

SimonSapin · 2018-01-17T13:54:02Z

@alex That sounds like a good idea. (This can use the metadata method of fs::File, which on Unix is implemented with fstat64.) Please file it separately, since this PR is merged.

sfackler reviewed Jan 10, 2018

View reviewed changes

mbrubeck mentioned this pull request Jan 10, 2018

Use the new fs_read_write functions in rustc internals #47328

Merged

Pre-allocate in fs::read and fs::read_string

44912bf

mbrubeck force-pushed the len branch from 1745f43 to 44912bf Compare January 10, 2018 18:48

petrochenkov reviewed Jan 10, 2018

View reviewed changes

kennytm mentioned this pull request Jan 10, 2018

Rollup of 11 pull requests #47339

Closed

kennytm mentioned this pull request Jan 11, 2018

Rollup of 12 pull requests #47350

Closed

bors added a commit that referenced this pull request Jan 11, 2018

Auto merge of #47350 - kennytm:rollup, r=kennytm

27270e6

Rollup of 12 pull requests - Successful merges: #47283, #47288, #47289, #47298, #47305, #47307, #47310, #47322, #47324, #47328, #47340, #47344 - Failed merges:

bors added a commit that referenced this pull request Jan 12, 2018

Auto merge of #47350 - kennytm:rollup, r=kennytm

712e755

Rollup of 12 pull requests - Successful merges: #47283, #47288, #47289, #47298, #47305, #47307, #47310, #47322, #47324, #47328, #47340, #47344 - Failed merges:

kennytm mentioned this pull request Jan 12, 2018

Rollup of 24 pull requests #47392

Merged

bors merged commit 44912bf into rust-lang:master Jan 12, 2018

alex mentioned this pull request Jan 17, 2018

Use fstat instead of stat in libstd/fs.rs initial_buffer_size #47519

Closed

mbrubeck mentioned this pull request Sep 18, 2018

Use std::fs::read TeXitoi/benchmarksgame-rs#79

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pre-allocate in fs::read and fs::read_string #47324

Pre-allocate in fs::read and fs::read_string #47324

mbrubeck commented Jan 10, 2018

sfackler Jan 10, 2018

mbrubeck Jan 10, 2018

Mark-Simulacrum Jan 10, 2018

sfackler Jan 10, 2018

mbrubeck commented Jan 10, 2018

sfackler commented Jan 10, 2018

bors commented Jan 10, 2018

Mark-Simulacrum commented Jan 10, 2018

bors commented Jan 10, 2018

mbrubeck commented Jan 10, 2018

sfackler commented Jan 10, 2018

bors commented Jan 10, 2018

bors commented Jan 10, 2018

petrochenkov Jan 10, 2018

mbrubeck Jan 10, 2018 •

edited

Loading

mbrubeck Jan 10, 2018

alex commented Jan 17, 2018

SimonSapin commented Jan 17, 2018

Pre-allocate in fs::read and fs::read_string #47324

Pre-allocate in fs::read and fs::read_string #47324

Conversation

mbrubeck commented Jan 10, 2018

sfackler Jan 10, 2018

Choose a reason for hiding this comment

mbrubeck Jan 10, 2018

Choose a reason for hiding this comment

Mark-Simulacrum Jan 10, 2018

Choose a reason for hiding this comment

sfackler Jan 10, 2018

Choose a reason for hiding this comment

mbrubeck commented Jan 10, 2018

sfackler commented Jan 10, 2018

bors commented Jan 10, 2018

Mark-Simulacrum commented Jan 10, 2018

bors commented Jan 10, 2018

mbrubeck commented Jan 10, 2018

sfackler commented Jan 10, 2018

bors commented Jan 10, 2018

bors commented Jan 10, 2018

petrochenkov Jan 10, 2018

Choose a reason for hiding this comment

mbrubeck Jan 10, 2018 • edited Loading

Choose a reason for hiding this comment

mbrubeck Jan 10, 2018

Choose a reason for hiding this comment

alex commented Jan 17, 2018

SimonSapin commented Jan 17, 2018

mbrubeck Jan 10, 2018 •

edited

Loading