Implement tag-based cache invalidation, and apply to Gedcom Records. #3748
Conversation
Codecov Report
@@ Coverage Diff @@
## main #3748 +/- ##
============================================
- Coverage 26.15% 23.70% -2.46%
- Complexity 11866 11987 +121
============================================
Files 1071 1072 +1
Lines 40884 44296 +3412
============================================
- Hits 10694 10500 -194
- Misses 30190 33796 +3606
Continue to review full report at Codecov.
|
|
A few initial thoughts, based on a quick scan... If we only ever use one tag on an item, then do we need the taggable functionality? Updating the signature to cache::remember() might break existing third-party modules which use the cache with a TTL. It might be safer to add the There can be many thousands of pending changes (e.g. after a search/replace) - which is a lot of data to fetch from the DB. If we invalidate its cache (e.g. during a subsequent search/replace), we could be loading thousands of rows, thousands of times. I'll look a bit more closely at this over the coming days. |
|
Thanks Greg for this initial feedback.
That was going to be my initial approach. The problem is that the keys are unique, quite specific to the cache item creator, and more importantly rather unpredictable for the item invalidator. A few of the keys are based on What I like with the tags is that this is more a logical topic approach. There is still some kind of coupling, as the creator and the invalidator need to agreed on a tag naming, but that looks a lot less painful (and worst case, you just create a tag for nothing, or invoke a tag invalidation for nothing). Moreover, the tag can be shared, which I am actually using across the different cache item creators (the GedcomRecord factories, the To be honest, I have no credit in this approach, this is the one suggested by the Symfony documentation itself for cache invalidation (besides Expiration). To quote it:
True, I did not pay much attention to it, and have not been very considerate for custom modules (which is a shame coming from me, with my several custom modules...).
Again, I understand where you are coming from with the concerns on the pending changes, as the current implementation is indeed quite rigid, and tend to be an all-or-nothing approach. That is why this is using a specific tag I would however argue a bit with that specific example, as it looks to me more like a flawed design, or rather a prioritisation of one concern (performance) over another one (data integrity): data integrity needs to have cache invalidation to make sure that the object you are manipulating is in line with what the database holds. And on that respect, I think that |
jon48
force-pushed
the
feature/cache-invalidation
branch
from
2104542
to
7acddc2
Compare
|
I have updated the PR to rebase (and squash) on top of the latest code. Regarding the potential of thousands of calls being done in scenarios like data fixes, I have had a look at the code, and have gathered metrics, but they do not fall into the specific case that could indeed trigger it. Here are some metrics for some of the data fix, where I added log (arbitrarily category Search - Replace
Missing Married Name
Missing Deaths
Names tags
Names slashes
Rename places
|
This is a proposal to try and address #3747. A few files changes, but there is only really a couple that contain the significant changes.
The invalidation is done through the addition of tags to the Cache object (a feature supported by the underlying Symfony cache implementation and adapters). This requires a stricter constraint on the Cache signature itself (accepting a
Symfony\Contracts\Cache\TagAwareCacheInterfaceinstead ofSymfony\Contracts\Cache\CacheInterfaceadapter).The object creating cache items just needs to declare a tag, that can be then invoked for invalidation by other objects updating the object. The overall architecture changes are quite limited, and that can be applied to other invalidations than GedcomRecords.
For the GedcomRecords themselves, tags are defined as
gedrec-XXX@YYwhereXXXis the Xref of the object, andYYthe tree ID. They are added whenever a cache item related to a GedcomRecord is created (the factories, thecanShowmethod, an improvement on thegetFactsWithMediamethod of the MediaTabModule), and the methods onGedcomRecordupdating it can then invalidate that tag.There is an opinionated implementation choice, as only the Array adapter is invalidated by GedcomRecord, not the FileSystem one (it can be, but I thought it may not be required - I could not really decide on it).
The pending changes implementation can be controversial as well, as any change on a record invalidate the whole pending changes (as it is all-or-nothing for a tree). Your experience would be of interest on that one.
I do not see many impacts of that implementation on the existing (performances to be assessed more explicitly if you have any tool), but it can protect the system from weird loading sequences, and it provides a mechanism that can be extended to any cache usage (even allowing modules to invalidate predefined tags if necessary).
A new unit test has been added as well for the Cache class, which I could rework is the PR is not accepted.