bpo-45944: Avoid calling isatty() for most open() calls #29870
Conversation
| @@ -237,11 +237,13 @@ _io_open_impl(PyObject *module, PyObject *file, const char *mode, | |||
|
|
|||
| char rawmode[6], *m; | |||
| int line_buffering, is_number; | |||
| __int64_t size = 0; | |||
There was a problem hiding this comment.
| __int64_t size = 0; | |
| Py_ssize_t size = 0; |
| size_obj = _PyObject_GetAttrId(raw, &PyId__size); | ||
| if (size_obj == NULL) | ||
| goto error; | ||
| size = PyLong_AsLongLong(size_obj); |
There was a problem hiding this comment.
| size = PyLong_AsLongLong(size_obj); | |
| size = PyLong_AsSsize_t(size_obj); |
| @@ -66,6 +66,7 @@ typedef struct { | |||
| unsigned int closefd : 1; | |||
| char finalizing; | |||
| unsigned int blksize; | |||
| __int64_t size; | |||
There was a problem hiding this comment.
| __int64_t size; | |
| Py_ssize_t size; |
There was a problem hiding this comment.
It should be off_t. The file size can exceed the Py_ssize_t range.
There was a problem hiding this comment.
You are right! size handling depends on platform and support for large file support. You can copy the approach from posixmodule.c:
#ifdef MS_WINDOWS
typedef long long Py_off_t;
#else
typedef off_t Py_off_t;
#endif
#ifdef HAVE_LARGEFILE_SUPPORT
size = (Py_off_t)PyLong_AsLongLong(arg);
#else
size = (Py_off_t)PyLong_AsLong(arg);
#endif
There was a problem hiding this comment.
It is too complicated, we need to duplicate this in several files. But it may be simpler if only cache the boolean result (st_size == 0 or S_IFCHR()) as _maybeatty. Then the size of off_t does not matter.
| @@ -469,6 +471,7 @@ _Py_COMP_DIAG_POP | |||
| if (fdfstat.st_blksize > 1) | |||
| self->blksize = fdfstat.st_blksize; | |||
| #endif /* HAVE_STRUCT_STAT_ST_BLKSIZE */ | |||
| self->size = fdfstat.st_size; | |||
There was a problem hiding this comment.
| self->size = fdfstat.st_size; | |
| self->size = fdfstat.st_size; |
| @@ -1184,6 +1187,7 @@ static PyGetSetDef fileio_getsetlist[] = { | |||
|
|
|||
| static PyMemberDef fileio_members[] = { | |||
| {"_blksize", T_UINT, offsetof(fileio, blksize), 0}, | |||
| {"_size", T_UINT, offsetof(fileio, size), 0}, | |||
There was a problem hiding this comment.
| {"_size", T_UINT, offsetof(fileio, size), 0}, | |
| {"_size", T_PYSSIZET, offsetof(fileio, size), 0}, |
|
This PR is stale because it has been open for 30 days with no activity. |
|
@collinanderson The PR has gone stale. Can you address the review comments? Or are you ok will someone else picking this up? |
|
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 |
|
(And there is no signs of life anyway...) |
|
Ohh awesome. Thank you! Yeah Sorry I'm not much of a C programmer. I'm super excited to see #120754 syscalls being optimized. |
https://bugs.python.org/issue45944
isatty() is a system call on linux. Most open()s are files, and we're already getting the size of the file. If it has a size, then we know it's not a atty, and can avoid calling it.
https://bugs.python.org/issue45944