Opened 9 years ago

Closed 8 years ago

#13159 closed enhancement (fixed)

Cache the number of arguments of a cached method

Reported by: SimonKing Owned by: tbd
Priority: major Milestone: sage-5.5
Component: performance Keywords: argspec cached method
Cc: Merged in: sage-5.5.beta1
Authors: Simon King Reviewers: David Roe
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description

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

Attachments (1)

trac13159_cache_nargs_of_cached_methods.patch (2.6 KB) - added by SimonKing 9 years ago.

Download all attachments as: .zip

Change History (6)

Changed 9 years ago by SimonKing

comment:1 Changed 9 years ago by SimonKing

  • Status changed from new to needs_review

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...

comment:2 Changed 8 years ago by roed

  • Status changed from needs_review to positive_review

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. :-)

comment:3 Changed 8 years ago by roed

  • Reviewers set to David Roe

comment:4 Changed 8 years ago by jdemeyer

  • Milestone changed from sage-5.4 to sage-5.5

comment:5 Changed 8 years ago by jdemeyer

  • Merged in set to sage-5.5.beta1
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.