Skip to content

bpo-37337: Add _PyObject_VectorcallMethod() #14228

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

Merged
merged 1 commit into from
Jun 28, 2019

Conversation

jdemeyer
Copy link
Contributor

@jdemeyer jdemeyer commented Jun 19, 2019

@jdemeyer
Copy link
Contributor Author

CC @vstinner since this is increasing the size of the C API.

@jdemeyer jdemeyer force-pushed the vectorcall_method branch from 4bd378a to 1c9011d Compare June 19, 2019 12:06
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.

This PR adds too many functions at once, it's hard to see how each function is used. Would it be possible to start by adding _PyObject_VectorcallMethod() and its _PyObject_VectorcallMethodId() variant.

Only once it's merged, propose a new PR to only add the private _PyObject_CallMethodNoArgs() and its _PyObject_CallMethodIdNoArgs() variant.

Adding a public PyObject_CallMethodNoArgs() should be the last PR. You should elaborate "efficiency" rationale: measure how many times it's called, the performance benefit (if any), effect on the stack usage, etc.

@bedevere-bot
Copy link

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@jdemeyer
Copy link
Contributor Author

Would it be possible to start by adding _PyObject_VectorcallMethod() and its _PyObject_VectorcallMethodId() variant.

Sure. It's just that sometimes you actually need to use the function in more places to see which effect it has. If I would have made a PR adding _PyObject_VectorcallMethodId but never using it, people might complain about adding a new function which isn't used anywhere.

But now that we see that these functions would certainly be used, we can indeed split this up.

I assume that by asking this, you agree to add the functions _PyObject_VectorcallMethod() and _PyObject_VectorcallMethodId().

@vstinner
Copy link
Member

I suggest to only add _PyObject_VectorcallMethod() and its _PyObject_VectorcallMethodId() variant, but also use them. My mean: remove the 3 other functions.

@jdemeyer
Copy link
Contributor Author

So what should I do with all the changes of the form

-    loop = _PyObject_CallMethodId(policy, &PyId_get_event_loop, NULL);
+    loop = _PyObject_CallMethodIdNoArgs(policy, &PyId_get_event_loop);

@jdemeyer jdemeyer force-pushed the vectorcall_method branch from 64576fe to 2be40b5 Compare June 19, 2019 15:55
@jdemeyer jdemeyer changed the title bpo-37337: Add_PyObject_VectorcallMethod() and PyObject_CallMethodNoArgs() bpo-37337: Add _PyObject_VectorcallMethod() and PyObject_CallMethodNoArgs() Jun 19, 2019
@jdemeyer jdemeyer changed the title bpo-37337: Add _PyObject_VectorcallMethod() and PyObject_CallMethodNoArgs() bpo-37337: Add _PyObject_VectorcallMethod() Jun 19, 2019
@jdemeyer
Copy link
Contributor Author

I suggest to only add _PyObject_VectorcallMethod() and its _PyObject_VectorcallMethodId() variant, but also use them.

Done.

@brettcannon brettcannon removed their request for review June 19, 2019 20:57
@jdemeyer
Copy link
Contributor Author

You set this to awaiting merge, so you plan to merge it? Or are you waiting for more reviews?

@methane
Copy link
Member

methane commented Jun 24, 2019

Bot changed the label, not l.
It looks good to me. But I'm waiting for other core-dev's review.

@vstinner Would you check this again?

@jdemeyer
Copy link
Contributor Author

I have made the requested changes; please review again

@bedevere-bot
Copy link

Thanks for making the requested changes!

@vstinner, @methane: please review the changes made to this pull request.

@methane methane merged commit b1263d5 into python:master Jun 28, 2019
lisroach pushed a commit to lisroach/cpython that referenced this pull request Sep 10, 2019
DinoV pushed a commit to DinoV/cpython that referenced this pull request Jan 14, 2020
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