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-119109: functool.partial vectorcall supports pto->kw & fallback to tp_call removed #120783

Closed
wants to merge 2 commits into from

Conversation

dg-pb
Copy link
Contributor

@dg-pb dg-pb commented Jun 20, 2024

Although comment stated that "merging keyword arguments for vector call is messy" (not exact quote), but I have found that it is fairly straight forward.

This achieves 2 goals:

  1. Solves the raised issue of partial being stuck in tp_call after keywords are deleted even if function supports vectorcall.
  2. Improves performance when len(pto-kw) != 0 and function supports vectorcall.

Number 1. is solved better than it was in #119125 as there are no more switching between methods and faster vectorcall is used whenever appropriate. Also, this does not require additional variable in class struct.

Comparison of performance:

def sub(a, b):
    return a - b

p = partial(sub, b=2)
# Before
%timeit p(1)    # 240 ns
# After
%timeit p(1)    # 180 ns
# Using simpler but slower approach with `_PyObject_VectorcallDictTstate`
%timeit p(1)    # 200 ns

This is only initial attempt and I think the code can be made simpler. (much simpler if decide to use _PyObject_VectorcallDictTstate).

This will need to be adjusted to #119827 so do not merge.

@rhettinger rhettinger requested review from vstinner and removed request for rhettinger June 20, 2024 23:39
@vstinner
Copy link
Member

Is the PR a draft or is it ready for review?

@dg-pb
Copy link
Contributor Author

dg-pb commented Jun 24, 2024

Is the PR a draft or is it ready for review?

This one is a draft. Just an example of concept that works for current functools.partial.

Reason being that #119827 will need a different implementation, which I have, but still waiting for reply how to go about it (#119827 (comment)):
a) Make it part of #119827
b) Wait after it is merged and issue a new PR

Here is implementation that works for partial with Placeholders:
https://gist.github.com/dg-pb/45f6d3af262056aaea1acd4ce29ee4dd

It can be reviewed - sooner or later it will become PR. It is contained within one function - no documentation updates or anything additional is needed.

Let me know if there is a good way to issue a separate PR on top of the one which is not yet merged - I am not aware of such.

@dg-pb
Copy link
Contributor Author

dg-pb commented Sep 26, 2024

Will issue a new PR for partial with placeholders

@dg-pb dg-pb closed this Sep 26, 2024
@dg-pb dg-pb deleted the remove_partial_fallback branch September 26, 2024 05:07
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.

3 participants