Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

win,fs: use the new Windows fast stat API #4327

Open
wants to merge 1 commit into
base: v1.x
Choose a base branch
from

Conversation

huseyinacacak-janea
Copy link

Windows added a new API for file information, which doesn't have to open the file thus greatly improving performance: https://learn.microsoft.com/en-us/windows/win32/api/winbase/nf-winbase-getfileinformationbyname .

This PR implements this improvement on libuv. This was based on the Python implementation: https://github.com/python/cpython/blob/3a72fc36f93d40048371b789e32eefc97b6ade63/Modules/posixmodule.c#L2089-L2120 .

The stat functions are already covered by tests, so no test was added here. I considered comparing the result of old and new code, but that would require exposing internal fs functions, and we would be testing Windows functionality, not libuv.

@vtjnash
Copy link
Member

vtjnash commented Feb 27, 2024

Looks like the kernel API for this was added in Windows 10, version 1703 circa 2017 (EOL 2019), though not documented until circa 10/30/2019 https://learn.microsoft.com/en-us/windows-hardware/drivers/ddi/ntifs/nf-ntifs-ntqueryinformationbyname

@vtjnash
Copy link
Member

vtjnash commented Feb 27, 2024

The version number where this exact feature is supposed to be available starting in Windows 11 doesn't exist yet and/or isn't released yet, depending on which documentation page you go by:
https://learn.microsoft.com/en-us/windows-hardware/drivers/ddi/ntifs/ns-ntifs-file_stat_basic_information

But looks like python has been using this API since about 1 year ago, when it was added for them:
python/cpython#99726 (comment)

At a cursory glance, this looks great

@huseyinacacak-janea
Copy link
Author

@vtjnash thank you for your suggestion. The kernel API for this is available as of Windows 11, build 26048. The API NtQueryInformationByName could be used before, but we need the information from FileStatBasicInformation.

I use an API function GetFileInformationByName that is compatible with the kernel API, but with a more user-friendly interface. The API function uses the kernel API under the hood, so there is no noticeable difference in performance. Also, requiring this function lets us use this functionality only when it is available.

This is already available in Windows Insiders, which I have been using for testing.

@vtjnash
Copy link
Member

vtjnash commented Mar 4, 2024

Okay, it looks like 24H2 is 26052, which is consistent with the documentation saying >= 26048, released recently:
https://blogs.windows.com/windows-insider/2024/02/08/announcing-windows-11-insider-preview-build-26052-canary-and-dev-channels/

@huseyinacacak-janea
Copy link
Author

Rebased to include the sanitizer test fix. I see a new failure on macOS, I believe that's not related to the changes here.

@huseyinacacak-janea
Copy link
Author

@vtjnash is there anything else I can do to help here?

@vtjnash
Copy link
Member

vtjnash commented Mar 11, 2024

I think it is ready to go, I just figure it will get merged in a batch sometime before the next release

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants