bpo-45944: Avoid calling isatty() for most open() calls by collinanderson · Pull Request #29870 · python/cpython
Conversation
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a clever hack!
I made a few suggestions to use correct Py_ssize_t everwhere.
|
|
||
| char rawmode[6], *m; | ||
| int line_buffering, is_number; | ||
| __int64_t size = 0; |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| __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); |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| size = PyLong_AsLongLong(size_obj); | |
| size = PyLong_AsSsize_t(size_obj); |
| unsigned int closefd : 1; | ||
| char finalizing; | ||
| unsigned int blksize; | ||
| __int64_t size; |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| __int64_t size; | |
| Py_ssize_t size; |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be off_t. The file size can exceed the Py_ssize_t range.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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; |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| self->size = fdfstat.st_size; | |
| self->size = fdfstat.st_size; |
|
|
||
| static PyMemberDef fileio_members[] = { | ||
| {"_blksize", T_UINT, offsetof(fileio, blksize), 0}, | ||
| {"_size", T_UINT, offsetof(fileio, size), 0}, |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| {"_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 skip news label instead.
(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.