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

gh-120906: Support arbitrary hashable keys in FrameLocalsProxy #122309

Merged
merged 10 commits into from
Jul 30, 2024

Conversation

encukou
Copy link
Member

@encukou encukou commented Jul 26, 2024

Make FrameLocalsProxy support arbitrary keys for lookup and update.

The given key needs to be hashable. It's a mapping, after all.
For the linear scan through the "real" locals:

  • If the given key is an interned exact string, we use pointer comparison
  • Otherwise, we fall back to generic PyObject_RichCompareBool
    If there's no match, we know it matches none of the "real" locals,
    and the read/write functions fall back to the "extras" dict.

Add tests, including tests for the mapping protocol (with selected exceptions).

@gaogaotiantian
Copy link
Member

So for the first glance, there are two questions:

  1. What if the key equals to a unicode string, but has a different hash? I know it's not "supposed" to be, but that could happen and this implementation has a different behavior than a real dict.
  2. The separate loop was intentional - do a quick pointer equality check first, then do the slow check. It will save us some time. I think we should keep that.

I did no go through the whole change because I don't believe we've reached to the point where we agree how to do this.

@encukou
Copy link
Member Author

encukou commented Jul 27, 2024

  1. I don't think we need to match dict in this anomalous case.
    The dict behaviour itself depends on internal implementation details one shouldn't rely on.

    However, we can use the hash to skip PyObject_RichCompareBool calls, which seems beneficial on its own.
    (But it doesn't match dict behaviour exactly. Dict doesn't use the full hash; how much of the hash it uses depends on the internal table size.)

  2. OK. (I'd be surprised if the speedup didn't matter in real-world use cases, so I went for simpler code before.)

Lib/test/test_frame.py Show resolved Hide resolved
Lib/test/test_frame.py Outdated Show resolved Hide resolved
Copy link
Contributor

@ncoghlan ncoghlan left a comment

Choose a reason for hiding this comment

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

General approach LGTM, some suggestions inline regarding test case details.

Lib/test/test_frame.py Outdated Show resolved Hide resolved
Lib/test/test_frame.py Outdated Show resolved Hide resolved
Lib/test/test_frame.py Outdated Show resolved Hide resolved
Lib/test/test_frame.py Outdated Show resolved Hide resolved
Co-authored-by: Alyssa Coghlan <ncoghlan@gmail.com>

// We do 2 loops here because it's highly possible the key is interned
// and we can do a pointer comparison.
for (int i = 0; i < co->co_nlocalsplus; i++) {
PyObject *name = PyTuple_GET_ITEM(co->co_localsplusnames, i);
if (name == key) {
found_key = true;
Copy link
Member

Choose a reason for hiding this comment

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

If you removed found_key check here, maybe just return -1 if the condition does not meet. Otherwise it will go through the loop again and check something we already knew.

Copy link
Member

Choose a reason for hiding this comment

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

The rest of the PR LGTM.

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, thanks for the catch!

Copy link
Member Author

Choose a reason for hiding this comment

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

Could you check the change?
I'm still not too familiar with the internals of this mapping; I just found out (hopefully correctly) what the undocumented CO_FAST_HIDDEN does.

Copy link
Contributor

Choose a reason for hiding this comment

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

The change looks correct to me (with the search flag restored, the CO_FAST_HIDDEN handling is unchanged from @gaogaotiantian's original PR).

I did suggest an explanatory comment on the early return, but that's definitely not a merge blocker. So even if @gaogaotiantian doesn't get time to take a look, I think this is good to go for rc1.

Copy link
Contributor

@ncoghlan ncoghlan Jul 30, 2024

Choose a reason for hiding this comment

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

Capturing my understanding of the CO_FAST_HIDDEN handling, since it's genuinely not clear:

  • in Python 3.12, the iteration variables for inlined comprehensions are visible for reads while the comprehension(s) is(/are) running, but not writable (even with LocalsToFast shenanigans), and not visible outside the comprehension(s)
  • in Python 3.13, all of those properties remain true even though sys._getframe().f_locals produces fast locals proxy instances during iteration (writes to hidden variables go into the frame's "extra locals" storage rather than modifying the comprehension iteration variable)

Future versions could revisit that design if debugger developers ask for it to change, but live editing loop variables seems sufficiently fraught with peril that I'll be genuinely surprised if anyone ever notices this small discrepancy between module level comprehensions and function level ones.

Co-authored-by: Alyssa Coghlan <ncoghlan@gmail.com>
@encukou
Copy link
Member Author

encukou commented Jul 30, 2024

@gaogaotiantian and/or @markshannon, your review would be appreciated, so we can get this in 3.13.

@markshannon
Copy link
Member

Looks good.

There is one test I'd like to added, though.
This little program now succeeds (it fails on main):

class Str(str):
    pass

def foo():
    sys._getframe().f_locals[Str("x")] = 2
    assert x == 2
    x = 1

foo()

but it doesn't seem like there is a test for this case (writing a fast local using a non-str key).
Could you add a test for this?

@encukou
Copy link
Member Author

encukou commented Jul 30, 2024

Will do in ~6 hours, feel free to beat me to it.

@encukou
Copy link
Member Author

encukou commented Jul 30, 2024

Test added. (There was one for writing a fast local using a non-str key, but not for writing a so far uninitialized local like your example, so I added that.)

Thank you for the reviews!

@encukou encukou enabled auto-merge (squash) July 30, 2024 22:06
@encukou encukou merged commit 5912487 into python:main Jul 30, 2024
37 checks passed
@encukou encukou added the needs backport to 3.13 bugs and security fixes label Jul 30, 2024
@miss-islington-app
Copy link

Thanks @encukou for the PR 🌮🎉.. I'm working now to backport this PR to: 3.13.
🐍🍒⛏🤖

@miss-islington-app
Copy link

Sorry, @encukou, I could not cleanly backport this to 3.13 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker 5912487938ac4b517209082ab9e6d2d3d0fb4f4d 3.13

@encukou encukou deleted the frame_locals_eq branch July 30, 2024 22:22
encukou added a commit to encukou/cpython that referenced this pull request Jul 30, 2024
…sProxy (pythonGH-122309)

Co-authored-by: Alyssa Coghlan <ncoghlan@gmail.com>
(cherry picked from commit 5912487)
encukou added a commit to encukou/cpython that referenced this pull request Jul 30, 2024
…sProxy (pythonGH-122309)

Co-authored-by: Alyssa Coghlan <ncoghlan@gmail.com>
(cherry picked from commit 5912487)
@bedevere-app
Copy link

bedevere-app bot commented Jul 30, 2024

GH-122488 is a backport of this pull request to the 3.13 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.13 bugs and security fixes label Jul 30, 2024
Yhg1s pushed a commit that referenced this pull request Jul 31, 2024
…GH-122309) (#122488)

[3.13] gh-120906: Support arbitrary hashable keys in FrameLocalsProxy  (GH-122309)

Co-authored-by: Alyssa Coghlan <ncoghlan@gmail.com>
(cherry picked from commit 5912487)
blhsing pushed a commit to blhsing/cpython that referenced this pull request Aug 22, 2024
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.

5 participants