Add a set of memoization decorators for functions.

Review Request #2308 - Created June 2, 2015 and submitted

Information
John Sirois
pants
jsirois/memoization/decorators
1624
fe8bdae...
Reviewers
pants-reviews
ity, patricklaw, zundel
These can be used to encapsulate the common pattern of memoizing
calculated properties as well as more complex cases.  Several properties
with boilerplate memoization are fixed up to use this new system.

 src/python/pants/backend/codegen/tasks/BUILD                      |   1 +
 src/python/pants/backend/codegen/tasks/apache_thrift_gen.py       |  22 ++----
 src/python/pants/backend/jvm/targets/BUILD                        |   1 +
 src/python/pants/backend/jvm/targets/import_jars_mixin.py         |  51 ++++++-------
 src/python/pants/backend/jvm/targets/unpacked_jars.py             |   4 +-
 src/python/pants/util/BUILD                                       |   5 ++
 src/python/pants/util/memo.py                                     | 156 ++++++++++++++++++++++++++++++++++++++++
 tests/python/pants_test/backend/jvm/targets/test_unpacked_jars.py |   2 -
 tests/python/pants_test/util/BUILD                                |   9 +++
 tests/python/pants_test/util/test_memo.py                         | 193 ++++++++++++++++++++++++++++++++++++++++++++++++++
 10 files changed, 396 insertions(+), 48 deletions(-)
CI went green here:
  https://travis-ci.org/pantsbuild/pants/builds/65126296

Issues

  • 0
  • 3
  • 0
  • 3
Description From Last Updated
Benjy Weinberger
John Sirois
John Sirois
Benjy Weinberger
Eric Ayers
John Sirois
John Sirois
John Sirois
John Sirois
Eric Ayers
Eric Ayers
John Sirois
John Sirois
John Sirois
Review request changed

Status: Closed (submitted)

Patrick Lawson

I'm glad you went ahead with this, but I think that it's overkill to try to provide arbitrary (parameterized) memoization in a decorator. In particular there are at least two issues noted below where the memoization can fail silently and disasterously. I would much prefer to see the more tractable but very common problem of a cached property solved like this just using getattr and setattr, and general memoization left to authors on a case-by-case basis. General memoization is rare I think, and should be considered somewhat carefully by the author doing it. In particular, I think the cost/benefit of a correct generalized decorator is definitely not worth it.

  1. Since the cost is payed already, I'll circle back with fixes for the 2 items you point out.  If you feel strongly this should die instead, I'll leave it to you to replace with the Foursquare impl you talked about upstreaming when you find time.
  2. I'll give this some thought. I'm not convinced that this is worthwhile or, depending on your constraints, even possible in a generalized fashion.

Alpha

src/python/pants/util/memo.py (Diff revision 5)
 
 

This theoretically has a collision when there are args which are tuples.

  1. Good catch.
  2. Addressed here with failing test before fix : https://rbcommons.com/s/twitter/r/2317/
src/python/pants/util/memo.py (Diff revision 5)
 
 

I'm pretty sure that id can be reused after an object is GCed, meaning this is not a suitably stable memo key.

  1. It can indeed per the docs.  I'll use a tuple of `(id(obj), obj)` for the the 1st slot in this case to retain a strong reference to obj.
  2. Addressed here - but no test for this one: https://rbcommons.com/s/twitter/r/2317/
Loading...