Skip to content

bpo-45012 Release GIL around stat in os.scandir #28085

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

Merged
merged 1 commit into from
Sep 7, 2021

Conversation

skonieczny
Copy link
Contributor

@skonieczny skonieczny commented Aug 31, 2021

Releasing GIL allows other threads to continue
its work when os.scandir is fetching DirEntry.stat
info from file system.

It might be important in case of
slow or unresponsive file system.

https://bugs.python.org/issue45012

@the-knights-who-say-ni
Copy link

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept this contribution by verifying everyone involved has signed the PSF contributor agreement (CLA).

CLA Missing

Our records indicate the following people have not signed the CLA:

@skonieczny

For legal reasons we need all the people listed to sign the CLA before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

If you have recently signed the CLA, please wait at least one business day
before our records are updated.

You can check yourself to see if the CLA has been received.

Thanks again for the contribution, we look forward to reviewing it!

@skonieczny
Copy link
Contributor Author

@erlend-aasland I have squashed your suggestion into first commit

@@ -0,0 +1 @@
DirEntry object returned by os.scandir release GIL around stat, lstat and fstatat syscalls.
Copy link
Contributor

@erlend-aasland erlend-aasland Sep 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One more thing: please use reST markup in NEWS entries.

Suggested change
DirEntry object returned by os.scandir release GIL around stat, lstat and fstatat syscalls.
In :mod:`posix`, release GIL during ``stat()``, ``lstat()``, and
``fstatat()`` syscalls. Patch by Stanisław Skonieczny.

See the devguide for a reST primer :)

@erlend-aasland
Copy link
Contributor

erlend-aasland commented Sep 2, 2021

I have squashed your suggestion into first commit

Thanks! I left a small remark about your NEWS entry as well.

@skonieczny
Copy link
Contributor Author

Thanks! I will squash it into first commit, but I would like to keep info about os.scandir in the NEWS to make clear that this commit does not fix other function from os module.

Releasing GIL allows other threads to continue
its work when os.scandir is fetching DirEntry.stat
info from file system.

It might be important in case of
slow or unresponsive file system.
@skonieczny
Copy link
Contributor Author

@erlend-aasland
I have applied your changes and squashed everything into one commit.

@erlend-aasland erlend-aasland requested a review from ambv September 2, 2021 11:26
@erlend-aasland
Copy link
Contributor

erlend-aasland commented Sep 2, 2021

Thanks! I will squash it into first commit, but I would like to keep info about os.scandir in the NEWS to make clear that this commit does not fix other function from os module.

Ok. In that case, I'd consider rewording the NEWS entry further. Perhaps something like this:

In :func:`os.DirEntry.stat`, release GIL during ``stat()``, ``lstat()``, and
``fstatat()`` syscalls . Patch by Stanisław Skonieczny.

@erlend-aasland
Copy link
Contributor

By the way, using git fetch --no-ff main instead of git rebase main makes the PR easier to review, as it works well with GitHub. Also, we squash every PR that's merged into main, so it does not matter it you've got a lot of amending commits.

@skonieczny
Copy link
Contributor Author

skonieczny commented Sep 2, 2021

Also, we squash every PR that's merged into main, so it does not matter it you've got a lot of amending commits.

Good to know. Thanks!

@@ -13489,8 +13489,10 @@ _Py_COMP_DIAG_POP
if (self->dir_fd != DEFAULT_DIR_FD) {
#ifdef HAVE_FSTATAT
if (HAVE_FSTATAT_RUNTIME) {
Py_BEGIN_ALLOW_THREADS
result = fstatat(self->dir_fd, path, &st,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dir_fd isn't exposed to Python code and it's only set during object creation (in DirEntry_from_posix_info) so AFAICT this is safe.

@ambv ambv merged commit 9dc363e into python:main Sep 7, 2021
@skonieczny
Copy link
Contributor Author

@ambv Is there a chance to backport it to older python versions?

@ambv
Copy link
Contributor

ambv commented Sep 8, 2021

Not really. 3.10.0 is past rc2 and bugfix releases don't accept performance improvements.

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.

6 participants