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.

Sign up for GitHub

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

Jump to bottom

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

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

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

fiveseven-lambda opened this issue Sep 20, 2024 · 3 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
@fiveseven-lambda fiveseven-lambda linked a pull request Sep 20, 2024 that will close this issue
Open
@the8472
Copy link
Member

the8472 commented Sep 20, 2024 •
edited
Loading

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.

rust/library/std/src/io/mod.rs

Lines 501 to 514 in 1a5a224

// 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.

rust/library/std/src/io/mod.rs

Lines 903 to 906 in 1a5a224

#[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)
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
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
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Update documentation for std::fs::read_to_string to reflect performan… fiveseven-lambda/rust
5 participants
@the8472 @jieyouxu @fiveseven-lambda @workingjubilee @rustbot