Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork34.1k
GH-120754: Disable buffering in Path.read_bytes#122111
Merged
hauntsaninja merged 2 commits intopython:mainfromAug 16, 2024
Merged
GH-120754: Disable buffering in Path.read_bytes#122111hauntsaninja merged 2 commits intopython:mainfrom
hauntsaninja merged 2 commits intopython:mainfrom
Conversation
`Path.read_bytes()` is used to read a whole file. buffering /BufferedIO is focused around making small, possibly interleaved,read/write efficient which doesn't add value in this case.On my Mac, running the benchmark:```pythonimport pyperffrom pathlib import Pathdef read_all(all_paths): for p in all_paths: p.read_bytes()def read_file(path_obj): path_obj.read_bytes()all_rst = list(Path("Doc").glob("**/*.rst"))all_py = list(Path(".").glob("**/*.py"))assert all_rst, "Should have found rst files"assert all_py, "Should have found python source files"runner = pyperf.Runner()runner.bench_func("read_file_small", read_file, Path("Doc/howto/clinic.rst"))runner.bench_func("read_file_large", read_file, Path("Doc/c-api/typeobj.rst"))```before:```python.....................read_file_small: Mean +- std dev: 6.80 us +- 0.07 us.....................read_file_large: Mean +- std dev: 10.8 us +- 0.2 us````after:```python.....................read_file_small: Mean +- std dev: 5.67 us +- 0.05 us.....................read_file_large: Mean +- std dev: 9.77 us +- 0.52 us```hauntsaninja approved these changesAug 16, 2024
jeremyhylton pushed a commit to jeremyhylton/cpython that referenced this pull requestAug 19, 2024
`Path.read_bytes()` is used to read a whole file. buffering /BufferedIO is focused around making small, possibly interleaved,read/write efficient which doesn't add value in this case.On my Mac, running the benchmark:```pythonimport pyperffrom pathlib import Pathdef read_all(all_paths): for p in all_paths: p.read_bytes()def read_file(path_obj): path_obj.read_bytes()all_rst = list(Path("Doc").glob("**/*.rst"))all_py = list(Path(".").glob("**/*.py"))assert all_rst, "Should have found rst files"assert all_py, "Should have found python source files"runner = pyperf.Runner()runner.bench_func("read_file_small", read_file, Path("Doc/howto/clinic.rst"))runner.bench_func("read_file_large", read_file, Path("Doc/c-api/typeobj.rst"))```before:```python.....................read_file_small: Mean +- std dev: 6.80 us +- 0.07 us.....................read_file_large: Mean +- std dev: 10.8 us +- 0.2 us````after:```python.....................read_file_small: Mean +- std dev: 5.67 us +- 0.05 us.....................read_file_large: Mean +- std dev: 9.77 us +- 0.52 us```blhsing pushed a commit to blhsing/cpython that referenced this pull requestAug 22, 2024
`Path.read_bytes()` is used to read a whole file. buffering /BufferedIO is focused around making small, possibly interleaved,read/write efficient which doesn't add value in this case.On my Mac, running the benchmark:```pythonimport pyperffrom pathlib import Pathdef read_all(all_paths): for p in all_paths: p.read_bytes()def read_file(path_obj): path_obj.read_bytes()all_rst = list(Path("Doc").glob("**/*.rst"))all_py = list(Path(".").glob("**/*.py"))assert all_rst, "Should have found rst files"assert all_py, "Should have found python source files"runner = pyperf.Runner()runner.bench_func("read_file_small", read_file, Path("Doc/howto/clinic.rst"))runner.bench_func("read_file_large", read_file, Path("Doc/c-api/typeobj.rst"))```before:```python.....................read_file_small: Mean +- std dev: 6.80 us +- 0.07 us.....................read_file_large: Mean +- std dev: 10.8 us +- 0.2 us````after:```python.....................read_file_small: Mean +- std dev: 5.67 us +- 0.05 us.....................read_file_large: Mean +- std dev: 9.77 us +- 0.52 us```cmaloney added a commit to cmaloney/cpython that referenced this pull requestAug 28, 2024
NOTE: This needs a full buildbot test pass before merge, see:python#121143 (comment).1. Added `statx` to set of allowed syscall forms (Should make Raspian bot pass).2. Check that the `fd` returned from `open` is passed to all future calls. This helps ensure things like the `stat` call uses the file descriptor rather than the `filename` to avoid TOCTOU isuses.3. Update the `Path().read_bytes()` test case to additionally validate the reduction in`isatty`/`ioctl` + `seek` calls frompython#1221114. Better diagnostic assertion messagess from@gpshead, so when the test fails have first information immediately available. Makes remote CI debugging much simpler.
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
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
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.
Uh oh!
There was an error while loading.Please reload this page.
Path.read_bytes()is used to read a whole file.buffering=/BufferedIOis focused around making small, possibly interleaved, read and write efficient which doesn't add value for this case. This makesseek+isattysystem call no longer called in this case, as well as removing the construction of the BufferedIO, allocation of a buffer, etc.Benchmark
Run on my Mac
Benchmark code
Before:
After: