[Django] #21012: Provide shared "caches" dict to avoid creating multiple cache class instances.
Django
instances.
--------------------------------------+--------------------
Reporter: FunkyBob | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Core (Cache system) | Version: master
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 1
Easy pickings: 0 | UI/UX: 0
--------------------------------------+--------------------
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.
https://github.com/django/django/pull/1539
--
Ticket URL: <https://code.djangoproject.com/ticket/21012>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
instances.
Reporter: FunkyBob | Owner: FunkyBob
Type: | Status: assigned
Cleanup/optimization | Version: master
Component: Core (Cache system) | Resolution:
Severity: Normal | Triage Stage:
Keywords: | Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by FunkyBob):
* owner: nobody => FunkyBob
* needs_better_patch: => 0
* status: new => assigned
* needs_tests: => 0
* needs_docs: => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/21012#comment:1>
Django
instances.
Reporter: FunkyBob | Owner: FunkyBob
Type: | Status: assigned
Cleanup/optimization | Version: master
Component: Core (Cache system) | Resolution:
Keywords: | checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Changes (by loic84):
* stage: Unreviewed => Ready for checkin
Comment:
Looks good to me, tested on both 2.7 and 3.3.
--
Ticket URL: <https://code.djangoproject.com/ticket/21012#comment:2>
Django
instances.
Reporter: FunkyBob | Owner: FunkyBob
Type: Cleanup/optimization | Status: assigned
Component: Core (Cache system) | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by apollo13):
* needs_better_patch: 0 => 1
* stage: Ready for checkin => Accepted
Comment:
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…
--
Ticket URL: <https://code.djangoproject.com/ticket/21012#comment:3>
Django
instances.
Reporter: FunkyBob | Owner: FunkyBob
Type: Cleanup/optimization | Status: assigned
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by aaugustin):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/21012#comment:4>
Django
instances.
Reporter: FunkyBob | Owner: FunkyBob
Type: Cleanup/optimization | Status: assigned
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by jezdez):
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
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/21012#comment:5>
Django
instances.
Reporter: FunkyBob | Owner: FunkyBob
Type: Cleanup/optimization | Status: assigned
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by apollo13):
* cc: apollo13 (added)
Comment:
Replying to [comment:5 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).
--
Ticket URL: <https://code.djangoproject.com/ticket/21012#comment:6>
Django
instances.
Reporter: FunkyBob | Owner: FunkyBob
Type: Cleanup/optimization | Status: assigned
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by FunkyBob):
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...
--
Ticket URL: <https://code.djangoproject.com/ticket/21012#comment:7>
Django
instances.
Reporter: FunkyBob | Owner: FunkyBob
Type: Cleanup/optimization | Status: assigned
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by FunkyBob):
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:
1. had them ask for the same cache and assert identical
2. ask for different caches and assert them different
be sufficient?
--
Ticket URL: <https://code.djangoproject.com/ticket/21012#comment:8>
Django
instances.
Reporter: FunkyBob | Owner: FunkyBob
Type: Cleanup/optimization | Status: assigned
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by FunkyBob):
The patch is now ready and passing all tests.
Please let me know if you'd like me to expand the docs more.
--
Ticket URL: <https://code.djangoproject.com/ticket/21012#comment:9>
Django
instances.
Reporter: FunkyBob | Owner: aaugustin
Type: | Status: assigned
Cleanup/optimization | Version: master
Component: Core (Cache system) | Resolution:
Keywords: | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 1
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Changes (by aaugustin):
* owner: FunkyBob => aaugustin
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/21012#comment:10>
Django
instances.
Reporter: FunkyBob | Owner: aaugustin
Type: | Status: assigned
Cleanup/optimization | Version: master
Component: Core (Cache system) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 1
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* cc: FunkyBob (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/21012#comment:11>
Django
instances.
Reporter: FunkyBob | Owner: aaugustin
Type: | Status: assigned
Cleanup/optimization | Version: master
Component: Core (Cache system) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 1
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by aaugustin):
New PR based on FunkyBob's work:
https://github.com/django/django/pull/1972
--
Ticket URL: <https://code.djangoproject.com/ticket/21012#comment:12>
Django
instances.
Reporter: FunkyBob | Owner: aaugustin
Type: | Status: assigned
Cleanup/optimization | Version: master
Component: Core (Cache system) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 1
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by aaugustin):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/21012#comment:13>
Django
instances.
Reporter: FunkyBob | Owner: aaugustin
Type: | Status: assigned
Cleanup/optimization | Version: master
Component: Core (Cache system) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 1
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by apollo13):
`LocMemCache` uses global state, so it would still be process-local; but
we should ensure that `__init__` is actually threadsafe.
--
Ticket URL: <https://code.djangoproject.com/ticket/21012#comment:14>
Django
instances.
Reporter: FunkyBob | Owner: aaugustin
Type: | Status: assigned
Cleanup/optimization | Version: master
Component: Core (Cache system) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 1
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by apollo13):
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
--
Ticket URL: <https://code.djangoproject.com/ticket/21012#comment:15>
Django
instances.
Reporter: FunkyBob | Owner: aaugustin
Type: | Status: assigned
Cleanup/optimization | Version: master
Component: Core (Cache system) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 1
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by aaugustin):
We should probably implement something in django.test.signals to handle
situations where the CACHES settings is changed.
--
Ticket URL: <https://code.djangoproject.com/ticket/21012#comment:16>
Django
instances.
Reporter: FunkyBob | Owner: aaugustin
Cleanup/optimization | Version: master
Component: Core (Cache system) | Resolution: fixed
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 1
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"ffc37e2343a93cf6d44247e20cd263b41f931716"]:
{{{
#!CommitTicketReference repository=""
revision="ffc37e2343a93cf6d44247e20cd263b41f931716"
Fixed #21012 -- New API to access cache backends.
Thanks Curtis Malony and Florian Apolloner.
Squashed commit of the following:
commit 3380495e93f5e81b80a251b03ddb0a80b17685f5
Author: Aymeric Augustin <aymeric....@m4x.org>
Date: Sat Nov 23 14:18:07 2013 +0100
Looked up the template_fragments cache at runtime.
commit 905a74f52b24a198f802520ff06290a94dedc687
Author: Aymeric Augustin <aymeric....@m4x.org>
Date: Sat Nov 23 14:19:48 2013 +0100
Removed all uses of create_cache.
Refactored the cache tests significantly.
Made it safe to override the CACHES setting.
commit 35e289fe9285feffed3c60657af9279a6a2cfccc
Author: Aymeric Augustin <aymeric....@m4x.org>
Date: Sat Nov 23 12:23:57 2013 +0100
Removed create_cache function.
commit 8e274f747a1f1c0c0e6c37873e29067f7fa022e8
Author: Aymeric Augustin <aymeric....@m4x.org>
Date: Sat Nov 23 12:04:52 2013 +0100
Updated docs to describe a simplified cache backend API.
commit ee7eb0f73e6d4699edcf5d357dce715224525cf6
Author: Curtis Maloney <cur...@tinbrain.net>
Date: Sat Oct 19 09:49:24 2013 +1100
Fixed #21012 -- Thread-local caches, like databases.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/21012#comment:17>
Django
instances.
Reporter: FunkyBob | Owner: aaugustin
Type: | Status: closed
Cleanup/optimization | Version: master
Component: Core (Cache system) | Resolution: fixed
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 1
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by apollo13):
Attached the changes Ned suggested; waiting for feedback from Alex.
--
Ticket URL: <https://code.djangoproject.com/ticket/21012#comment:18>
Django
instances.
Reporter: FunkyBob | Owner: aaugustin
Type: | Status: closed
Cleanup/optimization | Version: master
Component: Core (Cache system) | Resolution: fixed
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 1
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by Florian Apolloner <florian@…>):
In [changeset:"d47f794f8fa05591632b8cad4b134858e7ae140d"]:
{{{
#!CommitTicketReference repository=""
revision="d47f794f8fa05591632b8cad4b134858e7ae140d"
Properly closed cache connections at the end of the request.
This only affects the new cache api and not the deprecated get_cache.
Refs #21012
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/21012#comment:19>
Django
instances.
Reporter: FunkyBob | Owner: aaugustin
Type: | Status: closed
Cleanup/optimization |
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"d038c547b5ce585cbf9ef5bb7e5298f52e4a243b"]:
{{{
#!CommitTicketReference repository=""
revision="d038c547b5ce585cbf9ef5bb7e5298f52e4a243b"
Removed django.core.cache.get_cache() per deprecation timeline; refs
#21012.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/21012#comment:20>
Django
instances.
Reporter: FunkyBob | Owner: Aymeric
Type: | Augustin
Cleanup/optimization | Status: closed
Component: Core (Cache system) | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"148702e7258eafb27d5488b33a723c87d898e22b" 148702e7]:
{{{
#!CommitTicketReference repository=""
revision="148702e7258eafb27d5488b33a723c87d898e22b"
Refs #21012 -- Removed unnecessary _create_cache() hook.
This removes unused (since d038c547b5ce585cbf9ef5bb7e5298f52e4a243b)
workaround to load a cache backend with its dotted import path and
moves remaining logic to the CacheHandler.
Thanks Tim Graham for the review.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/21012#comment:21>