Rewrite subTypeInfo to use caching mechanism #14715

eileenmcnaughton · 2019-07-03T01:27:58Z

Overview

Fix unnecessary DB lookups on sites with no sub types due to cache miss

Before

Cache being missed

After

Cache not being missed

Technical Details

So @totten @seamuslee001 I spotted this one getting a lot of setex in our redis logs & it also seems a good one for me to clarify the 'right' methodology. The issue here is that the change in this PR seems to work & fixes it but I'm no longer sure the best way to clear the cache - currently if you add a type it calls

     // reset the cache after adding
    self::subTypeInfo(NULL, FALSE, FALSE, TRUE);

This PR flattens the key so we would need to do multiple cache deletes to achieve the same - what is the right approach?

Comments

civibot · 2019-07-03T01:28:00Z

(Standard links)

If this is your first pull-request for CiviCRM, please browse CONTRIBUTING.md for information about the development and testing processes.
If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

seamuslee001 · 2019-07-03T19:46:58Z

Test fails look related @eileenmcnaughton

eileenmcnaughton · 2019-07-03T19:48:03Z

@seamuslee001 yeah this is more a discussion piece at the moment - I had some questions on it ^^

seamuslee001 · 2019-07-03T19:59:38Z

@eileenmcnaughton when you say it flatterns the key what do you mean?

eileenmcnaughton · 2019-07-03T20:33:12Z

So the current function stores a static array

$cache = ['permutation1' => x, 'permutation2' => y];

If we look at things like OptionGroup::values it saves a value for each option group which is like

Cache->set('permutation1', x)
Cache->set('permutation1', y)

This later feels better to me - except when you want to clear them all - which we would do here if editing contact subtypes

totten · 2019-07-10T03:20:06Z

This PR flattens the key so we would need to do multiple cache deletes to achieve the same - what is the right approach?

IMHO, adding a new cache-group is probably the simplest way to get the desired effect. You'd probably want to:

Declare the cache in Container.php.
- Bonus: The default cache doesn't specify withArray=>fast, but you can specify it on a new group. If I'm reading correctly, that's closer to the previous policy (ie using the composition of a php-local cache plus a redis cache).
Update CRM_Utils_System::flushCache() to also flush this cache.

There are alternatives, of course, e.g.

If you can easily list all the values of permutation1, permutation2, etc -- then just delete all of them. ($cache->deleteMultiple(['permutation1', 'permutation2', ...])).
Some cache backends support tagging and/or hierarchies. The PSR-16 interface doesn't expose this functionality. It might be possible to write or find an extension to PSR-16 that makes it possible, but I'd regard that as a distinct project. (Probably 1-3 days' work.)

eileenmcnaughton · 2019-07-10T03:25:11Z

@totten so I feel like we'll quickly get a lot of containers then? Perhaps that is not a bad thing. We can declare them in extensions too it just feels kinda like a heavy lift

eileenmcnaughton · 2019-07-10T03:47:04Z

"Bonus: The default cache doesn't specify withArray=>fast"

@totten I thought we HAD made it so that we got fast cache mgemt by default?

totten · 2019-07-10T03:50:39Z

@eileenmcnaughton Yeah, there is a bit of boilerplate in declaring a cache.

Something like this mitigates that. We can probably come up with a similar mitigation for flushCache() so that flushCache() basically clears all cache.* services.

The most aggressive response to the boilerplate issue would be to move the cache mgmt out of Civi\Core\Container and create a dedicated Civi\Core\CacheManager service which could be more Clever. Ex: If you try to access an unrecognized cache, then it creates it implicitly/on-demand per some general policy, but does it in a way were we can still discover/clear out any of those lazily-made caches. We'd lose the presumption that all caches are injectable services, but otherwise (with a day or so's work) I think it could be done as a drop-in update.

totten · 2019-07-10T03:54:17Z

@totten I thought we HAD made it so that we got fast cache mgemt by default?

I think that's true for caches that go through the CRM_Core_BAO_Cache_Psr16 adapter. But to my reading of CRM_Utils_Cache and Civi\Core\Container, the system-wide short+long caches don't.

eileenmcnaughton · 2019-07-10T03:56:50Z

hmm - I had thought we got to the point where Civi::cache()->get() WAS going through that adapter - we definitely don't want this (or the already merged groups one) to not be array backed.

eileenmcnaughton · 2019-07-10T04:06:34Z

Discussion on chat

https://chat.civicrm.org/civicrm/pl/9n5gbbesf3bsjn8dzw5q6bcgyw

short version - this risks having a detrimental effect on performance for Redis/Memcache users without figuring out the fastArray stuff

eileenmcnaughton · 2019-08-02T04:16:25Z

Added 'has-test' because after stepping through the tests that failed when I made a mistake I can see there is pretty solid cover here

eileenmcnaughton · 2019-08-02T04:18:52Z

As an aside - I'm pretty sure that we can drop the $ignoreCache parameter too -will put up a PR to do so if I have a reviewer

eileenmcnaughton · 2019-08-02T21:40:59Z

After reviewing the test fails I decided to rename the cache contactTypes since it needs to be cleared whenever the contact type cache is cleared (outside tests that should be rare)

I also note that when adding or deleting a contactType we need to

clear the cached data
rebuild the menus

I kinda wonder if
a) we should have an on_clear callback that rebuilds the menus whenever the cache is cleared &
b) move both to some kinda listener.. - I think @colemanw sees listeners / some sort of post action as the way to go with creates & adds

Also I think we should add a pseudoconstant cache @seamuslee001 to move the cached vars in the Core_Pseudoconstant class into

eileenmcnaughton · 2019-08-07T01:32:30Z

@seamuslee001 I think this is mergeable now

seamuslee001 · 2019-08-08T22:08:21Z

CRM/UF/Page/ProfileEditor.php

@@ -50,7 +50,7 @@ public static function registerProfileScripts() {
            'phoneType' => CRM_Core_PseudoConstant::get('CRM_Core_DAO_Phone', 'phone_type_id'),
          ],
          'initialProfileList' => $ufGroups,
-          'contactSubTypes' => CRM_Contact_BAO_ContactType::subTypes(),
+          'contactTypes' => CRM_Contact_BAO_ContactType::subTypes(),


@eileenmcnaughton why the change of key here looks odd

@seamuslee001 agree it looks wrong - changed it back

seamuslee001 · 2019-08-08T22:09:15Z

CRM/Contact/BAO/ContactType.php

      CRM_Core_BAO_Navigation::resetNavigation();
+      Civi::cache('contactTypes')->clear();


why are you using clear() here when we have consistently used flush() which is an alias of clear

because flush is actually deprecated!

ok do we want consistance or non deprecated code paths :-P

we get consistency when the others are migrated over

seamuslee001 · 2019-08-08T22:09:25Z

CRM/Contact/BAO/ContactType.php

-
-    // reset the cache after adding
-    self::subTypeInfo(NULL, FALSE, FALSE, TRUE);
+    Civi::cache('contactTypes')->clear();


same as L604

seamuslee001 · 2019-08-08T22:09:50Z

CRM/Utils/System.php

@@ -1442,6 +1442,7 @@ public static function flushCache() {
      Civi::cache('groups')->flush();
      Civi::cache('navigation')->flush();
      Civi::cache('customData')->flush();
+      Civi::cache('contactTypes')->clear();


Same as on ContactType BAO L604

This seems to be getting a lot of cache misses on prod - presumably due to a lack of sub types so fixing to use 'modern caching';

seamuslee001 · 2019-08-09T02:27:18Z

ok i'm ok with this now and if Jenkins is then we can merge. I'm pretty confident we have good test coverage covering this

eileenmcnaughton · 2019-08-09T02:30:52Z

@seamuslee001 thanks - working through this makes me feel like we probably also want a cache for 'pseudoconstants' - basically anything handled by CRM_Core_Pseudoconstant -

It was a good exercise to work through

civibot bot added the master label Jul 3, 2019

eileenmcnaughton force-pushed the cache_sub branch 2 times, most recently from 670f518 to 50a8e6f Compare August 2, 2019 04:15

eileenmcnaughton added the has-test label Aug 2, 2019

eileenmcnaughton force-pushed the cache_sub branch from 50a8e6f to 42a9f59 Compare August 2, 2019 21:37

seamuslee001 reviewed Aug 8, 2019

View reviewed changes

Rewrite subTypeInfo to use caching mechanism

81c4c14

This seems to be getting a lot of cache misses on prod - presumably due to a lack of sub types so fixing to use 'modern caching';

eileenmcnaughton force-pushed the cache_sub branch from 42a9f59 to 81c4c14 Compare August 9, 2019 02:23

seamuslee001 added the merge on pass label Aug 9, 2019

eileenmcnaughton merged commit d92d559 into civicrm:master Aug 9, 2019

eileenmcnaughton deleted the cache_sub branch August 9, 2019 06:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rewrite subTypeInfo to use caching mechanism #14715

Rewrite subTypeInfo to use caching mechanism #14715

eileenmcnaughton commented Jul 3, 2019

civibot bot commented Jul 3, 2019

seamuslee001 commented Jul 3, 2019

eileenmcnaughton commented Jul 3, 2019

seamuslee001 commented Jul 3, 2019

eileenmcnaughton commented Jul 3, 2019

totten commented Jul 10, 2019

eileenmcnaughton commented Jul 10, 2019

eileenmcnaughton commented Jul 10, 2019

totten commented Jul 10, 2019

totten commented Jul 10, 2019

eileenmcnaughton commented Jul 10, 2019

eileenmcnaughton commented Jul 10, 2019

eileenmcnaughton commented Aug 2, 2019

eileenmcnaughton commented Aug 2, 2019

eileenmcnaughton commented Aug 2, 2019

eileenmcnaughton commented Aug 7, 2019

seamuslee001 Aug 8, 2019

eileenmcnaughton Aug 9, 2019

seamuslee001 Aug 8, 2019

eileenmcnaughton Aug 9, 2019

seamuslee001 Aug 9, 2019

eileenmcnaughton Aug 9, 2019

seamuslee001 Aug 8, 2019

seamuslee001 Aug 8, 2019

seamuslee001 commented Aug 9, 2019

eileenmcnaughton commented Aug 9, 2019

		CRM_Core_BAO_Navigation::resetNavigation();
		Civi::cache('contactTypes')->clear();

Rewrite subTypeInfo to use caching mechanism #14715

Rewrite subTypeInfo to use caching mechanism #14715

Conversation

eileenmcnaughton commented Jul 3, 2019

Overview

Before

After

Technical Details

Comments

civibot bot commented Jul 3, 2019

seamuslee001 commented Jul 3, 2019

eileenmcnaughton commented Jul 3, 2019

seamuslee001 commented Jul 3, 2019

eileenmcnaughton commented Jul 3, 2019

totten commented Jul 10, 2019

eileenmcnaughton commented Jul 10, 2019

eileenmcnaughton commented Jul 10, 2019

totten commented Jul 10, 2019

totten commented Jul 10, 2019

eileenmcnaughton commented Jul 10, 2019

eileenmcnaughton commented Jul 10, 2019

eileenmcnaughton commented Aug 2, 2019

eileenmcnaughton commented Aug 2, 2019

eileenmcnaughton commented Aug 2, 2019

eileenmcnaughton commented Aug 7, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seamuslee001 commented Aug 9, 2019

eileenmcnaughton commented Aug 9, 2019