IBX-5502: Added additional tag cleaning to limit down number of orphaned tag entries #372

mateuszbieniek · 2023-05-09T14:25:58Z

Question	Answer
JIRA issue	IBX-5502
Type	bug
Target Ibexa version	`v3.3`
BC breaks	no

Due to the how 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-policy setting 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>-pfd are 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):

an interface to remove entries from sets
a garbage collector, as entries in tags might become orphaned as well when a key is evicted by Redis when maxmemory limit is reached.
But those two are out of this PR scope.

Checklist:

Provided PR description.
Tested the solution manually.
Checked that target branch is set correctly (master for features, the oldest supported for bugs).
Ran PHP CS Fixer for new PHP code (use $ composer fix-cs).
Asked for a review (ping @ezsystems/engineering-team).

…tries

sonarcloud · 2023-05-09T14:49:23Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
2.2% Duplication

konradoboza

Failing tests need to be investigated. Besides, you mentioned clearing tags with -languageCode suffix and I am kinda missing where it's done exactly.

eZ/Publish/Core/Persistence/Cache/ContentHandler.php

konradoboza · 2023-05-10T09:36:56Z

eZ/Publish/Core/Persistence/Cache/ContentHandler.php

- $this->cacheIdentifierGenerator->generateTag(
+ $locations = $this->persistenceHandler->locationHandler()->loadLocationsByContent($contentId);
+
+ $locationTags = array_map(function (Content\Location $location) {


Suggested change

$locationTags = array_map(function (Content\Location $location) {

$locationTags = array_map(function (Content\Location $location): string {

eZ/Publish/Core/Persistence/Cache/ContentHandler.php

eZ/Publish/Core/Persistence/Cache/TrashHandler.php

mateuszbieniek · 2023-08-10T11:48:41Z

I apologize - I've missed the CR.

Failing tests need to be investigated.

Fixed.

Besides, you mentioned clearing tags with -languageCode suffix and I am kinda missing where it's done exactly.

It is not a tag, but a key. For example, this key is a member of a "Location" tag - those additional tag purgings resolves this key being orphaned when Content is sent to the Trash.

sonarcloud · 2023-08-10T12:20:15Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
2.2% Duplication

kisztof

Could you implement the code suggestions provided by @konradoboza https://github.com/ezsystems/ezplatform-kernel/pull/372/files#r1189636279

Co-authored-by: Konrad Oboza <[email protected]>

mateuszbieniek · 2024-01-04T10:19:52Z

@kisztof done (test failing not related to pr) and thank you for reminding me. Happy new year!

sonarcloud · 2024-01-04T10:22:15Z

Quality Gate passed

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
No data about Coverage
2.2% Duplication on New Code

See analysis details on SonarCloud

konradoboza

My tests show, that one of the biggest culprits here is lp- 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.

Are you able to tell what performance losses are we observing here?

konradoboza · 2024-01-04T14:57:20Z

eZ/Publish/Core/Persistence/Cache/Tests/ContentHandlerTest.php

@@ -47,7 +49,7 @@ public function providerForUnCachedMethods(): array
 ['setStatus', [2, 0, 1], [['content_version', [2, 1], false]], null, ['c-2-v-1']],
 ['setStatus', [2, 1, 1], [['content', [2], false]], null, ['c-2']],
 ['updateMetadata', [2, new MetadataUpdateStruct()], [['content', [2], false]], null, ['c-2']],
- ['updateContent', [2, 1, new UpdateStruct()], [['content_version', [2, 1], false]], null, ['c-2-v-1']],
+ //['updateContent', [2, 1, new UpdateStruct()], [['content_version', [2, 1], false]], null, ['c-2-v-1']],


Why is this line commented?

As you can see this is a copypaste of the line above - I was probably checking something and overlooked it.
Ofc I'll fix that - thank you for spotting this!

konradoboza · 2024-01-04T14:58:15Z

eZ/Publish/Core/Persistence/Cache/Tests/ContentHandlerTest.php

+ /**
+ * @covers \eZ\Publish\Core\Persistence\Cache\ContentHandler::updateContent
+ */
+ public function testUpdateContent()


There are plenty of missing return types in here, but increasing code quality even with such small steps is worth the effort to me:

Suggested change

public function testUpdateContent()

public function testUpdateContent(): void

konradoboza · 2024-01-04T15:02:59Z

eZ/Publish/Core/Persistence/Cache/Tests/ContentHandlerTest.php

+ $this->cacheIdentifierGeneratorMock
+ ->expects($this->exactly(5))
+ ->method('generateTag')
+ ->withConsecutive(


withConsecutive and willReturnOnConsecutiveCalls are deprecated in the newer versions of PHPUnit. Please consider changing those to returnValueMap here as it would be needed on merge-up anyway. For example, you can refer to https://github.com/ibexa/product-catalog/pull/1034/files#diff-df82e5aba4361380977da9b3843954e8614ebbb83c317a6bd45d4b53c0fe55f2R95.

konradoboza · 2024-01-04T15:04:06Z

eZ/Publish/Core/Persistence/Cache/TrashHandler.php

@@ -51,13 +55,23 @@ public function trashSubtree($locationId)
 }, $reverseRelations);
 }

+ $versionTags = array_map(function (VersionInfo $versionInfo) use ($contentId): string {
+ return $this->cacheIdentifierGenerator->generateTag(


Suggested change

return $this->cacheIdentifierGenerator->generateTag(

return $this->cacheIdentifierGenerator->generateTag(

konradoboza · 2024-01-04T15:04:56Z

eZ/Publish/Core/Persistence/Cache/TrashHandler.php

+ $reverseRelations = $this->persistenceHandler->contentHandler()->loadRelations($contentId);
+ $versions = $this->persistenceHandler->contentHandler()->listVersions($contentId);


Suggested change

$reverseRelations = $this->persistenceHandler->contentHandler()->loadRelations($contentId);

$versions = $this->persistenceHandler->contentHandler()->listVersions($contentId);

$contentHandler = $this->persistenceHandler->contentHandler();

$reverseRelations = $contentHandler->loadRelations($contentId);

$versions = $contentHandler->listVersions($contentId);

mateuszbieniek · 2024-01-04T15:15:53Z

My tests show, that one of the biggest culprits here is lp- 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.

Are you able to tell what performance losses are we observing here?

To be frank no, but looking at it this is a simple tag. Due to the way how caching works - worst-case scenario it will get recreated the next time it is required, and as it is lp - Location Parent (?) - I would not expect anything spectacular, especially - judging from the public issue description - you have to empty trash for this to get orphaned - I won't expect that this will be ever again requested after that.

In addition, freeing this memory to be reused should - especially in the long run - benefit to overall application performance (more memory, less evictions).

Added additional tag cleaning to limit down number of orphaned tag en…

c82c9b7

…tries

mateuszbieniek added the Bug Something isn't working label May 9, 2023

mateuszbieniek requested a review from a team May 9, 2023 14:25

github-actions bot assigned alongosz, adamwojs, ciastektk, mikadamczyk and Steveb-p May 9, 2023

CS fixed

4b7e56c

konradoboza requested changes May 10, 2023

View reviewed changes

konradoboza requested a review from a team May 10, 2023 09:51

alongosz changed the title ~~IBX-5502 - Added additional tag cleaning to limit down number of orphaned tag entries~~ IBX-5502: Added additional tag cleaning to limit down number of orphaned tag entries Jul 28, 2023

Fixed test + changes after CR

fc20177

github-actions bot assigned kisztof Aug 10, 2023

mateuszbieniek requested a review from konradoboza August 10, 2023 11:48

Fixed the last test

0bc8d92

alongosz self-requested a review October 10, 2023 07:49

kisztof suggested changes Jan 4, 2024

View reviewed changes

mateuszbieniek and others added 2 commits January 4, 2024 11:18

Update eZ/Publish/Core/Persistence/Cache/ContentHandler.php

82d6a7d

Co-authored-by: Konrad Oboza <[email protected]>

Update eZ/Publish/Core/Persistence/Cache/ContentHandler.php

9aa1131

Co-authored-by: Konrad Oboza <[email protected]>

mateuszbieniek requested a review from kisztof January 4, 2024 10:18

konradoboza reviewed Jan 4, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

IBX-5502: Added additional tag cleaning to limit down number of orphaned tag entries #372

IBX-5502: Added additional tag cleaning to limit down number of orphaned tag entries #372

mateuszbieniek commented May 9, 2023 •

edited

Loading

sonarcloud bot commented May 9, 2023

konradoboza left a comment

konradoboza May 10, 2023

mateuszbieniek commented Aug 10, 2023

sonarcloud bot commented Aug 10, 2023

kisztof left a comment

mateuszbieniek commented Jan 4, 2024

sonarcloud bot commented Jan 4, 2024

konradoboza left a comment

konradoboza Jan 4, 2024 •

edited

Loading

mateuszbieniek Jan 4, 2024 •

edited

Loading

konradoboza Jan 4, 2024

konradoboza Jan 4, 2024

konradoboza Jan 4, 2024

konradoboza Jan 4, 2024

mateuszbieniek commented Jan 4, 2024

	$locationTags = array_map(function (Content\Location $location) {
	$locationTags = array_map(function (Content\Location $location): string {

	public function testUpdateContent()
	public function testUpdateContent(): void

	return $this->cacheIdentifierGenerator->generateTag(
	return $this->cacheIdentifierGenerator->generateTag(

		$reverseRelations = $this->persistenceHandler->contentHandler()->loadRelations($contentId);
		$versions = $this->persistenceHandler->contentHandler()->listVersions($contentId);

- $reverseRelations = $this->persistenceHandler->contentHandler()->loadRelations($contentId);
- $versions = $this->persistenceHandler->contentHandler()->listVersions($contentId);
+ $contentHandler = $this->persistenceHandler->contentHandler();
+ $reverseRelations = $contentHandler->loadRelations($contentId);
+ $versions = $contentHandler->listVersions($contentId);

IBX-5502: Added additional tag cleaning to limit down number of orphaned tag entries #372

Are you sure you want to change the base?

IBX-5502: Added additional tag cleaning to limit down number of orphaned tag entries #372

Conversation

mateuszbieniek commented May 9, 2023 • edited Loading

Checklist:

sonarcloud bot commented May 9, 2023

konradoboza left a comment

Choose a reason for hiding this comment

konradoboza May 10, 2023

Choose a reason for hiding this comment

mateuszbieniek commented Aug 10, 2023

sonarcloud bot commented Aug 10, 2023

kisztof left a comment

Choose a reason for hiding this comment

mateuszbieniek commented Jan 4, 2024

sonarcloud bot commented Jan 4, 2024

Quality Gate passed

konradoboza left a comment

Choose a reason for hiding this comment

konradoboza Jan 4, 2024 • edited Loading

Choose a reason for hiding this comment

mateuszbieniek Jan 4, 2024 • edited Loading

Choose a reason for hiding this comment

konradoboza Jan 4, 2024

Choose a reason for hiding this comment

konradoboza Jan 4, 2024

Choose a reason for hiding this comment

konradoboza Jan 4, 2024

Choose a reason for hiding this comment

konradoboza Jan 4, 2024

Choose a reason for hiding this comment

mateuszbieniek commented Jan 4, 2024

mateuszbieniek commented May 9, 2023 •

edited

Loading

konradoboza Jan 4, 2024 •

edited

Loading

mateuszbieniek Jan 4, 2024 •

edited

Loading