GeezerVison

All commit authors signed the Contributor License Agreement.

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.

Please update the title to use the gh issue number.

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.

!buildbot .Android.

🤖 New build scheduled with the buildbot fleet by @gpshead for commit b0e8276 🤖

Results will be shown at:

https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F136334%2Fmerge

The command will test the builders whose names match following regular expression: .*Android.*

The builders matched are:

• aarch64 Android PR
• AMD64 Android PR

FYI, we recently added Android to the GitHub Actions CI, so there's no longer much need to invoke the buildbots manually. The exception is if a PR has something which might be specific to Android aarch64, because GitHub Actions can't run the tests on that platform yet, although it will still perform a complete build.

Looking at the failures, the Android buildbots didn't run the most up-to-date code from this PR. Looking at the "Build Properties" tab, the "revision" is "b0e827681f45588ce67cd0924b3c320bb64351ff" (the latest), but "got_revision" is "a387390693c4f4ff793717a91a5137b7f608f9e4", a merge commit attributed to me but that I don't remember making of d53650a (the commit just before I added the configure check) with main.

Both the buildbots and GitHub Actions don't actually test the head of the PR branch, but a speculative commit showing what you would get if you merged the PR into main. Because of a merge conflict, it's not possible to merge the PR into main at the moment. It looks like GitHub Actions deals with this by not running any tests at all, while the buildbots test the last commit before the conflict.

I dislike this approach: modifying Python os.stat() to use statx. I would prefer to expose C statx() as Python os.statx() instead.

I've marked this ready for review. @vstinner I'd also appreciate a review from you; GitHub isn't showing me the interface to formally request one.

This PR has changed quite a bit from the first commit, and as it did so, I wrote quite a few words in the issue #83714 that may be informative (probably best to read from the bottom up).

With the thoroughness you're doing definitely feels like will get to the confidence to change os.stat to point to os.statx.

I don't recommend this. os.statx_result is only mostly compatible with os.stat_result. In particular, os.statx_result is not a tuple, so it can't be indexed, iterated, unpacked, hashed, or pickled. os.stat_result being a tuple and having tuple-like behaviors is (AFAICT) a historical accident, but it isn't deprecated.

👍 / agree with you, meant to say os.stat use the system call statx under the hood (not that the two functions would become the same).

Some benchmark numbers comparing os.stat(".") main at cf9ef73 vs this PR at c84f783 merged with main. Configured with --enable-optimizations --with-lto, running on a Ryzen 5950x, turbo boost disabled on highest-CPPC-perf core 0/16, Arch Linux kernel 6.16.7-arch1-1, glibc 2.42+r17+gd7274d718e6f-1, btrfs filesystem. Histograms in Gist.

The 'PR nobtime' row replaces the last fill_time call in _pystat_fromstructstatx with setting ST_BIRTHTIME_IDX and ST_BIRTHTIME_NS_IDX to None. So while os.stat is slower with this PR, most of the slowdown is due to creating an extra Python float and int object to fill the btime members in the os.stat_result.

I don't have a VM running a pre-statx kernel handy, but I did find the enosys utility from util-linux, which allows running under a seccomp filter that fails a syscall with ENOSYS. That will take the same code path as running on a kernel without statx, but doesn't necessarily have the same performance characteristics. It found a performance problem anyway: glibc's statx wrapper function doesn't remember if statx failed with ENOSYS, so it makes the statx system call every time before falling back to fstatat. That's why this PR has changed to set statx_works based on the test call in posixmodule_exec.

The following numbers are from under enosys after the change. Now that we don't rely on glibc falling back, os.stat doesn't try statx at all, so for our purposes this might be close to running on an old kernel with a new libc. (Also, based on this Zig issue there are at least some people running on a new kernel with a statx-blocking seccomp filter, so this case is relevant to them.)

We're maybe slightly slower, and this time not due to an extra float and int, but if you look at the histograms in the gist, there are more outliers here. It's hard to conclude much.

pyperformance startup benchmark, main vs this PR:

### python_startup ###
Mean +- std dev: 12.7 ms +- 0.1 ms -> 12.8 ms +- 0.2 ms: 1.00x slower
Not significant

### python_startup_no_site ###
Mean +- std dev: 8.48 ms +- 0.09 ms -> 8.54 ms +- 0.07 ms: 1.01x slower
Not significant

main vs PR nobtime:

### python_startup ###
Mean +- std dev: 12.7 ms +- 0.1 ms -> 12.7 ms +- 0.3 ms: 1.00x faster
Not significant

### python_startup_no_site ###
Mean +- std dev: 8.48 ms +- 0.09 ms -> 8.45 ms +- 0.21 ms: 1.00x faster
Not significant

PR nobtime vs PR:

### python_startup ###
Mean +- std dev: 12.7 ms +- 0.3 ms -> 12.8 ms +- 0.2 ms: 1.01x slower
Not significant

### python_startup_no_site ###
Mean +- std dev: 8.45 ms +- 0.21 ms -> 8.54 ms +- 0.07 ms: 1.01x slower
Not significant

This PR doesn't change any of the code in fileutils.c to use statx, so if we're slower, it's because of Python os.stat calls. I haven't actually profiled to count how many there are per startup.

main vs this PR running under enosys (I'm not sure if enosys is doing anything here):

### python_startup ###
Mean +- std dev: 12.7 ms +- 0.2 ms -> 12.8 ms +- 0.2 ms: 1.00x slower
Not significant

### python_startup_no_site ###
Mean +- std dev: 8.51 ms +- 0.06 ms -> 8.55 ms +- 0.04 ms: 1.00x slower
Not significant

As I wrote previously, I would prefer to leave os.stat() unchanged: keep it as a thin wrapper to the C stat() function. I dislike the idea of calling statx() which depends on the kernel and glibc version and might behave differently that stat(). I don't buy the argument of providing st_birthtime by default in os.stat(), most users don't need it (it doesn't exist currently).

I support the idea of adding a new os.statx() function which gives control on which attributes are retrieved. I suppose that the result of this function can be a types.SimpleNamespace object (see _PyNamespace_New() in C).

I suggest creating a new PR which only adds a new os.statx() function, I will review and support it :-)

As I wrote previously, I would prefer to leave os.stat() unchanged: keep it as a thin wrapper to the C stat() function. I dislike the idea of calling statx() which depends on the kernel and glibc version and might behave differently that stat().

Hmm. In 2024 a user asked for an update on the issue and you replied that ntninja's PR needed to be updated. That PR changed os.stat to call statx unconditionally, with no fallback for older kernels. What changed your mind?

I don't buy the argument of providing st_birthtime by default in os.stat(), most users don't need it (it doesn't exist currently).

That's a non sequitur. st_birthtime doesn't currently exist on Linux because Linux didn't provide a syscall to get btime until statx, and then when users asked for st_birthtime -- that's what the linked issue asked for -- no one finished the work to hook up statx in Python. It wasn't considered and declined because "most users don't need it".

The reason I am pushing so hard for st_birthtime from os.stat is because it is also available on Windows. If we make it available on Linux, that code starts working on Linux automatically, and new Linux code will also work on Windows. If getting btime requires a Linux-specific function, code targeting one OS will have to be changed to also support the other.

I support the idea of adding a new os.statx() function which gives control on which attributes are retrieved. I suppose that the result of this function can be a types.SimpleNamespace object (see _PyNamespace_New() in C).

And this PR does in fact add a os.statx function, though it returns a custom object type using getset and member descriptors. I would be (pleasantly) surprised if SimpleNamespace has competitive performance -- grepping for _PyNamespace_New, none of the uses are on fast paths -- but sure, I'll test it.

Victor and I synced on the above at our ongoing core team sprint yesterday, I'm in agreement: Lets start with only os.statx first, as that is most in keeping with the philosophy of the os module as most often being "just" a simple wrapper (at least for POSIX APIs) around the underlying single libc interfaced system call.

While that could be done in this PR you may find it cleaner for review purposes to make a new PR focused on just that.

this PR does in fact add a os.statx function, though it returns a custom object type using getset and member descriptors. I would be (pleasantly) surprised if SimpleNamespace has competitive performance -- grepping for _PyNamespace_New, none of the uses are on fast paths -- but sure, I'll test it.

I really appreciate that you're focusing on the performance of this. I'm interested to see what you find!

Long term, we envision there becoming a higher level API than the os module as a path stat interface. It could use os.stat, os.statx, and other APIs under the hood. But os.stat itself should not become such an API.

I realize this is proposing that we ultimately create a new higher level API for stat. But things like .st_birthtime are not currently widely used (even if due to our own irony of simply being unavailabile from Python) so I don't worry about the nearer term need for platform conditional code in order to get something of that form today. Those are already necessary to handle code to handle Python versions that lack the feature on a given platform.

We should track a new higher level API with its own issue. That may wind up becoming a PEP-style discussion as it'd be a new high level API - to be determined.

Long term, we envision there becoming a higher level API than the os module as a path stat interface. It could use os.stat, os.statx, and other APIs under the hood. But os.stat itself should not become such an API.

pathlib can be such higher level API.

@gpshead @vstinner While I agree pathlib should provide a nicer interface for stat, you are more optimistic than I am as to how soon that nicer interface will arrive. I guess as core team members you have more control over when it happens. :)

I created a new PR #139178.

Pathlib updates for stat attributes are currently being discussed -- https://discuss.python.org/t/expose-file-size-mode-etc-from-pathlib-path-info/103828 questions/criticism/ideas welcome :).