gh-86768: check if fd is seekable in o 8000 s.lseek on Windows #133137
Conversation
Modules/posixmodule.c
Outdated
| if (result >= 0) { | ||
| LARGE_INTEGER distance, newdistance; | ||
| distance.QuadPart = position; | ||
| if (SetFilePointerEx(h, distance, &newdistance, how)) { | ||
| result = newdistance.QuadPart; | ||
| } else { | ||
| result = -1; | ||
| } | ||
| } |
There was a problem hiding this comment.
Why not simply use _lseeki64() after checking GetFileType()? It looks much simpler.
There was a problem hiding this comment.
I thought Windows's CRT will call _get_osfhandle to convert the POSIX's fd to Windows's handle internally, which we've already called, so use SetFilePointerEx with the handle directly will gain some performance benefit. But I run the small benchmark script which mentioned at the top of this PR, and didn't see any noticeable performance regret, so updated to use _lseeki64 directly.
Modules/posixmodule.c
Outdated
| if (errno == 0) { | ||
| errno = winerror_to_errno(GetLastError()); | ||
| } |
There was a problem hiding this comment.
Why not use PyErr_SetFromWindowsErr(0)?
There was a problem hiding this comment.
SetFilePointerEx will set the error to GetLastError instead of errno, so we should check it. But after changed to call _lseeki64 directly, there is no need for this line of code.
Modules/posixmodule.c
Outdated
| @@ -219,6 +219,7 @@ | |||
| # if defined(MS_WINDOWS_DESKTOP) || defined(MS_WINDOWS_SYSTEM) | |||
| # define HAVE_SYMLINK | |||
| # endif /* MS_WINDOWS_DESKTOP | MS_WINDOWS_SYSTEM */ | |||
| extern int winerror_to_errno(int); | |||
|
hello, is it the function that is called when doing if yes, I am somewhat concerned about the performance impact of adding a syscall on every call. is it really needed? seek is very heavily used by applications and it is performance sensitive. |
|
For example, you can write a ZIP file to not seekable file. If the output file is seekable, It is unfortunate that this change will add 0.3 microseconds for each |
This change will introduce a performance regression:
Before the change:
After the change:
However I think it's acceptable because we added a check in the implementation and
os.lseekusually won't been called too many times in the real world.