Opened 22 months ago

Closed 19 months ago

Last modified 5 months ago

#21012 closed Cleanup/optimization (fixed)

Provide shared "caches" dict to avoid creating multiple cache class instances.

Reported by: FunkyBob Owned by: aaugustin
Component: Core (Cache system) Version: master
Severity: Normal Keywords:
Cc: apollo13, 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 apollo13 19 months ago.

Download all attachments as: .zip

Change History (21)

comment:1 Changed 22 months ago by FunkyBob

  • Needs documentation unset
  • Needs tests unset
  • Owner changed from nobody to FunkyBob
  • Patch needs improvement unset
  • Status changed from new to assigned

comment:2 Changed 22 months ago by loic84

  • Triage Stage changed from Unreviewed to Ready for checkin

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

comment:3 Changed 22 months ago by apollo13

  • Patch needs improvement set
  • Triage Stage changed from Ready for checkin to 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 Changed 22 months ago 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.

comment:5 follow-up: Changed 21 months ago 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
Last edited 21 months ago by jezdez (previous) (diff)

comment:6 in reply to: ↑ 5 Changed 21 months ago by apollo13

  • Cc apollo13 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 Changed 21 months ago 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...

comment:8 Changed 21 months ago 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?

comment:9 Changed 20 months ago 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.

comment:10 Changed 20 months ago by aaugustin

  • Owner changed from FunkyBob to aaugustin

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 Changed 20 months ago by FunkyBob

  • Cc FunkyBob added

comment:12 Changed 19 months ago by aaugustin

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

comment:13 follow-up: Changed 19 months ago 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.

comment:14 in reply to: ↑ 13 Changed 19 months ago by apollo13

LocMemCache uses global state, so it would still be process-local; but we should ensure that __init__ is actually threadsafe.

comment:15 Changed 19 months ago 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

comment:16 Changed 19 months ago by aaugustin

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

comment:17 Changed 19 months ago by Aymeric Augustin <aymeric.augustin@…>

  • Resolution set to fixed
  • Status changed from assigned to closed

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.

Changed 19 months ago by apollo13

comment:18 Changed 19 months ago by apollo13

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

comment:19 Changed 19 months ago by Florian Apolloner <florian@…>

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 Changed 5 months ago by Tim Graham <timograham@…>

In d038c547b5ce585cbf9ef5bb7e5298f52e4a243b:

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

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