Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 34k
gh-120754: Reduce system calls in full-file readall case#120755
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
Merged
Uh oh!
There was an error while loading. Please reload this page.
Merged
Changes from all commits
Commits
Show all changes
15 commits Select commit Hold shift + click to select a range
78c4de0 Reduce system calls in full-file readall case
cmaloney 30d335e 📜🤖 Added by blurb_it.
blurb-it[bot] 9d7f925 Update news to pass lint
cmaloney dd0b294 Fix warning around type coercion on 64 bit windows; range is explicit…
cmaloney 7ad6fa8 Change constant to named constant per review
cmaloney fa9ac6a Move downcast to Py_SAFE_DOWNCAST
cmaloney 93aee47 Update Misc/NEWS.d/next/Core and Builtins/2024-06-19-19-54-35.gh-issu…
hauntsaninja 39e48ee Update pyio to try and fstat and seek less
cmaloney b7d3880 Cap read size at _PY_READ_MAX to fix windows x86
cmaloney a4c2cb6 Cap initial bufsize on pyio
cmaloney 84bd2d8 Pyio was relying on getting the size after truncate for test_largefil…
cmaloney b505334 Comment formatting
erlend-aasland 7e276ec size_estimated -> estimated_size
cmaloney 9be6d1d Py_SAFE_DOWNCAST -> C casts
cmaloney dc8e910 Apply suggestions from code review
cmaloney File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Jump to file
Failed to load files.
Loading
Uh oh!
There was an error while loading. Please reload this page.
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
1 change: 1 addition & 0 deletions 1 Misc/NEWS.d/next/Core and Builtins/2024-06-19-19-54-35.gh-issue-120754.uF29sj.rst
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| Reduce the number of system calls invoked when reading a whole file (ex. ``open('a.txt').read()``). For a sample program that reads the contents of the 400+ ``.rst`` files in the cpython repository ``Doc`` folder, there is an over 10% reduction in system call count. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the purpose of the "+1"? It may overallocate 1 byte which is inefficient.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The read loop currently needs to do a
os.read()/_py_Readwhich is a single byte which returns 0 size to find the end of the file and exit the loop. The very beginning of that loop does a check for "if buffer is full, grow buffer" so not over-allocating by one byte results in a much bigger allocation by that. In the_iocase it then shrinks it back down at the end, whereas in the_pyiocase the EOF read is never appended.Could avoid the extra byte by writing a specialized "read known size" (w/ fallback to "read until EOF"), but was trying to avoid making more variants of the read loop and limit risk a bit.
As an aside: the
_pyioimplementation seems to have a lot of extra memory allocation and copy in the default case becauseos.read()internally allocates a buffer which it then copies into itsbytearray...