gh-83714: Implement os.statx #139178
Merged
gh-83714: Implement os.statx #139178
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jbosboom
requested review from
AA-Turner,
corona10,
emmatyping and
erlend-aasland
as code owners
vstinner
reviewed
Sep 22, 2025
Member
vstinner
left a comment
vstinner
left a comment
There was a problem hiding this comment.
Thanks. Here is a first review.
Co-authored-by: Victor Stinner <[email protected]>
vstinner
reviewed
Sep 23, 2025
Co-authored-by: Victor Stinner <[email protected]>
Contributor
Author
|
In addition to making the changes you directed, when modifying the
and |
vstinner
reviewed
Sep 24, 2025
Member
|
I'm not fully comfortable with having some os.statx() tests in test_posix and some others in test_os. But it seems like you reused existing code in test_posix and test_os, so I would say that I'm fine with it. We might merge test_posix and test_os into a single test, but that's a different topic. |
Member
I created #139322 for that :-) |
Contributor
Author
|
Do you still plan to merge |
Member
|
The plan to split test_os is stale for now. You can rebase your PR. |
Co-authored-by: Victor Stinner <[email protected]>
Contributor
Author
vstinner
reviewed
Oct 10, 2025
Member
vstinner
left a comment
vstinner
left a comment
There was a problem hiding this comment.
Overall, the change LGTM. There is just one last open question of adding a default value to the mask parameter or not.
cmaloney
reviewed
Oct 11, 2025
Contributor
cmaloney
left a comment
cmaloney
left a comment
There was a problem hiding this comment.
Overall really like this. I don't have a strong preference around the defaults. I think it would be good to have a documentation expert check the new docs over.
vstinner
reviewed
Oct 13, 2025
vstinner
reviewed
Oct 13, 2025
Member
|
This API is uneasy to use. It took me a while to understand To make the implementation simpler, I suppose that we should leave |
vstinner
approved these changes
Oct 14, 2025
Modules/posixmodule.c
Outdated
| } | ||
| } | ||
| else { | ||
| pystatx_result_spec.name = "os.statx_result"; |
Member
There was a problem hiding this comment.
Why not using os.statx_result name directly in pystatx_result_spec? (line 3505)
Contributor
Author
There was a problem hiding this comment.
I don't know. I was just following the pattern of other types created in posixmodule_exec, most of which assign their name here. The tests pass using os.statx_result directly in the spec, so I'll make that change.
Member
There was a problem hiding this comment.
Oh. That's a strange pattern 😄 I don't know why existing code does that.
Member
There was a problem hiding this comment.
I wrote #140168 to set type names earlier in posixmodule.c.
vstinner
reviewed
Oct 14, 2025
Member
vstinner
left a comment
vstinner
left a comment
There was a problem hiding this comment.
Oh you should also document the new os.statx() function by adding a new "os" section near: https://docs.python.org/dev/whatsnew/3.15.html#os-path (Doc/whatsnew/3.15.rst).
Contributor
Author
Yes, I did this.
Done. |
cmaloney
reviewed
Oct 15, 2025
cmaloney
reviewed
Oct 15, 2025
Contributor
|
👍 looking good to me with the kwarg doc change added in. |
Co-authored-by: Cody Maloney <[email protected]>
cmaloney
approved these changes
Oct 15, 2025
vstinner
reviewed
Oct 15, 2025
Member
|
I enabled auto-merge, don't touch anything 😁 |
|
|
|
Member
|
Oh no! AMD64 CentOS9 NoGIL 3.x buildbot build fails with: test.pythoninfo: ARM Raspbian 3.x and ARM64 Raspbian Debug 3.x also fail to build with: test.pythoninfo: |
Member
Contributor
Author
|
@vstinner I know what's wrong and how to fix it. I'll have a PR up later today. |
Contributor
Author
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR implements
os.statx, an interface to the Linuxstatx(2)system call introduced in kernel version 4.11 (April 2017) and first available in glibc version 2.28 (August 2018). This is derived from my earlier PR #136334 with the changes toos.statremoved, plus other changes (see below).This PR implements the return value of
os.statxwith a custom C typestatx_result, which contains astruct statxand uses member and getset descriptors to lazily create Python objects on attribute access. In a comment on the previous PR, @vstinner suggested usingtypes.SimpleNamespaceas the return value. I implemented that on the statx-simplenamespace branch in my fork. Using the benchmarks from this script (note they are not all fair comparisons):It's slower than this PR when only size and mtime are requested, because it creates objects for the unconditionally valid members and both the float-seconds and int-nanoseconds timestamps. We could get some of this back by defining our own mask bits (bit 32 and above). As more bits are set in the mask and attributes are accessed, the gap widens. I'm not sure if that's due to dict resizing or slower attribute access or both, and I don't see how to improve either. The advantage of the namespace implementation is its simplicity, and any speed hacks would dilute that.
(Also, because it doesn't create unrequested members, it's not a perfect wrapper around the system call. For "real" use it doesn't matter, but for testing the syscall, you'll miss some quirks. For example, btrfs seems to always return atime and btime, but returns mtime and ctime (always together) only if mtime and/or ctime were requested.)
In this PR, most attributes are implemented with member descriptors pointing into the
struct statxor a member instatx_result, so each attribute access onstatx_resultcreates a new int or float object. My previous PR cached the created objects in thestatx_resultfor the commonly-used attributes to avoid creating them more than once, which requires getset descriptors. Obviously, creating objects has a cost, but getset descriptors are slower than member descriptors. The crossover point turns out to be about two accesses (per attribute). If an attribute is only used once, this PR is faster; twice, slightly faster or even; three or more times, the previous PR is faster. I decided the complexity of the cache wasn't worth it. There's a cleaned-up version of the caching implementation on my fork if you want to take your own measurements.(I also didn't test the cache in the free-threaded build; I think the atomics/locking is correct, but I don't really understand how critical sections that can be "suspended" can possibly be safe.)
📚 Documentation preview 📚: https://cpython-previews--139178.org.readthedocs.build/