GeezerVison

All commit authors signed the Contributor License Agreement.

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

Could you add some tests? And share benchmark results compared against the main branch?

Is there a standard way to add tests for "this set of system calls is made" or "this many system calls is made"? I tried hunting through the existing tests but couldn't find anything like that or a good way to do that for underlying C code. Would definitely be nice to have a test around open().read() doesn't get more system calls added unintentionally.

re: Benchmarking, I did some with a test program and included details in the initial commit: 78c4de0, wall clock on my dev machine changes were generally in the noise. Happy to work on running a more general suite.

I simply meant to test that the code still works correctly with the changes you made.

Set up git worktree, build the main branch and readall_faster and then run the benchmark for both buillds.

For testing, the existing test_fileio checks basic behavior of .read() (https://github.com/python/cpython/blob/main/Lib/test/test_fileio.py#L133-L139). As an additional check I ran the test program from the first commit under strace and diffed the call log, validating in the diff all the read() calls were the same, and that changes to fstat() and lseek() calls were as expected.

I ran pyperformance benchmark and didn't get any big swings / just noise. Writing a little pyperf benchmark around "read whole file:

import pyperf
from pathlib import Path

def read_file(path_obj):
    path_obj.read_text()

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"))

cmaloney/readall_faster

.....................
read_file_small: Mean +- std dev: 7.92 us +- 0.07 us
.....................
read_file_large: Mean +- std dev: 21.2 us +- 0.6 us

main

python ../benchmark.py
.....................
read_file_small: Mean +- std dev: 8.43 us +- 0.12 us
.....................
read_file_large: Mean +- std dev: 24.0 us +- 0.4 us

for my particular Mac

Thanks, this is excellent.
Regarding writing a test, not sure there's really a standard thing, but you could pattern match

@hauntsaninja planning to try and make a separate PR for that (list item per what I think will be separate commits)

1. Pull out the skip if not linux, is strace available + run this under strace + parse strace results to test.support
2. Change the existing test over to it
3. Add a new test for file reading (small_file, big_file x binary, text). Extend the strace helper pieces to have "marker" support so I can separate out the "read file" I want from interpreter startup (which reads lots of imports using the same code)

Then use that infrastructure here (So this PR will get a merge commit + new commit which updates the test for the less system calls). I don't think that needs a separate GH Issue to track, if it does let me know.

re: _pyio I'll look at how far its behavior is currently from _io when I add the system call test. I would like not to pull getting them to match into the scope of work for this PR. Longer term I would really like to make os.read and all the I/O layers on top fast and more python native as I think that could enable some really cool potential optimizations, like constant folding away compiler-checked redundant checks, while also making the code more legible and debuggable. Currently at least in the code I read things like the _pyio buffer resizing is fairly different between _io and _pyio.

Re: DEFAULT_BUFFER_SIZE, I actually experimented with "just allocate and try and try to read DEFAULT_BUFFER_SIZE always", and found that for both small and large files it was slower. Not entirely sure what the slowdown was, but led me to the "cache the size" approach which is uniformly faster. Definitely an interesting constant to raise, and I think fairly important on the write side. Would be curious to see numbers for read.

Updated with changes to make _pyio.FileIO system calls match, tested locally with added strace syscall test #121143 (diff on top of that PR to get to passing with these changes 0606677)

Merged, thank you. It's a nice optimization.

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Hi! The buildbot AMD64 Ubuntu Shared 3.x has failed when building commit 2f5f19e.

What do you need to do:

1. Don't panic.
2. Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
3. Go to the page of the buildbot that failed (https://buildbot.python.org/all/#builders/506/builds/8282) and take a look at the build logs.
4. Check if the failure is related to this commit (2f5f19e) or if it is a false positive.
5. If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/all/#builders/506/builds/8282

Failed tests:

• test_largefile

Failed subtests:

• test_truncate - test.test_largefile.CLargeFileTest.test_truncate

Summary of the results of the build (if available):

==

Traceback (most recent call last):
  File "/srv/buildbot/buildarea/3.x.bolen-ubuntu/build/Lib/test/test_largefile.py", line 144, in test_truncate
    self.assertEqual(len(f.read()), 1)  # else wasn't truncated
                         ~~~~~~^^
MemoryError

@cmaloney: Oh, test_largefile failed. Can you investigate?

[1/1/1] test_largefile failed (1 error)
Re-running test_largefile in verbose mode (matching: test_truncate)
test_truncate (test.test_largefile.CLargeFileTest.test_truncate) ... ERROR
test_truncate (test.test_largefile.PyLargeFileTest.test_truncate) ... ok
======================================================================
ERROR: test_truncate (test.test_largefile.CLargeFileTest.test_truncate)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/srv/buildbot/buildarea/3.x.bolen-ubuntu/build/Lib/test/test_largefile.py", line 144, in test_truncate
    self.assertEqual(len(f.read()), 1)  # else wasn't truncated
                         ~~~~~~^^
MemoryError
----------------------------------------------------------------------
Ran 2 tests in 0.005s
FAILED (errors=1)
test test_largefile failed
1 test failed again:
    test_largefile

Looks like just the C implementation (CLargeFileTest) failed after truncate + seek on a very large file (https://github.com/python/cpython/blob/main/Lib/test/test_largefile.py#L144). My guess would be an underflow/overflow. In this PR I updated _pyio on truncate to set a new estimated size. Making FileIO match that or updating both _pyio and _iomodule if .truncate is used to set estimated_size to -1 so it will use default buffer size + size increasing logic will likely fix. Going to experiment/try and reproduce locally on my AMD64 Arch

@vstinner I think #121357 will fix the failure, although I'm unable to reproduce locally so far. estimated_size definitely in this case is significantly larger than the actual file size, and that results in a much bigger than necessary allocation which on a memory constrained machine could lead to an OOM / MemoryError. #121357 reduces maximum resident set size from 2464692 kbytes to 24532 kbytes