gh-116195: Implement win32_getppid_fast #116205
Conversation
Use the NtQueryInformationProcess system call to efficiently retrieve the parent process ID in a single step, rather than using the process snapshots API which retrieves large amounts of unnecessary information and is more prone to failure (since it makes heap allocations).
|
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 |
| @@ -9115,16 +9115,104 @@ os_setpgrp_impl(PyObject *module) | |||
| #ifdef HAVE_GETPPID | |||
|
|
|||
| #ifdef MS_WINDOWS | |||
| #include <processsnapshot.h> | |||
There was a problem hiding this comment.
I think this header file is still required because we are using PssCaptureSnapshot bellow.
There was a problem hiding this comment.
Indeed it is - missed that. Is there a way I can add commits to the PR without making a new one? On other repos just committing to my own branch worked.
There was a problem hiding this comment.
You can just add new commit to your branch, and this PR will be updated. Close a PR and create new one will make the review work harder.
| @@ -9115,16 +9115,104 @@ os_setpgrp_impl(PyObject *module) | |||
| #ifdef HAVE_GETPPID | |||
|
|
|||
| #ifdef MS_WINDOWS | |||
| #include <processsnapshot.h> | |||
There was a problem hiding this comment.
You can just add new commit to your branch, and this PR will be updated. Close a PR and create new one will make the review work harder.
|
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 |
1 similar comment
|
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 |
Co-authored-by: AN Long <aisk@users.noreply.github.com>
|
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 |
|
I see this PR is using the But I don't find any requirements for the naming convention, so I'm not confident about this. That is to say, if you want to keep them, we can wait for a core team member to make the convention clear. |
|
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 |
Alright, that has been fixed. |
There was a problem hiding this comment.
I only have a few code style nitpicks. Additionally, this change introduces a new static mutable variable. I don't know whether we should track it in the c-analyzer folder or do some other work.
Otherwise, this LGTM. Let's wait for a core team member to continue the review process.
Co-authored-by: AN Long <aisk@users.noreply.github.com>
|
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 |
Co-authored-by: AN Long <aisk@users.noreply.github.com>
|
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 |
Co-authored-by: AN Long <aisk@users.noreply.github.com>
|
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 |
Co-authored-by: AN Long <aisk@users.noreply.github.com>
|
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 |
|
Current change looks good to me now. Hi @zooba can you help us continue the review process as you have comments on the original issue? |
|
LGTM. I think we can add a NEWS entry for this, partly because there's a slight risk of changed behaviour, and also to let @vxiiduu claim credit. Thinking something like: |
|
Thank you! |
Use the NtQueryInformationProcess system call to efficiently retrieve the parent process ID in a single step, rather than using the process snapshots API which retrieves large amounts of unnecessary information and is more prone to failure (since it makes heap allocations). Includes a fallback to the original win32_getppid implementation in case the unstable API appears to return strange results.
Use the NtQueryInformationProcess system call to efficiently retrieve the parent process ID in a single step, rather than using the process snapshots API which retrieves large amounts of unnecessary information and is more prone to failure (since it makes heap allocations). Includes a fallback to the original win32_getppid implementation in case the unstable API appears to return strange results.
Use the NtQueryInformationProcess system call to efficiently retrieve the parent process ID in a single step, rather than using the process snapshots API which retrieves large amounts of unnecessary information and is more prone to failure (since it makes heap allocations). Includes a fallback to the original win32_getppid implementation in case the unstable API appears to return strange results.
win32_getppid has been supplemented with win32_getppid_fast which is more efficient.