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-124153: Remove _PyType_GetModuleByDef2 private function #124261

Merged
merged 8 commits into from
Sep 26, 2024

Conversation

neonene
Copy link
Contributor

@neonene neonene commented Sep 20, 2024

Unlike PyType_GetModuleByDef(), this uses PyType_HasFeature() for differentiation.
This will not be inlined, but hopefully 4% faster on Windows PGO.  The forceinline version was less efficient (2% faster).
Objects/typeobject.c Outdated Show resolved Hide resolved
PyType_GetModuleByDef() is now slower.
@neonene

This comment was marked as outdated.

@neonene

This comment was marked as outdated.

@encukou
Copy link
Member

encukou commented Sep 24, 2024

Looks like this removes the only call to _PyType_GetModuleByDef2; could you also remove the function?

@vstinner
Copy link
Member

Looks like this removes the only call to _PyType_GetModuleByDef2; could you also remove the function?

@neonene prepared #124262 but this follow-up PR cannot be merged before the code is updated to no longer call _PyType_GetModuleByDef2 ;-)

@neonene
Copy link
Contributor Author

neonene commented Sep 24, 2024

@neonene prepared #124262 but this follow-up PR cannot be merged before the code is updated to no longer call _PyType_GetModuleByDef2 ;-)

I've port the header update from #124262 to here. Is this what you mean?

@vstinner
Copy link
Member

Why not removing _PyType_GetModuleByDef2() body as well?

@neonene neonene changed the title gh-124153: Use Py_tp_token slot and PyType_GetBaseByToken() in a few modules gh-124153: Remove _PyType_GetModuleByDef2 private function Sep 24, 2024
@neonene
Copy link
Contributor Author

neonene commented Sep 25, 2024

(Does it really make sense to remove _PyType_GetModuleByDef2() before checking the features on main?)

@encukou
Copy link
Member

encukou commented Sep 25, 2024

To be honest I don't follow too closely the order in which the changes here should be made/merged.
I also have no way to benchmark with MSVC, so I trust you about the performance, and just review for functionality and readability :)

If this removes the last call to _PyType_GetModuleByDef2, and we don't plan to bring the function back, then it should be removed now. If it turns out to be needed again, we can find it in Git history.

Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

LGTM

@encukou encukou merged commit d7248cd into python:main Sep 26, 2024
37 checks passed
@neonene neonene deleted the usetoken branch September 26, 2024 16:51
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.

3 participants