Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

gh-99726: Improves correctness of stat results for Windows, and uses faster API when available#102149

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

Merged
zooba merged 13 commits intopython:mainfromzooba:gh-99726-2
Mar 16, 2023

Conversation

zooba
Copy link
Member

@zoobazooba commentedFeb 22, 2023
edited
Loading

@zooba
Copy link
MemberAuthor

Open questions on this are:

  • does it work with the new Windows API (I'll test when I get access to a build with it)
  • can we changest_ctime immediately or is it better to just not expose ChangeTime and use bothst_ctime andst_birthtime for the same value?

@zoobazooba self-assigned thisFeb 24, 2023
@zooba
Copy link
MemberAuthor

I decided that we can't just changest_ctime, so it's deprecated instead (in docs) but still returns the same asst_birthtime for now.

@zoobazooba marked this pull request as ready for reviewMarch 6, 2023 22:53
@zoobazooba requested a review froma team as acode ownerMarch 6, 2023 22:53
@zooba
Copy link
MemberAuthor

This supersedes#99755, or at least will conflict so much that it will need to be reimplemented.

@zooba
Copy link
MemberAuthor

@eryksun Could you take a look at this for me? Also, the API is slightly different from the last time you looked - we got some changes made in the OS to suit our needs better.

@DefaultRyan
Copy link

@zooba , I was analyzing some data from these changes, and I noticed that we have some "slow" calls from isdir/isfile/exists that are no longer calling stat due to#101324 (gh-101196: Make isdir/isfile/exists faster on Windows) by@mdboom . I suspect that those functions could be made faster by using the new and improved stat, when it exists. Not exists() might be a wash, but it's probably worth checking and running those benchmarks from#101324 again.

@zooba
Copy link
MemberAuthor

They shouldn't really be able to be any faster.GetFileAttributesW was already using the fast path that we now have for more info with the new API, so in theory they should be slightly faster due to a little less memcpy.

But since you've confirmed you're running these changes and haven't complained, I'll take that as the best signal I'm going to get that they're good and merge it in (tomorrow, just in case)!

@zoobazooba merged commit0f17576 intopython:mainMar 16, 2023
@zoobazooba deleted the gh-99726-2 branchMarch 16, 2023 17:27
@DefaultRyan
Copy link

They shouldn't really be able to be any faster.GetFileAttributesW was already using the fast path that we now have for more info with the new API, so in theory they should be slightly faster due to a little less memcpy.

But since you've confirmed you're running these changes and haven't complained, I'll take that as the best signal I'm going to get that they're good and merge it in (tomorrow, just in case)!

Thanks for replying, but I'm not sure I understand. The newisdir/isfile/islink are no longer callingos_stat, but are callingCreateFile +GetFileInformationByHandleEx +CloseHandle, which is the "slow" pattern that this PR was replacing withGetFileInformationByName for better perf. So with this change,stat is faster, butisdir/isfile/islink are still running the slower code.

What am I missing here?

@zooba
Copy link
MemberAuthor

Huh, you're right. You're missing that I misremembered how we implemented those.

They should probably change toGetFileAttributes and check for a reparse point that way, then use the slow path for symlinks (except for the case where we only want the symlink).

In any case, now this is merged, it can be a new issue.

@eryksun
Copy link
Contributor

@zooba, I was waiting to review this PR untilGetFileInformationByName() is pushed to the general availability channel. I'm surprised that you merged code that depends on a new Windows API function that isn't generally available and has no documentation -- not even a blog post. CPython has never been that aggressive in adopting new Windows features.

They shouldn't really be able to be any faster. GetFileAttributesW was already using the fast path that we now have for more info with the new API, so in theory they should be slightly faster due to a little less memcpy.

The newntpath.is*() functions (e.g.os__path_isdir_impl) useCreateFileW() andGetFileInformationByHandleEx(). UsingGetFileAttributesW() would still require a fallback implementation that callsCreateFileW() for reparse points. It was simpler to implement just theCreateFileW() path. Also, testing showed that usingCreateFileW() on balance was about as fast or faster thanGetFileAttributesW(). For reparse points, using justCreateFileW() was obviously faster, since we have to callCreateFileW() anyway. Here'swhat I said at the time:

For me, this implementation takes about about a third less time than usingos.stat(). It takes about 10% more time than usingGetFileAttributesW(), but usingGetFileAttributesW() is significantly more expensive for a reparse point (e.g. symlink, junction) becauseCreateFileW() has to be called to traverse it, which means the file is opened and queried twice.

On the other hand, at the time, my tests showed thatNtQueryInformationByName() was significantly faster thanNtQueryAttributesFile().NtQueryInformationByName() uses a completely different implementation in the kernel. In principle,NtQueryAttributesFile() could take the same path, so maybe they've updated its implementation in the development channel. Either way, thentpath.is*() functions would benefit from using this faster path to the filesystem information.

@zooba
Copy link
MemberAuthor

I'm surprised that you merged code that depends on a new Windows API function that isn't generally available and has no documentation -- not even a blog post. CPython has never been that aggressive in adopting new Windows features.

It's not a dependency - the behaviour is identical regardless of whether the API is present or not - it's just a perf optimisation when the API is available.

And we've always been this quick to adopt Windows features when I've been the one advocating to get them added into Windows for the sake of Python ;) There just haven't been a whole lot of them.

The newntpath.is*() functions ...

Yes, I acknowledged that I made a mistake with my earlier statement, and there's a new bug to update them and I already have someone looking at it (when they get time - if not, I'll probably get to it myself). Having gone through the functionality with one of the filesystem devs,GetFileAttributesW should be optimal for the non-reparse case, and as it will tell us if it was a reparse point, it's a more efficient first check thanGetFileInformationByName (which also has the same fallback). So it doesn't actually depend on this change at all.

carljm added a commit to carljm/cpython that referenced this pull requestMar 17, 2023
* main: (34 commits)pythongh-102701: Fix overflow in dictobject.c (pythonGH-102750)pythonGH-78530: add support for generators in `asyncio.wait` (python#102761)  Increase stack reserve size for Windows debug builds to avoid test crashes (pythonGH-102764)pythongh-102755: Add PyErr_DisplayException(exc) (python#102756)  Fix outdated note about 'int' rounding or truncating (python#102736)pythongh-102192: Replace PyErr_Fetch/Restore etc by more efficient alternatives (python#102760)pythongh-99726: Improves correctness of stat results for Windows, and uses faster API when available (pythonGH-102149)pythongh-102192: remove redundant exception fields from ssl module socket (python#102466)pythongh-102192: Replace PyErr_Fetch/Restore etc by more efficient alternatives (python#102743)pythongh-102737: Un-ignore ceval.c in the CI globals check (pythongh-102745)pythonGH-102748: remove legacy support for generator based coroutines from `asyncio.iscoroutine` (python#102749)pythongh-102721: Improve coverage of `_collections_abc._CallableGenericAlias` (python#102722)pythonGH-102653: Make recipe docstring show the correct distribution (python#102742)  Add comments to `{typing,_collections_abc}._type_repr` about each other (python#102752)pythongh-102594: PyErr_SetObject adds note to exception raised on normalization error (python#102675)pythongh-94440: Fix issue of ProcessPoolExecutor shutdown hanging (python#94468)pythonGH-100112:  avoid using iterable coroutines in asyncio internally (python#100128)pythongh-102690: Use Edge as fallback in webbrowser instead of IE (python#102691)pythongh-102660: Fix Refleaks in import.c (python#102744)pythongh-102738: remove from cases generator the code related to register instructions (python#102739)  ...
Fidget-Spinner pushed a commit to Fidget-Spinner/cpython that referenced this pull requestMar 27, 2023
… uses faster API when available (pythonGH-102149)This deprecates `st_ctime` fields on Windows, with the intent to change them to contain the correct value in 3.14. For now, they should keep returning the creation time as they always have.
warsaw pushed a commit to warsaw/cpython that referenced this pull requestApr 11, 2023
… uses faster API when available (pythonGH-102149)This deprecates `st_ctime` fields on Windows, with the intent to change them to contain the correct value in 3.14. For now, they should keep returning the creation time as they always have.
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers
No reviews
Assignees

@zoobazooba

Labels
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

ctime: I don't think that word means what you think it means.
4 participants
@zooba@DefaultRyan@eryksun@bedevere-bot

[8]ページ先頭

©2009-2025 Movatter.jp