Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Clarification on performance improvements in std::fs::read_to_string for Windows #130600

Closed
fiveseven-lambda opened this issue Sep 20, 2024 · 15 comments · May be fixed by #130610
Closed

Clarification on performance improvements in std::fs::read_to_string for Windows #130600

fiveseven-lambda opened this issue Sep 20, 2024 · 15 comments · May be fixed by #130610
Labels
A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. O-windows Operating system: Windows T-libs Relevant to the library team, which will review and decide on the PR/issue.

Comments

@fiveseven-lambda
Copy link

Location

Function read_to_string in std::fs
https://doc.rust-lang.org/stable/std/fs/fn.read_to_string.html

Summary

The current documentation for std::fs::read_to_string states that it is a convenience function for File::open and std::io::Read::read_to_string, providing fewer imports and no intermediate variables. However, after the recent pull request #110655, it seems that std::fs::read_to_string has received performance improvements specifically for Windows, addressing issues that File::open and std::io::Read::read_to_string still face on this platform.

This implies that using std::fs::read_to_string is not just a matter of convenience, but may also result in better performance on Windows compared to manually combining File::open and std::io::Read::read_to_string.

I believe this performance improvement should be mentioned in the documentation, especially for developers targeting Windows, as the current documentation only emphasizes the convenience aspect without mentioning the performance benefits.

Suggested change:
Add a note to the documentation of std::fs::read_to_string, clarifying that due to recent improvements in pull request #110655, it offers better performance on Windows than manually using File::open and std::io::Read::read_to_string.

Thank you for your consideration!

@fiveseven-lambda fiveseven-lambda added the A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools label Sep 20, 2024
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Sep 20, 2024
@workingjubilee
Copy link
Member

PRs welcome!

@jieyouxu jieyouxu added O-windows Operating system: Windows T-libs Relevant to the library team, which will review and decide on the PR/issue. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Sep 20, 2024
@the8472
Copy link
Member

the8472 commented Sep 20, 2024

it seems that std::fs::read_to_string has received performance improvements specifically for Windows, addressing issues that File::open and std::io::Read::read_to_string still face on this platform.

This should not be the case. The read-loop limits the buffer size passed to the underlying reader regardless of whether you use fs::read_to_string or Read::read_to_string. I.e. #110650 should be fixed in both cases.

// Use heuristics to determine the max read size if no initial size hint was provided
if size_hint.is_none() {
// The reader is returning short reads but it doesn't call ensure_init().
// In that case we no longer need to restrict read sizes to avoid
// initialization costs.
if !was_fully_initialized {
max_read_size = usize::MAX;
}
// we have passed a larger buffer than previously and the
// reader still hasn't returned a short read
if buf_len >= max_read_size && bytes_read == buf_len {
max_read_size = max_read_size.saturating_mul(2);
}

@fiveseven-lambda
Copy link
Author

But the size_hint is not passed to default_read_to_string when you call std::io::Read::read_to_string. Since the trait std::io::Read doesn't have any method like .size_hint() on std::iter::Iterator, we cannot know the file size in advance.

#[stable(feature = "rust1", since = "1.0.0")]
fn read_to_string(&mut self, buf: &mut String) -> Result<usize> {
default_read_to_string(self, buf, None)
}

@the8472
Copy link
Member

the8472 commented Sep 21, 2024

#110650 was not about allocation, it was about a too-large buffer getting passed to the windows API.

@fiveseven-lambda
Copy link
Author

Thank you for pointing that out, and I apologize for not noticing the changes introduced in #118222 earlier.
It seems I had a major misunderstanding regarding the performance issue. I really appreciate you taking the time to explain it.
At this point, I no longer feel the need to propose any changes to the documentation.

@fiveseven-lambda
Copy link
Author

@workingjubilee so I'm sorry for the confusion. I will close both the issue and the PR.

@the8472
Copy link
Member

the8472 commented Sep 21, 2024

Hrrm, looking at it again, the heuristics don't apply optimally.

Usually file io APIs will fill the whole buffer so no short reads will occur, which means it will stay in the doubling regime for a while, which avoids excessive initialization costs. But when it hits EOF there'll be one short read. And since we can't distinguish from short reads and EOF we have do one more read to figure that out. At that point the full buffer will be passed because the heuristics think no initialization happens (since we only check the initialization done via BorrowedBuf, not whatever windows is doing).

There should be ways to improve this.

@the8472
Copy link
Member

the8472 commented Sep 21, 2024

Thanks for bringing it to our attention, there appears to be room for improvement after all!

@fiveseven-lambda
Copy link
Author

Hmm, this is becoming a bit difficult for me to follow. Should we close this issue and PR, and instead create a new one focused on performance improvements?

@the8472
Copy link
Member

the8472 commented Sep 21, 2024

Yes, I think that would be good. My idea would be deferring the max_read_size = usize::MAX; until we got two consecutive short reads without excess BorrowedBuf initialization. There probably are other ways to achieve the same results though.

If you like you can take a stab at it, otherwise I will.

@the8472
Copy link
Member

the8472 commented Sep 21, 2024

@ChrisDenton btw, did you observe what windows is doing? Is it zeroing the buffers in the kernel? If that is the case we could check this in a test by writing some sentinel bytes at the end of a large buffer and check if they survive a trip through the read_to_end API.

@ChrisDenton
Copy link
Member

ChrisDenton commented Sep 21, 2024

Iirc it's aggressively locking every page in the buffer. Which can matter if the buffer is large but the read is relatively small. EDIT: to expand on that, in my notes I have that it does some validation of the pages (e.g. are they accessible?) then marks them as non-pageable and maps them into kernel space. There is an optimization for small reads where it uses an internal buffer then simply copies to the user buffer.

@fiveseven-lambda
Copy link
Author

It seems difficult for me to touch the source code.

@the8472
Copy link
Member

the8472 commented Sep 21, 2024

PR #130670

@CAD97
Copy link
Contributor

CAD97 commented Sep 21, 2024

But the size_hint is not passed to default_read_to_string when you call std::io::Read::read_to_string.

This is incorrect. The heuristics never apply in this case, as <&File as Read>::read_to_end does not use the default implementation and does supply the metadata-derived expected read size. The only optimization that fs::read_to_string has is that it can use the size from the file metadata directly and doesn't need to retrieve the current seek position of the file handle.

rust/library/std/src/fs.rs

Lines 830 to 835 in 2836482

// Reserves space in the buffer based on the file size when available.
fn read_to_string(&mut self, buf: &mut String) -> io::Result<usize> {
let size = buffer_capacity_required(self);
buf.try_reserve(size.unwrap_or(0))?;
io::default_read_to_string(self, buf, size)
}

GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this issue Sep 22, 2024
…ChrisDenton

delay uncapping the max_read_size in File::read_to_end

In rust-lang#130600 (comment) I realized that we're likely still passing too-large buffers to the OS, at least once at the end.

Previous issues and PRs:
* rust-lang#110650
* rust-lang#110655
* rust-lang#118222

r? ChrisDenton
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this issue Sep 22, 2024
…ChrisDenton

delay uncapping the max_read_size in File::read_to_end

In rust-lang#130600 (comment) I realized that we're likely still passing too-large buffers to the OS, at least once at the end.

Previous issues and PRs:
* rust-lang#110650
* rust-lang#110655
* rust-lang#118222

r? ChrisDenton
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this issue Sep 22, 2024
…ChrisDenton

delay uncapping the max_read_size in File::read_to_end

In rust-lang#130600 (comment) I realized that we're likely still passing too-large buffers to the OS, at least once at the end.

Previous issues and PRs:
* rust-lang#110650
* rust-lang#110655
* rust-lang#118222

r? ChrisDenton
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Sep 22, 2024
Rollup merge of rust-lang#130670 - the8472:read-to-end-heuristics, r=ChrisDenton

delay uncapping the max_read_size in File::read_to_end

In rust-lang#130600 (comment) I realized that we're likely still passing too-large buffers to the OS, at least once at the end.

Previous issues and PRs:
* rust-lang#110650
* rust-lang#110655
* rust-lang#118222

r? ChrisDenton
github-actions bot pushed a commit to rust-lang/miri that referenced this issue Sep 25, 2024
delay uncapping the max_read_size in File::read_to_end

In rust-lang/rust#130600 (comment) I realized that we're likely still passing too-large buffers to the OS, at least once at the end.

Previous issues and PRs:
* #110650
* #110655
* #118222

r? ChrisDenton
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. O-windows Operating system: Windows T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants