Bug 8039 - Fix FileCache cache miss behavior
Fix FileCache cache miss behavior
Status: NEW
Product: OJS
Classification: Unclassified
Component: Framework
2.4.x
All All
: P3 normal
Assigned To: beghelli
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-11-22 13:27 PST by Alec Smecher
Modified: 2012-12-03 05:53 PST (History)
2 users (show)

See Also:
Version Reported In:
Also Affects:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Alec Smecher 2012-11-22 13:27:55 PST
Currently any cache miss invalidates the entire cache, leading to a reload. This will probably make some operations (e.g. reading journal settings that are not set) slow.
Comment 1 Juan Pablo Alperin 2012-11-22 13:38:47 PST
While you're fixing this, could you add a bit of documentation to the FileCache to explain that it should not use the setCache method and use setEntireCache instead? That tripped me up recently.
Comment 2 Alec Smecher 2012-11-22 13:42:35 PST
Yup, or I might tear out FileCache/CacheManager entirely in favour of someone else's better implementation. There ought to be dozens.
Comment 3 beghelli 2012-11-26 09:17:25 PST
At least in FileCacheTest case, the problem is not with retrieving data from cache that is not set, but is using the setCache method. Instead of setting the data on cache, it will flush it. That's why the second call to retrieve a data from file cache will result in cachemiss fallback being called.

Unless we have that kind of use (trying to set a data into cache if it was not found previously there) I don't think it will cause problems with performance.

Anyway, maybe the problem is more related with the setCache method and less with the get one. Flushing the cache instead of setting something there is a very strange operation for that method; it receives even data and id parameters but does nothing with them.
Comment 4 Alec Smecher 2012-11-26 09:38:55 PST
Bruno, most _cacheMiss cases (e.g. in JournalSettingsDAO) use setCache to attempt to store null values in the cache. That's where the problem call is coming in. Even if that was removed, any _cacheMiss (i.e. trying to get a journal setting that isn't in the DB) would result in a full reload of the journal settings even though they're already cached in local memory. So I suppose the bug is technically outside FileCache, but many _cacheMiss implementations calling on FileCache result in bad behavior.
Comment 5 beghelli 2012-11-26 09:46:29 PST
Thanks Alec, but I have a doubt: why we try to set data cache when we know that it will flush the existing data and not setting the one that we are passing? Just to make sure that the cache is updated next time?
Comment 6 Alec Smecher 2012-11-26 10:09:42 PST
Bruno, IIRC the plan was to have updateSettings calls call setCache, so that the cache is regenerated next time. The problem code for cache misses was an attempt to work around the problem of the cache being flushed and regenerated every time a request came in for an item not in the cache.

As I remember, these are probably complications imposed by trying to write a single caching API that is implementation-dependent, i.e. works for memcache, opcode cache-based object caches, and file caches. Much of that has been simplified because nobody has ever used anything but file caches for the most common caches like journal settings caches.

Before we spend time refining our own FileCache implementation I think it's worth looking at alternatives like <http://pear.php.net/package/Cache>.
Comment 7 beghelli 2012-11-29 02:29:26 PST
I couldn't verify completely the issue that this bug is describing. Let's take the JournalSettingsDAO example. If I try to get a setting not present in DB, like 'notPresentedSetting', and the journal setting cache is already loaded, then the cache will not call the cachemiss fallback. What's missing is just the requested data, and not the data inside cache:

if (!isset($this->cache)) return $this->cacheMiss;
return (isset($this->cache[$id])?$this->cache[$id]:null);

But I could verify that if you are trying to load 'nonPresentSetting' and the journal setting cache is not loaded yet, you will have the situation described in this bug. See the cache miss fallback from JournalSettingsDAO:

$settings =& $this->getSettings($cache->getCacheId());
if (!isset($settings[$id])) {
    $cache->setCache($id, null);
    return null;
}
return $settings[$id];

So, I think that's not any cache miss that will invalidate the entire cache. It's only those who try to get a not present data with the cache data not loaded yet. For those cases, the cache will be loaded and then erased by the setCache called inside the cache miss fallback. 

>Even if that was removed, any _cacheMiss (i.e. trying to get a journal >setting that isn't in the DB) would result in a full reload of the journal >settings even though they're already cached in local memory.

Couldn't verify this. If we remove the setCache inside the cache miss fallback, the cache will not be flushed at all. Maybe I am missing something...

What causing confusion is that actually the cache miss is being called only when we have a totally absence of data inside cache. But the cache miss concept is more specific: it's the absence of the requested data inside the cache. If we had cache miss fallback being called for those cases also, and with the setCache still being used, we would have the situation described in this bug.

To fix that I think we need to call cache miss for every cache miss situation. And then we have to fix the setCache method for the FileCache. Instead of flushing the content, it should do what it's expected to do: set data inside the cache. The proposed Cache package might help us handling this (setting additional cache data inside the file), but I think that we can continue to use our Cache classes as a wrapper around it.

Does it make any sense?
Comment 8 Alec Smecher 2012-11-29 12:47:20 PST
That makes sense to me, Bruno. Did you have a look at the 3rd-party cache libraries?
Comment 9 beghelli 2012-11-29 13:25:01 PST
(In reply to comment #8)
> That makes sense to me, Bruno. Did you have a look at the 3rd-party cache
> libraries?

Yes, I spent more time on that one that's a pear module. It has a cache container interface and all the specific implemented containers (file, shared memory, database, etc) seems to respect that interface. :P

Actually we have containers for Xcache, APC cache and mem cache, so, if we want to use this library and still support those kind of caching containers, we should extend the base Container class and adapt our current code.

If we can drop those 3 types of containers, then I think it's just a matter of keeping and adapt the CacheManager to handle all the cache requests (instantiate the correct cache object, etc) and leave the rest to the Cache library.
Comment 10 Alec Smecher 2012-11-30 09:05:49 PST
There are currently two useful types of caches: file caches (used for settings and locale files), and object caches (used to persist frequently-used objects between connections, mostly IIRC issues and articles in the reader interface). I would be happy to replace both if the API works properly. The current structures are working OK, but a more refined/flexible implementation would be good, and it would also mean less of our own code to maintain.