#21351 -- replaced memoize by lru_cache #1842

wants to merge 1 commit into


None yet

3 participants


Python 3.2 comes with lru_cache, which does basically the same as memoize. Currently memoize isn't tested or verified, which is a liability. As was suggested in ticket 21351, lru_cache could replace memoize. However as lru_cache is only available from Python 3.2 and up, a backport is required for older versions. The ticket also links to a backport (MIT licensed) of lru_cache. I've included this backport for older Python versions.

The following questions arised: (discussion in ticket)

  • Can the backport be included, as it is MIT licensed?
  • memoize is an internal API, should it follow the deprecation policy?
  • Is this the correct way to include a backport?
@aaugustin aaugustin commented on the diff Nov 4, 2013
@@ -0,0 +1,168 @@
aaugustin Nov 4, 2013 Django member

Can you add a comment at the top of the file stating clearly where the file comes from (stdlib of Python x.y.z) and when it should be removed?

Bouke Nov 4, 2013

It comes from http://code.activestate.com/recipes/578078-py26-and-py30-backport-of-python-33s-lru-cache/ written by Raymond Hettinger and MIT licensed, the backport is derived from lru_cache in the stdlib.

@bmispelon bmispelon commented on the diff Nov 4, 2013
@@ -116,15 +113,15 @@ def get_callable(lookup_view, can_fail=False):
"Could not import %s. View does not exist in module %s." %
(lookup_view, mod_name))
return lookup_view
-get_callable = memoize(get_callable, _callable_cache, 1)
bmispelon Nov 4, 2013 Django member

You've removed the num_args=1 argument (which lru_cache doesn't support) here which is probably a bug.

The fact that the test suite doesn't break because of this is intriguing and should be investigated further.

Bouke Nov 4, 2013

I don't believe num_args=1 has any added value, apart from memoize requiring it for argument slicing. lru_cache assumes all provided arguments matter. Looking at how memoize is used, num_args matches everywhere with the possible number of arguments. Are am I missing something?

bmispelon Nov 4, 2013 Django member

The num_args argument was introduced in b6812f2 specifically for get_callable (note how it can take two arguments but the num_args is 1).

I suspect there might be something weird going on but I haven't had time to look into it and unfortunately, we cannot ask the original committer of that change anymore :(

Bouke Nov 5, 2013

I've tried debugging the num_args and the impact on get_callable, but I only met confusion.

Say a function has two parameters and one result, then A,B -> X and A,C -> Y. However as the function's cache strategy only accounts for the first argument, it will always do the same, namely A,? -> X UNLESS in the case of an exception. Now when the exception happened for A,D -> ?, then no cache is written and A,C -> Y. However if A,C -> Y is executed FIRST and cached, then A,? -> Y and thus A,D -> Y -- no exception being raised.

See also my added test which shows the problem for num_args=1.

@bmispelon bmispelon commented on the diff Nov 6, 2013
from django.contrib.staticfiles import utils
from django.contrib.staticfiles.storage import AppStaticStorage
-_finders = OrderedDict()
bmispelon Nov 6, 2013 Django member

lru_cache uses a plain dict internally so at first glance, this appears to break compatibility.

However, I don't understand how using an OrderedDict for the cache would change anything (from what I understand, the cache is never iterated over). Do you have some more insight on this?

Django member


The pull request doesn't apply cleanly on my local checkout. Could you rebase it to the master branch (and also squash all the commits into one)?



I've updated and squashed the patch; tests OK on both Python 2 and 3.

Django member

Thanks for the update. I can confirm that tests pass.

However, I don't see a mention in the 1.7 release notes anymore.
I could swear I had seen that in this patch before. Did it get lost in the squashing or did I dream it?

Once that gets in, that PR is ready for checkin as far as I'm concerned.

Nice work!

@Bouke Bouke Fixed #21351 -- Replaced memoize with Python's lru_cache
Replaced the custom, untested memoize with a similar decorator from Python's
3.2 stdlib. Although some minor performance degradation (see ticket), it is
expected that in the long run lru_cache will outperform memoize once it is
implemented in C.

Thanks to EvilDMP for the report and Baptiste Mispelon for the idea of
replacing memoize with lru_cache.

There was only the mentioning of removing memoize in deprecations.txt. I've added a note to the release notes.

Django member

Looks good to me, although shouldn't python be written as Python (twice)?

Django member

Merged in 9b7455e.

Thanks for your contribution!

@bmispelon bmispelon closed this Nov 11, 2013
@Bouke Bouke deleted the Bouke:tickets/21351 branch Nov 11, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment