Skip to content

bpo-30509: Optimize and clean up calling type slots. #1861

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
Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

bpo-30509: Optimize and clean up calling type slots. #1861

wants to merge 7 commits into from

Conversation

serhiy-storchaka
Copy link
Member

No description provided.

@serhiy-storchaka serhiy-storchaka added the performance Performance or resource usage label May 30, 2017
@mention-bot
Copy link

@serhiy-storchaka, thanks for your PR! By analyzing the history of the files in this pull request, we identified @tim-one, @larryhastings and @benjaminp to be potential reviewers.

vstinner
vstinner reviewed May 30, 2017
View reviewed changes
Objects/typeobject.c Outdated
*/

static PyObject *
lookup_maybe(PyObject *self, _Py_Identifier *attrid)
PyObject *
Copy link
Member

Choose a reason for hiding this comment

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

Why do you make this function public? (you removed static)

Copy link
Member Author

Choose a reason for hiding this comment

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

lookup_maybe() was used only in _PyObject_LookupSpecial() and set_names(). The latter is not performance critical and can use _PyObject_LookupSpecial() instead of lookup_maybe(). Thus I just inlined lookup_maybe() in the single place where it is used, in _PyObject_LookupSpecial().

Copy link
Member

Choose a reason for hiding this comment

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

Oh, I see. I didn't notice that _PyObject_LookupSpecial() already exist in the current code. I checked, it's called from various places in CPython :-) Your change makes sense ;-)

vstinner
vstinner reviewed Jun 13, 2017
View reviewed changes
Objects/typeobject.c Outdated
@@ -6209,6 +6225,7 @@ slot_tp_hash(PyObject *self)
}

if (func == NULL) {
PyErr_Clear();
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand why PyErr_Clear() is needed with your change.

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually it is not needed.

vstinner
vstinner approved these changes Jun 14, 2017
View reviewed changes
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.

While I am not 100% convinced that the change makes Python faster, the change itself now LGTM thanks to the first cleanup commit. I let you decide to merge it or not ;-) (my vote is +0)

@Mariatta Mariatta closed this Jul 5, 2018
@Mariatta Mariatta reopened this Jul 5, 2018
@csabella
Copy link
Contributor

@serhiy-storchaka were you interested in revisiting this to see if it should be merged or not? Thanks!

@github-actions
Copy link

This PR is stale because it has been open for 30 days with no activity.

@github-actions github-actions bot added the stale Stale PR or inactive for long period of time. label Feb 19, 2022
@github-actions github-actions bot removed the stale Stale PR or inactive for long period of time. label Jul 28, 2022
@github-actions
Copy link

This PR is stale because it has been open for 30 days with no activity.

@github-actions github-actions bot added the stale Stale PR or inactive for long period of time. label Aug 28, 2022
@github-actions github-actions bot removed the stale Stale PR or inactive for long period of time. label Jan 7, 2023
@github-actions
Copy link

github-actions bot commented Feb 6, 2023

This PR is stale because it has been open for 30 days with no activity.

@github-actions github-actions bot added the stale Stale PR or inactive for long period of time. label Feb 6, 2023
@github-actions github-actions bot removed the stale Stale PR or inactive for long period of time. label Feb 22, 2023
@github-actions
Copy link

This PR is stale because it has been open for 30 days with no activity.

@github-actions github-actions bot added the stale Stale PR or inactive for long period of time. label Mar 25, 2023
@vstinner
Copy link
Member

vstinner commented Apr 5, 2023

It would be interesting to rebase this PR on the main branch.

@github-actions github-actions bot removed the stale Stale PR or inactive for long period of time. label Apr 6, 2023
@github-actions
Copy link

github-actions bot commented May 7, 2023

This PR is stale because it has been open for 30 days with no activity.

@github-actions github-actions bot added the stale Stale PR or inactive for long period of time. label May 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vstinner vstinner vstinner approved these changes

Assignees
No one assigned
Labels
DO-NOT-MERGE performance Performance or resource usage stale Stale PR or inactive for long period of time.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@serhiy-storchaka @mention-bot @csabella @vstinner @brettcannon @iritkatriel @Mariatta @hauntsaninja @the-knights-who-say-ni @ezio-melotti