GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-85160: Reduce memory usage of `singledispatchmethod` when owner instances cannot be weakref'd by AlexWaygood · Pull Request #107706 · python/cpython

Conversation

@AlexWaygood

@AlexWaygood AlexWaygood commented Aug 7, 2023 •
edited by bedevere-bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

No news, since this is just a small followup to #107148, which is a new feature in Python 3.13

See #107148 (comment) (cc. @eendebakpt)

  • Issue: singledispatchmethod significantly slower than singledispatch #85160

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@AlexWaygood
pythongh-85160: Reduce memory usage of singledispatchmethod when in…
bfce05c
…stances cannot be weakref'd
@AlexWaygood AlexWaygood added the skip news label Aug 7, 2023
@AlexWaygood
AlexWaygood requested a review from corona10 August 7, 2023 10:35
@AlexWaygood
AlexWaygood requested a review from rhettinger as a code owner August 7, 2023 10:35
@bedevere-bot bedevere-bot mentioned this pull request Aug 7, 2023
singledispatchmethod significantly slower than singledispatch #85160
Closed
@bedevere-bot bedevere-bot added the awaiting core review label Aug 7, 2023
@AlexWaygood AlexWaygood changed the title gh-85160: Reduce memory usage of singledispatchmethod when instances cannot be weakref'd gh-85160: Reduce memory usage of singledispatchmethod when owner instances cannot be weakref'd Aug 7, 2023
@serhiy-storchaka

serhiy-storchaka commented Aug 7, 2023

Copy link
Copy Markdown
Member

Alternatively, you can get rid of self._all_weakrefable_instances:

diff --git a/Lib/functools.py b/Lib/functools.py
index 2a8a69b3c5..5622eb6467 100644
--- a/Lib/functools.py
+++ b/Lib/functools.py
@@ -935,7 +935,6 @@ def __init__(self, func):
         self.dispatcher = singledispatch(func)
         self.func = func
         self._method_cache = weakref.WeakKeyDictionary()
-        self._all_weakrefable_instances = True
 
     def register(self, cls, method=None):
         """generic_method.register(cls, func) -> func
@@ -945,11 +944,11 @@ def register(self, cls, method=None):
         return self.dispatcher.register(cls, func=method)
 
     def __get__(self, obj, cls=None):
-        if self._all_weakrefable_instances:
+        if self._method_cache:
             try:
                 _method = self._method_cache[obj]
             except TypeError:
-                self._all_weakrefable_instances = False
+                self._method_cache = None
             except KeyError:
                 pass
             else:
@@ -963,7 +962,7 @@ def _method(*args, **kwargs):
         _method.register = self.register
         update_wrapper(_method, self.func)
 
-        if self._all_weakrefable_instances:
+        if self._method_cache is not None:
             self._method_cache[obj] = _method
 
         return _method

It reduces memory even more, and in all cases.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@AlexWaygood

AlexWaygood commented Aug 7, 2023

Copy link
Copy Markdown
Member Author

Alternatively, you can get rid of self._all_weakrefable_instances:

Very nice. And the performance seems to be the same. The first line of __get__ needs to be this, however (rather than just if self._method_cache), or tests start to fail:

    def __get__(self, obj, cls=None):
        if self._method_cache is not None:
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@AlexWaygood @serhiy-storchaka
Get rid of _all_weakrefable_instances
3d8d4ba
Co-authored-by: Serhiy Storchaka <[email protected]>
@AlexWaygood
AlexWaygood force-pushed the singledispatch-memory-usage branch from 951a87e to 3d8d4ba Compare August 7, 2023 12:00
@AlexWaygood
AlexWaygood requested a review from serhiy-storchaka August 7, 2023 12:00
@serhiy-storchaka

serhiy-storchaka commented Aug 7, 2023

Copy link
Copy Markdown
Member

Or you need to catch TypeError when save in the cache:

        if self._method_cache is not None:
            try:
                self._method_cache[obj] = _method
            except TypeError:
                self._method_cache = None
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

serhiy-storchaka
serhiy-storchaka approved these changes Aug 7, 2023
View reviewed changes
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting core review labels Aug 7, 2023
@AlexWaygood

AlexWaygood commented Aug 7, 2023

Copy link
Copy Markdown
Member Author

Or you need to catch TypeError when save in the cache:

try/except is expensive; this would be much slower. With this PR currently:

bench singledispatchmethod: Mean +- std dev: 875 ns +- 32 ns
bench singledispatchmethod slots: Mean +- std dev: 2.01 us +- 0.06 us

With this diff applied to my PR branch:

Diff
--- a/Lib/functools.py
+++ b/Lib/functools.py
@@ -945,7 +945,7 @@ def register(self, cls, method=None):
         return self.dispatcher.register(cls, func=method)

     def __get__(self, obj, cls=None):
-        if self._method_cache is not None:
+        if self._method_cache:
             try:
                 _method = self._method_cache[obj]
             except TypeError:
@@ -963,8 +963,10 @@ def _method(*args, **kwargs):
         _method.register = self.register
         update_wrapper(_method, self.func)

-        if self._method_cache is not None:
+        try:
             self._method_cache[obj] = _method
+        except TypeError:
+            pass

         return _method
bench singledispatchmethod: Mean +- std dev: 1.02 us +- 0.04 us
bench singledispatchmethod slots: Mean +- std dev: 3.32 us +- 0.27 us
Benchmark script
import pyperf

setup="""
from functools import singledispatch, singledispatchmethod

class Test:
    @singledispatchmethod
    def go(self, item, arg):
        pass

    @go.register
    def _(self, item: int, arg):
        return item + arg

class Slot:
    __slots__ = ('a', 'b')
    @singledispatchmethod
    def go(self, item, arg):
        pass

    @go.register
    def _(self, item: int, arg):
        return item + arg

t = Test()
s= Slot()
"""

runner = pyperf.Runner()
runner.timeit(name="bench singledispatchmethod", stmt="""_ = t.go(1, 1)""", setup=setup )
runner.timeit(name="bench singledispatchmethod slots", stmt="""_ = s.go(1, 1)""", setup=setup )
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@AlexWaygood
AlexWaygood enabled auto-merge (squash) August 7, 2023 12:12
@serhiy-storchaka

serhiy-storchaka commented Aug 7, 2023

Copy link
Copy Markdown
Member

Simpler if self._method_cache: makes the first call faster (this is why I wrote this so at first place). But if self._method_cache is not None: may make all following calls marginally faster, although I do not know how large the difference. So perhaps the latter variant is better in long run.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@serhiy-storchaka

serhiy-storchaka commented Aug 7, 2023

Copy link
Copy Markdown
Member

try/except is expensive; this would be much slower.

I mean that you keep if self._method_cache is not None: for setting, but add catch TypeError. It will not add an overhead.

AlexWaygood reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@AlexWaygood
AlexWaygood disabled auto-merge August 7, 2023 12:18
@AlexWaygood

AlexWaygood commented Aug 7, 2023

Copy link
Copy Markdown
Member Author

It will not add an overhead.

I still measure about a 14% overhead from this approach compared to my current PR. (Tested on a PGO-optimised non-debug build on Windows, with a quiet machine.) I'm inclined to stick with how it is currently -- it also feels like slightly cleaner code if we have only one try/except in __get__

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@serhiy-storchaka

serhiy-storchaka commented Aug 7, 2023

Copy link
Copy Markdown
Member

I still measure about a 14% overhead from this approach compared to my current PR.

With the test for None preceding the try block? I am surprised, try/except should not add an overhead if no exception was raised.

Anyway, your current code is better. I just explained why I intentionally did not use is None in the initial proposition.

AlexWaygood reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@AlexWaygood

AlexWaygood commented Aug 7, 2023

Copy link
Copy Markdown
Member Author

I still measure about a 14% overhead from this approach compared to my current PR.

With the test for None preceding the try block? I am surprised, try/except should not add an overhead if no exception was raised.

Yep. I'm also surprised there was that difference.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@AlexWaygood
AlexWaygood merged commit 2ac103c into python:main Aug 7, 2023
@bedevere-bot bedevere-bot removed the awaiting merge label Aug 7, 2023
@AlexWaygood
AlexWaygood deleted the singledispatch-memory-usage branch August 7, 2023 12:46
@AlexWaygood

AlexWaygood commented Aug 7, 2023

Copy link
Copy Markdown
Member Author

Thanks for the review, Serhiy!

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@blaggacao blaggacao mentioned this pull request Dec 4, 2024
chore: python3.13 support frappe/frappe#28624
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@serhiy-storchaka serhiy-storchaka serhiy-storchaka approved these changes
@corona10 corona10 Awaiting requested review from corona10
@rhettinger rhettinger Awaiting requested review from rhettinger rhettinger is a code owner

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

3 participants

@AlexWaygood @serhiy-storchaka @bedevere-bot
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.