| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
There was a problem hiding this comment.
lgtm with nit comment
It's become a lot faster
import pyperf
runner = pyperf.Runner()
runner.timeit(name="bench singledispatchmethod",
stmt="""
_ = t.go(1, 1)
""",
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
t = Test()
""")
Mean +- std dev: [base] 1.37 us +- 0.01 us -> [opt] 410 ns +- 2 ns: 3.35x faster
Sorry, something went wrong.
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
|
I did some more playing around locally and found we could improve performance even more with a few tweaks. Proposed a PR to your branch at eendebakpt#5, which also adds some more test coverage. |
Sorry, something went wrong.
|
@AlexWaygood I merged your PR and added one more optimization: we can skip the obj is not None check since for obj=None the weakref indexing will fail with a TypeError. One more optimization is in https://github.com/eendebakpt/cpython/pull/6/files. It eliminates the caching variable. The assumption there is that if self._method_cache[obj] returns a KeyError, the expression self._method_cache[obj] = ... is safe. I still have to do benchmarking on this. |
Sorry, something went wrong.
There was a problem hiding this comment.
LGTM. @ethanhs, how does this look to you now?
Sorry, something went wrong.
|
Latest benchmarks: bench singledispatchmethod: Mean +- std dev: [main] 2.48 us +- 0.02 us -> [pr] 1.04 us +- 0.02 us: 2.38x faster bench classmethod: Mean +- std dev: [main] 3.45 us +- 0.06 us -> [pr] 3.50 us +- 0.07 us: 1.01x slower bench classmethod on instance: Mean +- std dev: [main] 3.49 us +- 0.06 us -> [pr] 1.12 us +- 0.01 us: 3.11x faster bench slotted class: Mean +- std dev: [main] 2.48 us +- 0.03 us -> [pr] 2.53 us +- 0.03 us: 1.02x slower Geometric mean: 1.64x faster So the performance improves for the common case (normal methods) and is more or less equal for class methods and slotted classes. One more variation is to eliminate attribute lookups in the dispatch wrapper: eendebakpt/cpython@singledispatchmethod3...eendebakpt:cpython:singledispatchmethod3b bench singledispatchmethod: Mean +- std dev: [pr] 1.04 us +- 0.02 us -> [pr_lookup] 979 ns +- 15 ns: 1.06x faster bench classmethod on instance: Mean +- std dev: [pr] 1.12 us +- 0.01 us -> [pr_lookup] 1.06 us +- 0.01 us: 1.06x faster bench slotted class: Mean +- std dev: [pr] 2.53 us +- 0.03 us -> [pr_lookup] 2.52 us +- 0.03 us: 1.01x faster Benchmark hidden because not significant (1): bench classmethod Geometric mean: 1.03x faster |
Sorry, something went wrong.
I can reproduce the speedup locally (great spot!), and it seems like a reasonable thing to do. Please name the variable dispatch rather than dp, though :) One other optimisation that shaves around 5 microseconds off the benchmark for slotted classes and other objects that can't be weakref'd (but doesn't do much for those that can): --- a/Lib/functools.py
+++ b/Lib/functools.py
@@ -959,7 +959,7 @@ def _method(*args, **kwargs):
method = self.dispatcher.dispatch(args[0].__class__)
return method.__get__(obj, cls)(*args, **kwargs)
- _method.__isabstractmethod__ = self.__isabstractmethod__
+ _method.__isabstractmethod__ = getattr(self.func, "__isabstractmethod__", False)But if you want to do ^that, you should probably add a comment about why we're duplicating the logic in the __isabstractmethod__ property. |
Sorry, something went wrong.
Hmm, I thought I could see a speedup from this earlier, but now I can no longer reproduce it. I assume it was only noise -- best to leave it. |
Sorry, something went wrong.
|
@mental32 and @eendebakpt, thanks so much for all your work on this! |
Sorry, something went wrong.
| try: | ||
| _method = self._method_cache[obj] | ||
| except TypeError: | ||
| self._all_weakrefable_instances = False |
There was a problem hiding this comment.
Hmm, I wonder if this would have been a good idea, to keep memory usage down for the slow path (since in the slow path, we don't really need the WeakKeyDictionary at all)
| self._all_weakrefable_instances = False | |
| self._all_weakrefable_instances = False | |
| del self._method_cache |
@corona10, what do you think? Does it matter?
Sorry, something went wrong.
There was a problem hiding this comment.
One could even delay the creation of the Weakkeydictionary untill the first invocation of __get__. The code will look a bit odd though, a simple del looks cleaner.
Sorry, something went wrong.
There was a problem hiding this comment.
Yeah, I think I prefer the approach with del
Sorry, something went wrong.
A small followup to #107148 Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
| Back | FazBrowse Home | New Git URL |
This PR implements the idea from #85160 to improve performance of the singledispatchmethod by caching the generated dispatch method. It is a continuation of #23213
Also see #106448