Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork33.3k
bpo-45944: Avoid calling isatty() for most open() calls#29870
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
There was a problem hiding this comment.
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 correctPy_ssize_t everwhere.
| charrawmode[6],*m; | ||
| intline_buffering,is_number; | ||
| __int64_tsize=0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
| __int64_tsize=0; | |
| Py_ssize_tsize=0; |
| size_obj=_PyObject_GetAttrId(raw,&PyId__size); | ||
| if (size_obj==NULL) | ||
| gotoerror; | ||
| size=PyLong_AsLongLong(size_obj); |
There was a problem hiding this comment.
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); |
| unsignedintclosefd :1; | ||
| charfinalizing; | ||
| unsignedintblksize; | ||
| __int64_tsize; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
| __int64_tsize; | |
| Py_ssize_tsize; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It should beoff_t. The file size can exceed thePy_ssize_t range.
There was a problem hiding this comment.
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 fromposixmodule.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);#endifThere was a problem hiding this comment.
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 orS_IFCHR()) as_maybeatty. Then the size ofoff_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.
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; |
| staticPyMemberDeffileio_members[]= { | ||
| {"_blksize",T_UINT, offsetof(fileio,blksize),0}, | ||
| {"_size",T_UINT, offsetof(fileio,size),0}, |
There was a problem hiding this comment.
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 Pythonrequire a NEWS entry. Add one using theblurb_it web app or theblurb 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...) |
collinanderson commentedOct 8, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Ohh awesome. Thank you! Yeah Sorry I'm not much of a C programmer. I'm super excited to see#120754 syscalls being optimized. |
Uh oh!
There was an error while loading.Please reload this page.
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