#21012 closed Cleanup/optimization (fixed)
Provide shared "caches" dict to avoid creating multiple cache class instances.
Reported by: | FunkyBob | Owned by: | Aymeric Augustin |
---|---|---|---|
Component: | Core (Cache system) | Version: | dev |
Severity: | Normal | Keywords: | |
Cc: | Florian Apolloner, FunkyBob | Triage Stage: | Accepted |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | yes |
Easy pickings: | no | UI/UX: | no |
Description
Pursuant to Florian's recent post on the mailing list, here is a patch to provide django.core.cache.caches, a defaultdict that will provide cache instances from CACHES.
Attachments (1)
Change History (22)
comment:1 by , 11 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:2 by , 11 years ago
Triage Stage: | Unreviewed → Ready for checkin |
---|
comment:3 by , 11 years ago
Patch needs improvement: | set |
---|---|
Triage Stage: | Ready for checkin → Accepted |
I don't think this is the right way to go. First we need to figure out how cache classes are supposed to behave, eg does it even make sense to have one global instance of them or should be put them into threadlocals etc… Eg, what happens if two requests at the same time request a value, will python-memcached acquire the GIL during it's operation or can actually both requests send a request over the wire etc…
comment:4 by , 11 years ago
I'm sorry, I haven't followed the entire discussion. Could someone explain to me why the cache connections aren't thread-local like the database connections? Otherwise this looks like a good idea.
follow-up: 6 comment:5 by , 11 years ago
We should consider the CACHES setting as the canonical way to create cache objects, instead of having yet another way (get_cache
) that is naturally hard to cache due to its ability to accept arbitrary cache backend parameters. In other words, I think we should deprecate get_cache
and add a new mapping name -> instance available as a thread local (e.g. like the here already proposed caches
variable).
from django.core.cache import caches, default_cache assert caches['default'] == default_cache assert caches['staticfiles'] != default_cache
Alternatively we could enable django.core.cache.cache
to switch between the backends:
from django.core.cache import cache, default_cache assert cache('default').get('test') == default_cache.get('test') assert cache('staticfiles').get('test') != default_cache.get('test') with cache('default') as context_default_cache: assert context_default_cache == default_cache
comment:6 by , 11 years ago
Cc: | added |
---|
Replying to jezdez:
from django.core.cache import caches, default_cache assert caches['default'] == default_cache assert caches['staticfiles'] != default_cache
So basically the same we do for django.db.connection/s -- +1 to streamline those two implementations
with cache('default') as context_default_cache: assert context_default_cache == default_cache
While this does look neat, I am not sure about the gain, eg it wouldn't effect called subfunctions etc (unless that contextmanager changes it in the thread local -- which is probably not what you want).
comment:7 by , 11 years ago
I have a work in progress providing a CacheHandler, similar to the db ConnectionManager.
https://github.com/funkybob/django/compare/ticket-21012
Just need to write docs and tests now...
comment:8 by , 11 years ago
I realise it will need a test, but for now could someone read over:
https://github.com/funkybob/django/compare/ticket-21012
Would a test which fired up two threads and:
- had them ask for the same cache and assert identical
- ask for different caches and assert them different
be sufficient?
comment:9 by , 11 years ago
The patch is now ready and passing all tests.
Please let me know if you'd like me to expand the docs more.
comment:10 by , 11 years ago
Owner: | changed from | to
---|
I left a small comment on the pull request.
If no one merges the patch by then, I'll try to review it at the sprints in Berlin in two weeks.
comment:11 by , 11 years ago
Cc: | added |
---|
comment:12 by , 11 years ago
New PR based on FunkyBob's work: https://github.com/django/django/pull/1972
follow-up: 14 comment:13 by , 11 years ago
There's a risk that this patch with make LocMemCache
thread-local instead of process-local. We should check that, and possibly document it, before merging the patch.
comment:14 by , 11 years ago
LocMemCache
uses global state, so it would still be process-local; but we should ensure that __init__
is actually threadsafe.
comment:15 by , 11 years ago
I just had a quick chat with Ned Batchelder on #python and he suggested to:
- Use just one dictionary instead of the three, which seems sensible to me. (eg
self._cache, self._expire_info, self._lock = _cache_info.setdefault(name, ({}, {}, {}))
) - Use read/lock/read in
__init__
to make clear what the code is doing and not rely on changing (python) implementations
comment:16 by , 11 years ago
We should probably implement something in django.test.signals to handle situations where the CACHES settings is changed.
comment:17 by , 11 years ago
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
by , 11 years ago
Attachment: | locmem.diff added |
---|
Looks good to me, tested on both 2.7 and 3.3.