From 3dd28d11d095ec27364c09c09d6f66086ebd90b2 Mon Sep 17 00:00:00 2001 From: Cody Maloney Date: Thu, 15 Oct 2026 10:43:19 -0700 Subject: [PATCH] gh-159446: Release the GIL less in FileIO open and readall When FileIO is doing the open use one GIL release for both the open and fstat. Passing a file descriptor or using an opener still does the fstat separately. readall always took the GIL at least twice: Once to read the data and a second time through the loop for the EOF. On small files the GIL release and reacquire was a significant portion of time. Move so the readall loop checks for signals just as often but only releases the GIL once before I/O and re-acquires after. _Py_read_noraise() is the platform read() factored out of _Py_read() for callers that release the GIL themselves, like _Py_write_noraise(). Reading a small file with FileIO(path).readall() + close(), PGO+LTO, 4 interleaved rounds of 10 processes: | Benchmark | base | this change | |----------------|:-------:|:---------------------:| | fileio_readall | 3.68 us | 3.60 us: 1.02x faster | Callgrind, instructions per call: | Build | base | this change | |------------------|-------:|------------:| | PGO+LTO | 10,646 | 9,842 | | -O3, no PGO | 14,154 | 13,303 |
Benchmarks bm_fileio_readall.py: ```python import io import pyperf def fileio_readall(): with io.FileIO(__file__) as f: f.readall() runner = pyperf.Runner() runner.bench_func("fileio_readall", fileio_readall) ``` ```sh for round in 1 2 3 4; do # alternate the order between rounds base/python bm_fileio_readall.py -p 10 -q --append base.json change/python bm_fileio_readall.py -p 10 -q --append change.json done python -m pyperf compare_to base.json change.json --table ``` callgrind_readall.py: ```python import io import sys for _ in range(int(sys.argv[1])): with io.FileIO(__file__) as f: f.readall() ``` ```sh # Instructions per call: (Ir(2000) - Ir(0)) / 2000 for n in 0 2000; do PYTHONHASHSEED=0 valgrind --tool=callgrind --callgrind-out-file=cg.$n \ ./python -S callgrind_readall.py $n done ```
Co-Authored-By: Claude Opus 5.5 --- Include/internal/pycore_fileutils.h | 5 ++ Lib/test/_test_eintr.py | 7 +++ ...-10-14-22-55-42.gh-issue-159446.i6l5GR.rst | 3 + Modules/_io/fileio.c | 58 ++++++++++++++----- Python/fileutils.c | 54 ++++++++++------- 5 files changed, 91 insertions(+), 36 deletions(-) create mode 100644 Misc/NEWS.d/next/Library/2026-10-14-22-55-42.gh-issue-159446.i6l5GR.rst diff --git a/Include/internal/pycore_fileutils.h b/Include/internal/pycore_fileutils.h index a5fe5206477d05..95bca6ec9bfc9b 100644 --- a/Include/internal/pycore_fileutils.h +++ b/Include/internal/pycore_fileutils.h @@ -115,6 +115,11 @@ extern Py_ssize_t _Py_read( void *buf, size_t count); +extern Py_ssize_t _Py_read_noraise( + int fd, + void *buf, + size_t count); + // Export for 'select' shared extension (Solaris devpoll_flush()) PyAPI_FUNC(Py_ssize_t) _Py_write( int fd, diff --git a/Lib/test/_test_eintr.py b/Lib/test/_test_eintr.py index 4a050792df73c4..6916676e5019f9 100644 --- a/Lib/test/_test_eintr.py +++ b/Lib/test/_test_eintr.py @@ -11,6 +11,7 @@ import contextlib import faulthandler import fcntl +import io import os import platform import select @@ -163,6 +164,12 @@ def test_readinto(self): self.assertEqual(os.readinto(fd, buffer), len(expected)) self.assertEqual(buffer, expected) + def test_fileio_readall(self): + # The first readall() reads everything up to EOF. + data = [io.FileIO(fd, closefd=False).readall() + for fd, _ in self._interrupted_reads()] + self.assertEqual(data, [b"helloworldspam", b"", b""]) + def test_write(self): rd, wr = os.pipe() self.addCleanup(os.close, wr) diff --git a/Misc/NEWS.d/next/Library/2026-10-14-22-55-42.gh-issue-159446.i6l5GR.rst b/Misc/NEWS.d/next/Library/2026-10-14-22-55-42.gh-issue-159446.i6l5GR.rst new file mode 100644 index 00000000000000..701e9051462390 --- /dev/null +++ b/Misc/NEWS.d/next/Library/2026-10-14-22-55-42.gh-issue-159446.i6l5GR.rst @@ -0,0 +1,3 @@ +Opening and reading a small file with :class:`io.FileIO` is about 5% faster: +the constructor no longer raises and discards a :exc:`TypeError` for a path, +and the :term:`GIL` is released fewer times. diff --git a/Modules/_io/fileio.c b/Modules/_io/fileio.c index e5cd7aca5de2f1..3bcee7be6a1e48 100644 --- a/Modules/_io/fileio.c +++ b/Modules/_io/fileio.c @@ -275,7 +275,7 @@ _io_FileIO___init___impl(fileio *self, PyObject *nameobj, const char *mode, #elif !defined(MS_WINDOWS) int *atomic_flag_works = NULL; #endif - int fstat_result; + int fstat_result = -1; int async_err = 0; #ifdef Py_DEBUG @@ -403,9 +403,18 @@ _io_FileIO___init___impl(fileio *self, PyObject *nameobj, const char *mode, goto error; } + PyMem_Free(self->stat_atopen); + self->stat_atopen = PyMem_New(struct _Py_stat_struct, 1); + if (self->stat_atopen == NULL) { + PyErr_NoMemory(); + goto error; + } if (fd >= 0) { self->fd = fd; self->closefd = closefd; + Py_BEGIN_ALLOW_THREADS + fstat_result = _Py_fstat_noraise(self->fd, self->stat_atopen); + Py_END_ALLOW_THREADS } else { self->closefd = 1; @@ -424,6 +433,10 @@ _io_FileIO___init___impl(fileio *self, PyObject *nameobj, const char *mode, #else self->fd = open(name, flags, 0666); #endif + if (self->fd >= 0) { + fstat_result = _Py_fstat_noraise(self->fd, + self->stat_atopen); + } Py_END_ALLOW_THREADS } while (self->fd < 0 && errno == EINTR && !(async_err = PyErr_CheckSignals())); @@ -465,6 +478,9 @@ _io_FileIO___init___impl(fileio *self, PyObject *nameobj, const char *mode, } goto error; } + Py_BEGIN_ALLOW_THREADS + fstat_result = _Py_fstat_noraise(self->fd, self->stat_atopen); + Py_END_ALLOW_THREADS } fd_is_own = 1; @@ -474,15 +490,6 @@ _io_FileIO___init___impl(fileio *self, PyObject *nameobj, const char *mode, #endif } - PyMem_Free(self->stat_atopen); - self->stat_atopen = PyMem_New(struct _Py_stat_struct, 1); - if (self->stat_atopen == NULL) { - PyErr_NoMemory(); - goto error; - } - Py_BEGIN_ALLOW_THREADS - fstat_result = _Py_fstat_noraise(self->fd, self->stat_atopen); - Py_END_ALLOW_THREADS if (fstat_result < 0) { /* Tolerate fstat() errors other than EBADF. See Issue #25717, where an anonymous file on a Virtual Box shared folder filesystem would @@ -837,24 +844,43 @@ _io_FileIO_readall_impl(fileio *self, PyTypeObject *cls) } } - n = _Py_read(self->fd, - (char*)PyBytesWriter_GetData(writer) + bytes_read, - bufsize - bytes_read); + /* Fill the buffer in one GIL release: usually both the data and the + read() that finds EOF. */ + char *buf = PyBytesWriter_GetData(writer); + int fd = self->fd; + int err; + Py_BEGIN_ALLOW_THREADS + do { + n = _Py_read_noraise(fd, buf + bytes_read, bufsize - bytes_read); + if (n > 0) { + bytes_read += n; + } + } while (n > 0 && bytes_read < (Py_ssize_t)bufsize); + err = errno; + Py_END_ALLOW_THREADS if (n == 0) break; if (n == -1) { - if (errno == EAGAIN) { - PyErr_Clear(); + if (err == EINTR) { + /* PEP 475: retry unless a signal handler raised. */ + if (PyErr_CheckSignals() < 0) { + PyBytesWriter_Discard(writer); + return NULL; + } + continue; + } + if (err == EAGAIN) { if (bytes_read > 0) break; PyBytesWriter_Discard(writer); Py_RETURN_NONE; } + errno = err; + PyErr_SetFromErrno(PyExc_OSError); PyBytesWriter_Discard(writer); return NULL; } - bytes_read += n; } return PyBytesWriter_FinishWithSize(writer, bytes_read); diff --git a/Python/fileutils.c b/Python/fileutils.c index 3ea0f59f69ad1e..7699ca8b4b7270 100644 --- a/Python/fileutils.c +++ b/Python/fileutils.c @@ -2004,6 +2004,39 @@ Py_fclose(FILE *file) } +/* A single read() for callers that release the GIL themselves: no exception, + return -1 with errno set on error, EINTR included. */ +Py_ssize_t +_Py_read_noraise(int fd, void *buf, size_t count) +{ + Py_ssize_t n; + + /* The caller detached its thread state (released the GIL). */ + assert(_PyThreadState_GET() == NULL); + + if (count > _PY_READ_MAX) { + count = _PY_READ_MAX; + } + + _Py_BEGIN_SUPPRESS_IPH + errno = 0; +#ifdef MS_WINDOWS + _doserrno = 0; + n = read(fd, buf, (int)count); + // read() on a non-blocking empty pipe fails with EINVAL, which is + // mapped from the Windows error code ERROR_NO_DATA. + if (n < 0 && errno == EINVAL) { + if (_doserrno == ERROR_NO_DATA) { + errno = EAGAIN; + } + } +#else + n = read(fd, buf, count); +#endif + _Py_END_SUPPRESS_IPH + return n; +} + /* Read count bytes from fd into buf. On success, return the number of read bytes, it can be lower than count. @@ -2031,34 +2064,15 @@ _Py_read(int fd, void *buf, size_t count) * handler raised an exception. */ assert(!PyErr_Occurred()); - if (count > _PY_READ_MAX) { - count = _PY_READ_MAX; - } - - _Py_BEGIN_SUPPRESS_IPH do { Py_BEGIN_ALLOW_THREADS - errno = 0; -#ifdef MS_WINDOWS - _doserrno = 0; - n = read(fd, buf, (int)count); - // read() on a non-blocking empty pipe fails with EINVAL, which is - // mapped from the Windows error code ERROR_NO_DATA. - if (n < 0 && errno == EINVAL) { - if (_doserrno == ERROR_NO_DATA) { - errno = EAGAIN; - } - } -#else - n = read(fd, buf, count); -#endif + n = _Py_read_noraise(fd, buf, count); /* save/restore errno because PyErr_CheckSignals() * and PyErr_SetFromErrno() can modify it */ err = errno; Py_END_ALLOW_THREADS } while (n < 0 && err == EINTR && !(async_err = PyErr_CheckSignals())); - _Py_END_SUPPRESS_IPH if (async_err) { /* read() was interrupted by a signal (failed with EINTR)