IBX-5502: Added additional tag cleaning to limit down number of orphaned tag en… #477
Conversation
mateuszbieniek
changed the title
|
There was a problem hiding this comment.
The solution to the issue concerns me a bit. The idea behind tags was always to catch everything by using just one, so it feels like this needs to be resolved in a different place. However maybe I'm wrong or maybe there's no other way to solve it - POV ping @ibexa/php-dev.
@mateuszbieniek before moving with the remarks to the solution, see the one about getContentTags - in that case prior diff comments to that are not applicable.
| $locationPathTags = array_map(function (Content\Location $location): string { | ||
| return $this->cacheIdentifierGenerator->generateTag(self::LOCATION_PATH_IDENTIFIER, [$location->id]); | ||
| }, $locations); |
There was a problem hiding this comment.
Given LOCATION_PATH_IDENTIFIER tag exists only in conjunction with LOCATION_IDENTIFIER tag, are you sure both are necessary?
Moreover, I don't see LOCATION_PATH_IDENTIFIER tags (lp-%s) in the list off affected by the issue.
There was a problem hiding this comment.
As mentioned in the PR's description, the list presented only has an example, and the number of orphaned tags is not limited to it. But if you look really closely, you will also find mention of the lp- tag ;)
| @@ -335,12 +335,33 @@ public function updateContent($contentId, $versionNo, UpdateStruct $struct) | |||
| { | |||
| $this->logger->logCall(__METHOD__, ['content' => $contentId, 'version' => $versionNo, 'struct' => $struct]); | |||
| $content = $this->persistenceHandler->contentHandler()->updateContent($contentId, $versionNo, $struct); | |||
| $this->cache->invalidateTags([ | |||
| $this->cacheIdentifierGenerator->generateTag( | |||
| $locations = $this->persistenceHandler->locationHandler()->loadLocationsByContent($contentId); | |||
There was a problem hiding this comment.
Have you checked the impact of this on performance? Every load inside caching system is a potential risk. I'd like to avoid the situation where we release some memory, but make longer update time as the cost.
There was a problem hiding this comment.
Have you checked the impact of this on performance?
Only in the empirical way. If you have dozens or hundreds of locations, ofc this can have an impact, but then, probably this one is less of your worry then.
Every load inside caching system is a potential risk.
Any explanation behind this one? The change is made in a handler, not in the caching system, which to be frank main purpose is to do "loads".
I'd like to avoid the situation where we release some memory, but make longer update time as the cost.
That's, in my opinion is a wrong approach. This helps to clear unevictable memory from Redis - not doing this always results in eventually reaching a point when the whole app crashes. So this is not memory vs longer updates, but making the project less prone to failure due to OOM errors.
| [$contentId, $versionNo] | ||
| ), | ||
| ]); | ||
| [$contentId, $versionNo - 1] |
There was a problem hiding this comment.
We should not invalidate tags for an unrelated versions. The cache performance side effects of that might be difficult to predict.
There was a problem hiding this comment.
That's true, and I agree. Any suggestionson how we can handle c-<contentId>-v-<prevVersionNo> tags?
There was a problem hiding this comment.
ping @alongosz
| $locationTags = array_map(function (Content\Location $location): string { | ||
| return $this->cacheIdentifierGenerator->generateTag(self::LOCATION_IDENTIFIER, [$location->id]); | ||
| }, $locations); |
There was a problem hiding this comment.
This is not very readable to me. How about:
| $locationTags = array_map(function (Content\Location $location): string { | |
| return $this->cacheIdentifierGenerator->generateTag(self::LOCATION_IDENTIFIER, [$location->id]); | |
| }, $locations); | |
| $locationTags = array_map( | |
| fn (Content\Location $location): string => $this->cacheIdentifierGenerator->generateTag( | |
| self::LOCATION_IDENTIFIER, | |
| [$location->id] | |
| ), | |
| $locations | |
| ); |
There was a problem hiding this comment.
Well, to be honest in my opinion thi is even less readable ;)
But no strong opinion here.
mateuszbieniek
force-pushed
the
ibx-5502
branch
from
87c41af to
e8b66fc
Compare
|
Rebase of ezsystems/ezplatform-kernel#372
v4.6Due to how the cache is implemented in Symfony-based projects using the Redis adapter (and probably - not only Redis), entries (Members) inside a Tag (Set) are removed only when the tag is purged. Due to that, we can encounter on multiple occasions a situation, when a Tag is purged, resulting in cache items (Keys) being purged as well, but only the Tag that triggered a purge is cleaned up. All other tags that contain entries pointing to cache items that no longer exist will remain there until the affected tag is purged - which often is "never".
This results in orphaned entries in tags (or even in whole orphaned tags) filling up the memory. Memory allocated to tags in Redis is nonevictable, therefore it is freed only when purged on purpose (
maxmemory-policysetting won't affect orphaned members/sets). On very big and/or constantly changing content structures, this unevictable memory allocated to orphaned tags and entries can significantly lower the amount of memory available for standard cache items to be stored.Additional Tag purging added in this PR reduces this effect, but - unfortunately - it is not possible to resolve it fully with current Symfony's cache adapters implementation, due to the fact that there is no available interface to remove an entry from a Tag, without purging it whole - and that would result in purging cache items that were nor related to the original action performed on content.
Additional tag purging introduced by PR:
ContentHandler::updateContent:Multiple content-related entries (
ibx-c-<contentId>-<versionNo>-0,ibx-c-<contentId>-<versionNo>-<languageCode>,ibx-cvi-<contentId>,ibx-cl-<contentId>-pfdare orphaned in tags:l-<locationId>,lp-<locationId>,c-<contentId>-v-<versionNo>,c-<contentId>-v-<prevVersionNo>. Added purging of those tags to avoid it.TrashHandler::trashSubtree:Multiple content-related entries are orphaned (
ibx-c-<contentId>-v<versionNumber>,ibx-cl-<contentId>,ibx-cvi-<contentId>,ibx-l-<locationId>-<languageCode>,ibx-c-<contentId>-<versionNo>-0,ibx-c-<contentId>-<versionNo>-<languageCode>,ibx-cvi-<contentId>... (many more) are orphaned in tags:l-<locationId>,c-<contentId>-v-<versionNo>.Handling those two occurrences helps to limit greatly the number of orphaned members.
My tests show, that one of the biggest culprits here is
lp-<locationIdOfParent>tag, which can have multiple orphaned entries - therefore it might be a good idea to purge it as well, to avoid blocked memory in cost of potentially small performance loss - up for discussion.This PR won't resolve the issue fully but limits the scope of it using available tools. To resolve this fully, we will need (in my opinion):
But those two are out of this PR scope.
Checklist:
$ composer fix-cs).@ezsystems/engineering-team).