Do not treat an empty array as not set in caches. #12784
Conversation
On testing Redis we found that even after Redis cache had been built the set function was still being called. It turned out the reason is that when a value was set to an empty array that was being treated as FALSEY. In fact the cache function returns NULL if nothing is set & no default is specified & we should only treat NULL as a cache miss
| @@ -75,7 +75,7 @@ public static function &getItem($group, $path, $componentID = NULL) { | |||
| $cache = CRM_Utils_Cache::singleton(); | |||
| $cleanKey = self::cleanKey($argString); | |||
| self::$_cache[$argString] = $cache->get($cleanKey); | |||
| if (!self::$_cache[$argString]) { | |||
| if (self::$_cache[$argString] === NULL) { | |||
There was a problem hiding this comment.
Aside: on skimming, I had a knee-jerk reaction of using isset()/array_key_exists() and about handling of literal NULL values... but the line as written does make sense.
Firstly, we are guaranteed to have some value in this line (b/c of the preceding $cache->get()), so we don't need to be paranoid about the notation for reading self::$_cache.
Secondly, there is arguably an issue when the value is a purposeful NULL (i.e. it still does the extra lookup), but that's a rare scenario, and the cure would likely be as bad as the disease. How so? To disambiguate a purposeful NULL from a cache-miss-NULL, you either need to change the contract for $cache (veering away from PSR-16) or do an extra lookup ($cache->has(...)). The former is unpleasant from a qualitative/architectural standpoint. The latter would add an extra Redis lookup for all cache-misses (somewhat common) in order to avoid a SQL lookup for NULL values (uncommon). Evaluating that micro-optimization turns into numbers game that I don't really want to play. It's easier to KISS.
Generally, I think this PR is doing good if it just improves the cache performance for other FALSE-ish values (0, '0', FALSE, array()). Don't sweat NULL.
For some reason, lost in svn days, the membershipType pseudoconstant is set to not use the cache by passing = true. It was probably test frustration & does not make sense now
Only NULL is a cache miss....
Change-Id: I9abdc6e6adaa67f027b5682cb1f0ee84f30e3d94
|
Jenkins re test this please |
|
@eileenmcnaughton should this be against 5.6 |
|
@seamuslee001 I don't think it's a regression - just something not identified earlier |
|
test this please |
|
@seamuslee001 we have deployed this in production - my feeling is that it should be 'merge on pass' from the above & got derailed by the server issues - do you agree? |
|
yeh i'm good with that Tim seems fine with it adding merge on pass |
|
test failures unrelated merging as per the tag |
Overview
Reduce cache miss calls by respecting empty arrays as a cache hit
Before
Cache calls which return empty arrays are treated as misses
After
Cache calls which return empty arrays are treated as hits
Technical Details
On testing Redis we found that even after Redis cache had been
built the set function was still being called. It turned out the reason is that
when a value was set to an empty array that was being treated as FALSEY.
In fact the cache function returns NULL if nothing is set & no default is
specified & we should only treat NULL as a cache miss
Comments
As an aside Our testing is showing a speed decrease when we switch from Array Caching to Redis caching. My thinking is that array caching is in fact more efficient & ideally it would be array caching & fall back on Redis caching - so if not loaded this session that is where it loads from. That is how the BAO_Cache::getItem function works but the Civi::cache()->get does not.