bpo-45944: Avoid calling isatty() for most open() calls#29870
Uh oh!
There was an error while loading. Please reload this page.
Conversation
tiran left a comment
tiran left a comment There was a problem hiding this comment.
That's a clever hack!
I made a few suggestions to use correct Py_ssize_t everwhere.
| charrawmode[6], *m; | ||
| intline_buffering, is_number; | ||
| __int64_tsize=0; |
There was a problem hiding this comment.
| __int64_tsize=0; | |
| Py_ssize_tsize=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); |
| unsigned intclosefd : 1; | ||
| charfinalizing; | ||
| unsigned intblksize; | ||
| __int64_tsize; |
There was a problem hiding this comment.
| __int64_tsize; | |
| Py_ssize_tsize; |
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.
| 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; |
| staticPyMemberDeffileio_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. |
eendebakpt commented Nov 12, 2022
@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 |
cmaloney commented Oct 8, 2024
skirpichev commented Oct 8, 2024
(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. |
9 participants
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