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.

Sign up for GitHub

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

Jump to bottom

gh-74929: Make containment checks more efficient in FrameLocalsProxy #118624

Merged
merged 1 commit into from
May 6, 2024
Merged

gh-74929: Make containment checks more efficient in FrameLocalsProxy #118624

merged 1 commit into from
May 6, 2024

Conversation

gaogaotiantian
Copy link
Member

@gaogaotiantian gaogaotiantian commented May 6, 2024 •
edited by bedevere-app bot
Loading

Currently the more efficient contains function is not used when someone does key in frame.f_locals because that defaults to sq_contains and it's not properly hooked. The function is only called when using frame.f_locals.__contains__(key).

This hooks sq_contains so the in operation is much more efficient.

@gaogaotiantian gaogaotiantian requested review from gvanrossum and removed request for markshannon May 6, 2024 04:01
@bedevere-app bedevere-app bot mentioned this pull request May 6, 2024
Closed
@gvanrossum
Copy link
Member

Is this a correctness issue or merely a performance issue?

@gaogaotiantian
Copy link
Member Author

This is merely a performance issue. Without this change, the CONTAINS_OP will treat the proxy as a normal iterable and check the keys one by one. The iter() method worked fine so the result is correct. A list of keys will be generated and iterated.

@ncoghlan ncoghlan changed the title gh-74929: Make contains work properly for FrameLocalsProxy gh-74929: Make containment checks more efficient in FrameLocalsProxy May 6, 2024
ncoghlan
ncoghlan approved these changes May 6, 2024
View reviewed changes
Copy link
Contributor

@ncoghlan ncoghlan 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.

LGTM! (PR title tweaked to better reflect its purpose)

@ncoghlan ncoghlan merged commit afbe5bf into python:main May 6, 2024
38 checks passed
@gaogaotiantian gaogaotiantian deleted the frameproxy-contain branch May 6, 2024 17:28
SonicField pushed a commit to SonicField/cpython that referenced this pull request May 8, 2024
@gaogaotiantian @SonicField
pythongh-74929: Make containment checks more efficient in `FrameLocal…
e01dae6 
…sProxy` (python#118624)

Properly implement the `sq_contains` slot for frame locals proxy containment checks.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ncoghlan ncoghlan ncoghlan approved these changes

@gvanrossum gvanrossum Awaiting requested review from gvanrossum

@markshannon markshannon Awaiting requested review from markshannon markshannon is a code owner

Assignees
No one assigned
Labels
skip news
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@gaogaotiantian @gvanrossum @ncoghlan