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-124652: partialmethod simplifications #124788

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

dg-pb
Copy link
Contributor

@dg-pb dg-pb commented Sep 30, 2024

3 Parts:

  1. partial: allowing trailing placeholders
    1.1. +some performance benefits for partial.__new__ due to this change
  2. partialmethod simplifications

1. Allowing trailing Placeholders

So, I considered automatic trimming of trailing Placeholders instead of forbidding such. However it didn't sit well with me.

So I thought this over. So not having trailing Placeholders works well, however both approaches of achieving this have flaws:
a) Forbidding them - results in unexpected errors for the user.
b) Trimming automatically - obfuscates inner workings and can be confusing.

Furthermore, it sometimes could be desired to have a trailing Placeholder to convert argument to positional only.

So I think it would be best not to have this restriction at all. This makes things more explicit and predictable. E.g. both would work:

class A:
    def func(self, a):
        pass

    p1 = partial(func)
    p2 = partial(func, Placeholder)

And as expected, self of p1 can be both positional or keyword, while self of p2 becomes restricted to positional only.

Desired behaviour of partialmethod is achieved by appropriate delivery of args. Which stays as it was before - if no positionals provided self argument type is unaffected, while if args are provided then self becomes positional only.

2.1. +partial.__new__ performance benefits

Slightly faster for both cases (with and without Placeholders) when merging args of previous partial with new args.

This also simplifies code by not having to do error checks or trimming routines. While existing functionality handles trailing placeholders as it is (except minor Fast Path fix in C).

S="
from functools import partial, Placeholder
def f(a, b):
    pass
p1 = partial(f, 1, 2)
p2 = partial(f, Placeholder, 2)
"
C0='partial(f)'
C1='partial(f, 1, 2)'
C2='partial(p1, 1, 2)'
C3='partial(p2, 1, 2)'
                            # BEFORE | AFTER
$PYEXE -m timeit -s $S $C0  # 135 ns | 135 ns
$PYEXE -m timeit -s $S $C1  # 170 ns | 170 ns
$PYEXE -m timeit -s $S $C2  # 195 ns | 165 ns
$PYEXE -m timeit -s $S $C3  # 195 ns | 165 ns

2. partialmethod simplifications

  1. it is now untangled from partial (makes use of partial instead of containing any complexities of partial)
  2. it has no need for any special-casing in inspect
  3. its performance is now reasonable (for standard methods and partial objects)

partialmethod Benchmarks:

Setup

S="
from functools import partialmethod, Placeholder

@staticmethod
def s(a, b):
    pass
@classmethod
def c(self, a, b):
    pass
def m(self, a, b):
    pass

class D:
    def __get__(self, obj, cls=None):
        return (lambda a, b: None)
d = D()

class A:
    @staticmethod
    def s(a, b):
        pass
    @classmethod
    def c(self, a, b):
        pass
    def m(self, a, b):
        pass
    d = D()

    ps = partialmethod(s, 1)
    pc = partialmethod(c, 1)
    pm = partialmethod(m, 1)
    pd = partialmethod(d, 1)

a = A()
"

C1='partialmethod(s, 1)'    # staticmethod
C2='partialmethod(c, 1)'    # classmethod
C3='partialmethod(m, 1)'    # instance method
C4='partialmethod(d, 1)'    # unknown descriptor

C5='A.ps(2)'
C6='A.pc(2)'
C7='a.pm(2)'
C8='a.pd(2)'

NOTE: construction cost for standard methods is obfuscated 
      as they are constructed and cached on 1st call.

                            #  BEFORE | AFTER
--------------------------- #-----------------
$PYEXE -m timeit -s $S $C1  #  920 ns |  390 ns
$PYEXE -m timeit -s $S $C2  # 1000 ns |  420 ns
$PYEXE -m timeit -s $S $C3  #  900 ns |  440 ns
$PYEXE -m timeit -s $S $C4  # 1000 ns |  600 ns
--------------------------- #-----------------
$PYEXE -m timeit -s $S $C5  # 1300 ns |  330 ns
$PYEXE -m timeit -s $S $C6  #  830 ns |  390 ns
$PYEXE -m timeit -s $S $C7  #  800 ns |  390 ns
$PYEXE -m timeit -s $S $C8  # 1300 ns |  890 ns

@rhettinger rhettinger removed their request for review September 30, 2024 16:31
Lib/functools.py Outdated Show resolved Hide resolved
@dg-pb
Copy link
Contributor Author

dg-pb commented Oct 6, 2024

Removed keyword Placeholder restriction from this and will issue a separate PR after. Felt like too much is packed into 1 PR.

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.

2 participants