GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — GH-120754: Disable buffering in Path.read_bytes by cmaloney · Pull Request #122111 · python/cpython

Conversation

@cmaloney

@cmaloney cmaloney commented Jul 22, 2024 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

Path.read_bytes() is used to read a whole file. buffering= / BufferedIO is focused around making small, possibly interleaved, read and write efficient which doesn't add value for this case. This makes seek + isatty system 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
import pyperf
from pathlib import Path

def 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:

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

.....................
read_file_small: Mean +- std dev: 5.67 us +- 0.05 us
.....................
read_file_large: Mean +- std dev: 9.77 us +- 0.52 us
  • Issue: Speed up open().read() pattern by reducing the number of system calls #120754

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@cmaloney
pythonGH-120754: Disable buffering in Path.read_bytes
dac5dd9
`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:

```python
import pyperf
from pathlib import Path

def 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
cmaloney requested a review from barneygale as a code owner July 22, 2024 08:42
@bedevere-app bedevere-app Bot mentioned this pull request Jul 22, 2024
Speed up open().read() pattern by reducing the number of system calls #120754
Closed
@bedevere-app bedevere-app Bot added the awaiting review label Jul 22, 2024
@blurb-it
📜🤖 Added by blurb_it.
02eaa92
hauntsaninja
hauntsaninja approved these changes Aug 16, 2024
View reviewed changes
@bedevere-app bedevere-app Bot added awaiting merge and removed awaiting review labels Aug 16, 2024
@hauntsaninja
hauntsaninja merged commit 35d8ac7 into python:main Aug 16, 2024
@bedevere-app bedevere-app Bot removed the awaiting merge label Aug 16, 2024
@cmaloney
cmaloney deleted the cmaloney/read_bytes_nobuffer branch August 18, 2024 03:35
jeremyhylton pushed a commit to jeremyhylton/cpython that referenced this pull request Aug 19, 2024
@cmaloney @jeremyhylton
pythonGH-120754: Disable buffering in Path.read_bytes (python#122111)
c4ce2c2
`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:

```python
import pyperf
from pathlib import Path

def 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 request Aug 22, 2024
@cmaloney @blhsing
pythonGH-120754: Disable buffering in Path.read_bytes (python#122111)
91af897
`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:

```python
import pyperf
from pathlib import Path

def 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 cmaloney mentioned this pull request Aug 28, 2024
gh-120754: Refactor I/O modules to stash whole stat result rather than individual members #123412
Merged
cmaloney added a commit to cmaloney/cpython that referenced this pull request Aug 28, 2024
@cmaloney
Adjustments after revert
d3ecdba
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 from python#122111
4. Better diagnostic assertion messagess from @gpshead, so when the test fails have first information immediately available. Makes remote CI debugging much simpler.
@cmaloney cmaloney mentioned this pull request Aug 28, 2024
gh-120754: Add a strace helper and test set of syscalls for open().read(), Take 2 #123413
Merged
@cmaloney cmaloney mentioned this pull request Oct 3, 2024
gh-90102: Remove isatty call during regular open #124922
Merged
@cmaloney cmaloney mentioned this pull request Feb 2, 2025
Reduce copies when reading files in pyio, match behavior of _io #129005
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@hauntsaninja hauntsaninja hauntsaninja approved these changes
@barneygale barneygale Awaiting requested review from barneygale barneygale is a code owner

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

2 participants

@cmaloney @hauntsaninja
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.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.