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-116195: Implement win32_getppid_fast#116205

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 11 commits intopython:mainfromunknown repositoryMar 14, 2024
Merged

gh-116195: Implement win32_getppid_fast#116205

zooba merged 11 commits intopython:mainfromunknown repositoryMar 14, 2024

Conversation

@ghost
Copy link

@ghostghost commentedMar 1, 2024•
edited by bedevere-appbot
Loading

win32_getppid has been supplemented with win32_getppid_fast which is more efficient.

vxiiduu added3 commitsMarch 1, 2024 21:16
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).
@bedevere-app
Copy link

Most changes to Pythonrequire a NEWS entry. Add one using theblurb_it web app or theblurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply theskip news label instead.

#ifdefHAVE_GETPPID

#ifdefMS_WINDOWS
#include<processsnapshot.h>
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this header file is still required because we are usingPssCaptureSnapshot bellow.

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Eclips4 reacted with thumbs up emoji
@ghostghost closed thisMar 1, 2024
#ifdefHAVE_GETPPID

#ifdefMS_WINDOWS
#include<processsnapshot.h>
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Eclips4 reacted with thumbs up emoji
@ghostghost reopened thisMar 1, 2024
@bedevere-app
Copy link

Most changes to Pythonrequire a NEWS entry. Add one using theblurb_it web app or theblurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply theskip news label instead.

1 similar comment
@bedevere-app
Copy link

Most changes to Pythonrequire a NEWS entry. Add one using theblurb_it web app or theblurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply theskip news label instead.

Co-authored-by: AN Long <aisk@users.noreply.github.com>
@bedevere-app
Copy link

Most changes to Pythonrequire a NEWS entry. Add one using theblurb_it web app or theblurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply theskip news label instead.

@aisk
Copy link
Contributor

aisk commentedMar 3, 2024•
edited
Loading

I see this PR is using theCamelCase naming convention for local variables, likeCachedParentProcessId andNtdll. Although this is not described in PEP 7, most of the Windows part of this file uses the snake_case naming convention for local variables. I think we should keep them consistent.

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.

@bedevere-app
Copy link

Most changes to Pythonrequire a NEWS entry. Add one using theblurb_it web app or theblurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply theskip news label instead.

@ghost
Copy link
Author

I see this PR is using theCamelCase naming convention for local variables, likeCachedParentProcessId andNtdll. Although this is not described in PEP 7, most of the Windows part of this file uses the snake_case naming convention for local variables. I think we should keep them consistent.

Alright, that has been fixed.

aisk reacted with thumbs up emoji

Copy link
Contributor

@aiskaisk left a comment•
edited
Loading

Choose a reason for hiding this comment

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

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 thec-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>
@bedevere-app
Copy link

Most changes to Pythonrequire a NEWS entry. Add one using theblurb_it web app or theblurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply theskip news label instead.

Co-authored-by: AN Long <aisk@users.noreply.github.com>
@bedevere-app
Copy link

Most changes to Pythonrequire a NEWS entry. Add one using theblurb_it web app or theblurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply theskip news label instead.

Co-authored-by: AN Long <aisk@users.noreply.github.com>
@bedevere-app
Copy link

Most changes to Pythonrequire a NEWS entry. Add one using theblurb_it web app or theblurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply theskip news label instead.

Co-authored-by: AN Long <aisk@users.noreply.github.com>
@bedevere-app
Copy link

Most changes to Pythonrequire a NEWS entry. Add one using theblurb_it web app or theblurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply theskip news label instead.

@aisk
Copy link
Contributor

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?

@zooba
Copy link
Member

zooba commentedMar 14, 2024•
edited
Loading

LGTM. I think we can add a NEWS entry for this, partly because there's aslight risk of changed behaviour, and also to let@vxiiduu claim credit.

Thinking something like:Improves performance of :func:`os.getppid` by using an alternate system API when available. Contributed by <vxiiduu's choice of name here> (in the Windows category, so it doesn't need to explicitly say "on Windows")

aisk reacted with thumbs up emoji

@zoobazooba merged commitbe1c808 intopython:mainMar 14, 2024
@zooba
Copy link
Member

Thank you!

vstinner pushed a commit to vstinner/cpython that referenced this pull requestMar 20, 2024
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.
adorilson pushed a commit to adorilson/cpython that referenced this pull requestMar 25, 2024
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.
diegorusso pushed a commit to diegorusso/cpython that referenced this pull requestApr 17, 2024
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.
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@aisk@zooba
[8]ページ先頭
©2009-2025 Movatter.jp