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

Cache the number of arguments of a cached method #13159

Closed
simon-king-jena opened this issue Jun 25, 2012 · 5 comments
Closed

Cache the number of arguments of a cached method #13159

simon-king-jena opened this issue Jun 25, 2012 · 5 comments

Comments

@simon-king-jena
Copy link
Member

By #11115, if a cached method has only the argument self, then a special optimised implementation is chosen to speed up caching.

Problem: In order to determine the number of arguments, one often needs to inspect the sources of the to-be-wrapped method. And this will be done repeatedly when using a cached method on different instances of the same class.

With my patch, the number of arguments is computed only once, and stored as an argument to the cached method (note: If a cached method is bound to an instance then a CachedMethodCaller is returned - and this is when the number of arguments is relevant).

With patch:

sage: class A:
....:     @cached_method
....:     def a(self):
....:         return 5
....:     @cached_method
....:     def b(self, x=5):
....:         return x**2
....:
sage: cython("""
....: from sage.all import cached_method
....: class B:
....:     @cached_method
....:     def a(self):
....:         return 5
....:     @cached_method
....:     def b(self, x=5):
....:         return x**2
....: """)
sage: %timeit A().a
625 loops, best of 3: 7.28 µs per loop
sage: %timeit A().b
625 loops, best of 3: 6.28 µs per loop
sage: %timeit B().a
625 loops, best of 3: 7.23 µs per loop
sage: %timeit B().b
625 loops, best of 3: 6.37 µs per loop

Without patch

sage: class A:
....:     @cached_method
....:     def a(self):
....:         return 5
....:     @cached_method
....:     def b(self, x=5):
....:         return x**2
....:
sage: cython("""
....: from sage.all import cached_method
....: class B:
....:     @cached_method
....:     def a(self):
....:         return 5
....:     @cached_method
....:     def b(self, x=5):
....:         return x**2
....: """)
sage: %timeit A().a
625 loops, best of 3: 38.2 µs per loop
sage: %timeit A().b
625 loops, best of 3: 38.2 µs per loop
sage: %timeit B().a
625 loops, best of 3: 257 µs per loop
sage: %timeit B().b
625 loops, best of 3: 335 µs per loop

Component: performance

Keywords: argspec cached method

Author: Simon King

Reviewer: David Roe

Merged: sage-5.5.beta1

Issue created by migration from https://trac.sagemath.org/ticket/13159

@simon-king-jena
Copy link
Member Author

@simon-king-jena
Copy link
Member Author

comment:1

I am aware that my patch does not provide doctests. I simply don't know how this speed-up could be doctested. Perhaps the reviewer has an idea, or finds that in this case it is fine to do without a new doctest...

@roed314
Copy link
Contributor

roed314 commented Oct 24, 2012

comment:2

Looks good to me. Though I'm actually getting 300ns access times on the b methods, before and after the patch. The speedup on the a access methods is dramatic though.

I don't think that we need to add a doctest here, since the fact that cached methods work is documented plenty of places. Once #12720 is finished then that will do the speed testing for us. :-)

@roed314
Copy link
Contributor

roed314 commented Oct 24, 2012

Reviewer: David Roe

@jdemeyer jdemeyer modified the milestones: sage-5.4, sage-5.5 Oct 29, 2012
@jdemeyer
Copy link

jdemeyer commented Nov 1, 2012

Merged: sage-5.5.beta1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants