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

Jump to bottom

gh-100288: Specialise LOAD_ATTR_METHOD for managed dictionaries #100289

Merged
merged 5 commits into from Dec 23, 2022
Merged

gh-100288: Specialise LOAD_ATTR_METHOD for managed dictionaries #100289

merged 5 commits into from Dec 23, 2022
+101 −43

Conversation

Fidget-Spinner
Copy link
Member

@Fidget-Spinner Fidget-Spinner commented Dec 16, 2022 •
edited by bedevere-bot

This specialisation targets the following code:

class A:
 def b(s): ...

a = A()
a.__dict__
def f():
 for _ in range(10):
  a.b() # This LOAD_ATTR_METHOD was previously not specialised.

f()

@bedevere-bot bedevere-bot mentioned this pull request Dec 16, 2022
Open
2 tasks
@netlify
Copy link

netlify bot commented Dec 16, 2022 •
edited

✅ Deploy Preview for python-cpython-preview ready!

Name Link
🔨 Latest commit fe02743
🔍 Latest deploy log https://app.netlify.com/sites/python-cpython-preview/deploys/639c4ad496e5300008bf5de4
😎 Deploy Preview https://deploy-preview-100289--python-cpython-preview.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@Fidget-Spinner Fidget-Spinner requested review from markshannon and brandtbucher and removed request for markshannon Dec 16, 2022
@Fidget-Spinner Fidget-Spinner requested a review from markshannon Dec 16, 2022
@Fidget-Spinner
Copy link
Member Author

Fidget-Spinner commented Dec 16, 2022

Interesting, I can create a reproducer:

import dis
import configparser as c

p = c.ConfigParser()
def f():
 for _ in range(10):
  print(p.optionxform("Test"))

f()
p.optionxform = lambda a:a
f() # supposed to be "Test" but prints "test"

markshannon
markshannon reviewed Dec 16, 2022
View changes
Copy link
Member

@markshannon markshannon left a comment

Is this case common enough to justify the extra instruction?

(Thinking aloud here)
And is this best approach? Can we modify the layout of objects so that it doesn't matter if the __dict__ has been materialized, provided the keys haven't changed?

Python/bytecodes.c Outdated Show resolved Hide resolved
@Fidget-Spinner
Copy link
Member Author

Fidget-Spinner commented Dec 16, 2022 •
edited

Is this case common enough to justify the extra instruction?

It's the highest specialisation failure for LOAD_ATTR we currently have (35.5% of 16.2% specialisation failures).

@markshannon
Copy link
Member

markshannon commented Dec 16, 2022

It's the highest specialisation failure for LOAD_ATTR we currently have (35.5% of 16.2% specialisation failures).

Can't argue with the data 🙂

OK, let's go with this approach. We can always looks for ways to improve the object layout later.

@Fidget-Spinner
Copy link
Member Author

Fidget-Spinner commented Dec 23, 2022

@markshannon is it alright if I merge this?

@markshannon
Copy link
Member

markshannon commented Dec 23, 2022

@markshannon is it alright if I merge this?

Yes, but I'm not pressing any merge buttons just before Christmas 🙂

@Fidget-Spinner
Copy link
Member Author

Fidget-Spinner commented Dec 23, 2022

Yes, but I'm not pressing any merge buttons just before Christmas 🙂

HAHA right. I'll press it and if anything goes wrong it's 100% on me to fix it :).

@Fidget-Spinner Fidget-Spinner merged commit c3c7848 into python:main Dec 23, 2022
20 checks passed
@Fidget-Spinner Fidget-Spinner deleted the more_load_attr branch Dec 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@markshannon markshannon

@brandtbucher brandtbucher

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

Successfully merging this pull request may close these issues.

None yet
3 participants
@Fidget-Spinner @markshannon @bedevere-bot