HiveBrain v1.2.0
Get Started
← Back to all entries
patternpythondjangoMinor

LRU caching decorator that caches in the instance and in a shared cache

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
thesharedcachinginstancecachethatcachesanddecoratorlru

Problem

I needed a caching decorator, that could be used with methods and functions, that could take typed args and kwargs, and that was Python 2.7 compatible. Therefore I started with a backport of the lru_cache from Python 3.3. However, I also needed the ability to incorporate a shared cache (I am doing this currently via the Django cache framework) so that items that were not locally available in cache could still avoid more expensive and complex queries by hitting a shared cache.

I don't have a ton of experience writing performance optimized code so am interested in any feedback on ways to improve this.

```
from __future__ import unicode_literals
from django.core.cache import get_cache
from collections import namedtuple
from functools import update_wrapper
from threading import RLock
import logging
try:
from spooky import hash128 as hash
except:
from hashlib import sha256
hash = lambda x: sha256(x).hexdigest()
import inspect

logger = logging.getLogger('frontpage')
_CacheInfo = namedtuple("CacheInfo", ["l1_hits", "l1_misses", "l2_hits", "l2_misses", "l1_maxsize", "l1_currsize"])

def _make_key(user_function, args, kwds, typed,
kwd_mark = (object(),),
fasttypes = {int, str, frozenset, type(None)},
sorted=sorted, tuple=tuple, type=type, len=len, inst_attr='id'):
'Make a cache key from optionally typed positional and keyword arguments'
args = list(args)
if len(args) > 0:
if inspect.ismethod(getattr(args[0], user_function.__name__, None)):
instance = args.pop(0)
try:
key = ["{c}{i}".format(c=instance.__class__, i=getattr(instance, inst_attr)), user_function.__name__]
except:
key = ["{c}{i}".format(c=instance.__class__, i=instance.__hash__()), user_function.__name__]
else:
key = ["", user_function.__name__]
else:
key = ["", user_function.__name__]
if args:
key.append(tuple(args))
if kw

Solution

I'll be completely honest - I don't understand what _make_key is doing, how, or why. I could probably figure it out with some digging, but it seems that it could be better documented and commented.

One huge issue is that you have a bare except: in it - this is literally never a good idea. It looks like you want to catch an AttributeError - just do that. Even better, you could use the optional default value argument.

key = ["{c}{i}".format(c=instance.__class__, i=getattr(instance, inst_attr, hash(instance))), user_function.__name__]


You would also benefit from making things shorter - I like the PEP8 80 character per line limit (this is the formatting style I like, but use whatever you prefer).

key = ["{c}{i}".format(
           c=instance.__class__,
           i=getattr(instance, inst_attr, hash(instance))
       ), user_function.__name__]


You have a bit of unnecessary repetition in assigning the other value to key. I think I would rather do something like

if len(args) > 0 and inspect.ismethod(getattr(args[0], user_function.__name__, None)):
    key = ....
else:
    key = ["", user_function.__name__]


You could use comprehensions in here to make things a bit cleaner

if kwds:
    tuple_ = (kwd_mark,) + tuple(item for item in sorted(kwds.items()))
    key.append(tuple_)


You have a potential bug in the if typed section - in the case where typed is truthy but kwds is falsy (which may never happen) you're going to get a NameError - resolve this by creating sorted_items above the if statements, then use that within the sections below.

You should format your docstrings to match with some specific style guide - that'll make it easier for something like Sphinx to autogenerate documentation from the docstrings, and it's easier to read for people familiar with those styles as well. I like the numpydoc style guide.

Instead of setting to numbers, you probably want an Enum (enum34 is a pretty good backport) for L1_HITS, etc.

Your comment for the l1_maxsize is None seems misleading - isn't this size unlimited caching? Same for the last case.

Overall a lot of the code in the cache itself seems like it could be simplified a bit, and that it could (and should?) be broken up into more helper functions. In particular, the usage of a linked-list makes me a bit nervous - in my experience they are almost never the right data structure for the job. Additionally, more comments explaining some of the design decisions might be helpful - as is they aren't super intuitive.

Code Snippets

key = ["{c}{i}".format(c=instance.__class__, i=getattr(instance, inst_attr, hash(instance))), user_function.__name__]
key = ["{c}{i}".format(
           c=instance.__class__,
           i=getattr(instance, inst_attr, hash(instance))
       ), user_function.__name__]
if len(args) > 0 and inspect.ismethod(getattr(args[0], user_function.__name__, None)):
    key = ....
else:
    key = ["", user_function.__name__]
if kwds:
    tuple_ = (kwd_mark,) + tuple(item for item in sorted(kwds.items()))
    key.append(tuple_)

Context

StackExchange Code Review Q#118699, answer score: 2

Revisions (0)

No revisions yet.