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

Limit read size in File::read_to_end loop #110655

Merged
merged 1 commit into from
Apr 23, 2023
Merged

Limit read size in File::read_to_end loop #110655

merged 1 commit into from
Apr 23, 2023

Conversation

ChrisDenton
Copy link
Member

@ChrisDenton ChrisDenton commented Apr 21, 2023 •
edited
Loading

Fixes #110650.

Windows file reads have perf overhead that's proportional to the buffer size. When we have a reasonable expectation that we know the file size, we can set a reasonable upper bound for the size of the buffer in one read call.

@ChrisDenton
Limit read size in File::read_to_end loop
f74fe8b 
This works around performance issues on  Windows by limiting reads the size of reads when the expected size is known.
@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Apr 21, 2023
@rustbot
Copy link
Collaborator

rustbot commented Apr 21, 2023

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

joshtriplett
joshtriplett requested changes Apr 23, 2023
View reviewed changes
Copy link
Member

@joshtriplett joshtriplett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please don't add this amount of additional space when reading (for instance) an 8 byte file. If we need a bit of slack to make sure we've gotten EOF, that's fine, but let's not add 1024 bytes or more to an 8 byte read.

@joshtriplett
Copy link
Member

@bors r+ rollup=never

@bors
Copy link
Contributor

bors commented Apr 23, 2023

📌 Commit f74fe8b has been approved by joshtriplett

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 23, 2023
@bors
Copy link
Contributor

bors commented Apr 23, 2023

⌛ Testing commit f74fe8b with merge 9de7d91...

@bors
Copy link
Contributor

bors commented Apr 23, 2023

☀️ Test successful - checks-actions
Approved by: joshtriplett
Pushing 9de7d91 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Apr 23, 2023
@bors bors merged commit 9de7d91 into rust-lang:master Apr 23, 2023
@rustbot rustbot added this to the 1.71.0 milestone Apr 23, 2023
@bors bors mentioned this pull request Apr 23, 2023
Merged
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (9de7d91): 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)		 - - 0
Improvements ✅
(primary)		 -1.7% [-2.2%, -0.7%] 3
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) -1.7% [-2.2%, -0.7%] 3

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)		 2.0% [2.0%, 2.0%] 1
Improvements ✅
(primary)		 -0.6% [-0.6%, -0.6%] 1
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) -0.6% [-0.6%, -0.6%] 1

@ChrisDenton ChrisDenton deleted the read-to-end branch April 24, 2023 02:33
@v1gnesh v1gnesh mentioned this pull request Jul 21, 2023
Closed
@mqudsi mqudsi mentioned this pull request May 22, 2024
Open
This was referenced Sep 20, 2024
Closed
Open
@the8472 the8472 mentioned this pull request Sep 21, 2024
Merged
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Sep 22, 2024
@GuillaumeGomez
Rollup merge of rust-lang#130670 - the8472:read-to-end-heuristics, r=…
cc9b67f 
…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 pull request Sep 22, 2024
@GuillaumeGomez
Rollup merge of rust-lang#130670 - the8472:read-to-end-heuristics, r=…
0a0a828 
…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 pull request Sep 22, 2024
@GuillaumeGomez
Rollup merge of rust-lang#130670 - the8472:read-to-end-heuristics, r=…
82b4177 
…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 pull request Sep 22, 2024
@rust-timer
Unrolled build for rust-lang#130670
7907983 
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jyn514 jyn514 jyn514 approved these changes

@joshtriplett joshtriplett joshtriplett approved these changes

Assignees

@joshtriplett joshtriplett

Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.71.0
Development

Successfully merging this pull request may close these issues.

File::read_to_end's unexpected performance on Windows
6 participants
@ChrisDenton @rustbot @joshtriplett @bors @rust-timer @jyn514