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)
Change History (6)
Changed 9 years ago by
comment:1 Changed 9 years ago by
- Status changed from new to needs_review
comment:2 Changed 8 years ago by
- 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
- Reviewers set to David Roe
comment:4 Changed 8 years ago by
- Milestone changed from sage-5.4 to sage-5.5
comment:5 Changed 8 years ago by
- Merged in set to sage-5.5.beta1
- Resolution set to fixed
- Status changed from positive_review to closed
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...