Opened 11 years ago

Closed 11 years ago

Last modified 4 years ago

#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.

https://github.com/django/django/pull/1539

Attachments (1)

locmem.diff (1.3 KB ) - added by Florian Apolloner 11 years ago.

Download all attachments as: .zip

Change History (22)

comment:1 by FunkyBob, 11 years ago

Owner: changed from nobody to FunkyBob
Status: newassigned

comment:2 by loic84, 11 years ago

Triage Stage: UnreviewedReady for checkin

Looks good to me, tested on both 2.7 and 3.3.

comment:3 by Florian Apolloner, 11 years ago

Patch needs improvement: set
Triage Stage: Ready for checkinAccepted

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 Aymeric Augustin, 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.

comment:5 by Jannis Leidel, 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

Version 0, edited 11 years ago by Jannis Leidel (next)

in reply to:  5 comment:6 by Florian Apolloner, 11 years ago

Cc: Florian Apolloner 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 FunkyBob, 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 FunkyBob, 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:

  1. had them ask for the same cache and assert identical
  2. ask for different caches and assert them different

be sufficient?

comment:9 by FunkyBob, 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 Aymeric Augustin, 11 years ago

Owner: changed from FunkyBob to Aymeric Augustin

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 FunkyBob, 11 years ago

Cc: FunkyBob added

comment:12 by Aymeric Augustin, 11 years ago

New PR based on FunkyBob's work: https://github.com/django/django/pull/1972

comment:13 by Aymeric Augustin, 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.

in reply to:  13 comment:14 by Florian Apolloner, 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 Florian Apolloner, 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 Aymeric Augustin, 11 years ago

We should probably implement something in django.test.signals to handle situations where the CACHES settings is changed.

comment:17 by Aymeric Augustin <aymeric.augustin@…>, 11 years ago

Resolution: fixed
Status: assignedclosed

In 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.augustin@…>
Date: Sat Nov 23 14:18:07 2013 +0100

Looked up the template_fragments cache at runtime.

commit 905a74f52b24a198f802520ff06290a94dedc687
Author: Aymeric Augustin <aymeric.augustin@…>
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.augustin@…>
Date: Sat Nov 23 12:23:57 2013 +0100

Removed create_cache function.

commit 8e274f747a1f1c0c0e6c37873e29067f7fa022e8
Author: Aymeric Augustin <aymeric.augustin@…>
Date: Sat Nov 23 12:04:52 2013 +0100

Updated docs to describe a simplified cache backend API.

commit ee7eb0f73e6d4699edcf5d357dce715224525cf6
Author: Curtis Maloney <curtis@…>
Date: Sat Oct 19 09:49:24 2013 +1100

Fixed #21012 -- Thread-local caches, like databases.

by Florian Apolloner, 11 years ago

Attachment: locmem.diff added

comment:18 by Florian Apolloner, 11 years ago

Attached the changes Ned suggested; waiting for feedback from Alex.

comment:19 by Florian Apolloner <florian@…>, 11 years ago

In 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

comment:20 by Tim Graham <timograham@…>, 10 years ago

In d038c547b5ce585cbf9ef5bb7e5298f52e4a243b:

Removed django.core.cache.get_cache() per deprecation timeline; refs #21012.

comment:21 by Mariusz Felisiak <felisiak.mariusz@…>, 4 years ago

In 148702e7:

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.

Note: See TracTickets for help on using tickets.
Back to Top