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

bpo-45107: Make LOAD_METHOD_CLASS safer and faster, clean up comments #28177

Merged
merged 6 commits into from Sep 17, 2021

Conversation

Fidget-Spinner
Copy link
Member

@Fidget-Spinner Fidget-Spinner commented Sep 5, 2021

@Fidget-Spinner Fidget-Spinner changed the title [WIP] Optimize LOAD_METHOD specialization, clean up comments bpo-45107: Optimize LOAD_METHOD specialization, clean up comments Sep 5, 2021
@Fidget-Spinner Fidget-Spinner removed the request for review from markshannon Sep 5, 2021
@Fidget-Spinner
Copy link
Member Author

Fidget-Spinner commented Sep 5, 2021

I'll ping Mark when he's back.

IIUC, LOAD_METHOD_CLASS doesn't need to check cls.__dict__ at all. A single cls.tp_version_tag check is sufficient. So we can save on dict calculation and a branch.

@Fidget-Spinner Fidget-Spinner marked this pull request as ready for review Sep 5, 2021
@Fidget-Spinner Fidget-Spinner added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Sep 5, 2021
@bedevere-bot
Copy link

bedevere-bot commented Sep 5, 2021

🤖 New build scheduled with the buildbot fleet by @Fidget-Spinner for commit af0dd27 🤖

If you want to schedule another build, you need to add the "🔨 test-with-buildbots" label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Sep 5, 2021
Python/ceval.c Outdated Show resolved Hide resolved
@Fidget-Spinner
Copy link
Member Author

Fidget-Spinner commented Sep 5, 2021

@erlend-aasland the more I look at this the more iffy my code feels.

I'm trying to think if this could cause any segfaults. E.g. it accesses a non-type object at offsetoff(PyTypeObject, tp_version_tag, but the object size is smaller than that location, and it just crashes. I will try to produce some code to test my theory.

Previously the dict check also served as an object identity check since dk_version is unique. With that, accessing tp_version_tag is always safe.

This is guaranteed to always work though if all objects are bigger than offsetoff(PyTypeObject, tp_version_tag). I dont know if thats the case though. Do you remember?

@erlend-aasland
Copy link
Contributor

erlend-aasland commented Sep 6, 2021

I dont know if thats the case though. Do you remember?

I do not, and unfortunately I do not have any time to investigate now :(

@Fidget-Spinner Fidget-Spinner changed the title bpo-45107: Optimize LOAD_METHOD specialization, clean up comments bpo-45107: Make LOAD_METHOD_CLASS safer, clean up comments Sep 15, 2021
@Fidget-Spinner Fidget-Spinner requested a review from markshannon Sep 15, 2021
@Fidget-Spinner
Copy link
Member Author

Fidget-Spinner commented Sep 15, 2021

Thanks for the review Erlend, ultimately I decided to roll back the changes and make the checks stricter. My tests indicate we lost 0.1% of specialization hits, but the added safety is worth it. I can't prove that accessing tp_version_tag will never segfault in all cases.

@markshannon markshannon self-assigned this Sep 15, 2021
Copy link
Member

@markshannon markshannon left a comment

It looks like we missed a check that the operand of LOAD_METHOD_CLASS is actually a class.

Python/ceval.c Outdated Show resolved Hide resolved
Python/ceval.c Outdated Show resolved Hide resolved
@Fidget-Spinner Fidget-Spinner changed the title bpo-45107: Make LOAD_METHOD_CLASS safer, clean up comments bpo-45107: Make LOAD_METHOD_CLASS safer and faster, clean up comments Sep 16, 2021
@markshannon
Copy link
Member

markshannon commented Sep 16, 2021

Looks good, thanks.
Don't know what is up with the Windows builds 😕

@Fidget-Spinner
Copy link
Member Author

Fidget-Spinner commented Sep 16, 2021

FWIW, I can't repro the windows build errors locally. It seems to be affecting every PR right now.

@Fidget-Spinner Fidget-Spinner merged commit 70bed6f into python:main Sep 17, 2021
11 checks passed
@Fidget-Spinner Fidget-Spinner deleted the micro_opt_lm_class branch Sep 17, 2021
@Fidget-Spinner
Copy link
Member Author

Fidget-Spinner commented Sep 17, 2021

Thanks Mark and Erlend for the reviews!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants