FEATURE: Optimised Redis cache backend #2721

Sebobo · 2022-03-02T13:01:58Z

What I did

With this change the flushByTags is optimised to run flush operations in batches.
The maximum batch size can and should be configured based on the used data source.

This change relies on the interface changes in #2718

Additionally all optimisations from https://github.com/sandstorm/OptimizedRedisCacheBackend were added.

How I did it

Instead of uploading the flush Lua script and only flushing the entries for one tag.
The same is now done for batches of tags reducing the calls to Redis and the
evaluation time Redis needs to parse the script.

How to verify it

#2718 includes updated tests for the Redis backend that will also check this method.

Checklist

Code follows the PSR-2 coding style
Tests have been created, run and adjusted as needed -> see !!! FEATURE: Introduce flushByTags to taggable cache backends #2718
The PR is created against the lowest maintained branch

kitsunet · 2022-03-03T07:23:21Z

Look neat! Thank you, does this superseed the sandstorm opt or does that still have any extras then?

Sebobo · 2022-03-03T08:06:04Z

@kitsunet thanks.

I only added the main new method in this PR, it doesn't cover all the other improvements they did. But the script is already based on the Sandstorm version of the single tag flush. I had a call yesterday with @skurfuerst and we also discussed to now bring over all improvements from the Sandstorm version.

I have a few more calls planned with people who modified the Redis backend to get their input too.

I would then put all those into a separate PR where everyone with improvements then can chime in.

With this change the `flushByTags` is optimised to run in batches. The maximum batch size can and should be configured based on the used data source. This change relies on the interface changes in neos#2718

WIth this change optimisations from the community package sandstorm/optimized-redis-backend are introduced into the core Redis backend.

Sebobo · 2022-03-18T22:32:36Z

@kitsunet I now added in another commit the remaining optimisations from the sandstorm package.
As my flushByTags is already based on their script and not on the flushByTag of the core version, I think it doesn't make sense to only optimise this one method and have it behave a bit different.

If the change should be "clean" I would need to make the flushByTags based on the old script.

Wdyt? Should I make a "clean" PR and then another for the other optimisations?
I mean the sandstorm version is running in a lot of projects afaik.

If we go this way, I would of course adjust the PR title and description.

kdambekalns · 2022-03-23T13:50:58Z

Should I make a "clean" PR and then another for the other optimisations?

I remember faintly there were issues with that implementation when not used in a "strictly Neos" context, as it had cut some corners to optimize. Not sure if those are still existing, I'll try to research a bit more later today.

kdambekalns

Ok, I think the only potential issue with the OptimizedRedisCacheBackend left is the documented behaviour around leaving stale tags in place and the effect that could have on findIdentifiersBytag(). So if that is "copied"

Another thing I noticed: In the README of OptimizedRedisCacheBackend it says

For implementing flush(), we just iterate over the relevant part of the keyspace and remove all elements. Before, this was done through a similar logic as explained in FlushByTag, because the Redis Backend was freezable (but we do not need this for Neos Content Cache).

So, if this is case, implementing FreezableBackendInterface should be removed from the backend, no?

jonnitto · 2022-03-24T06:44:43Z

Trigger checks

Sebobo · 2022-03-24T08:53:01Z

Ok, I think the only potential issue with the OptimizedRedisCacheBackend left is the documented behaviour around leaving stale tags in place and the effect that could have on findIdentifiersBytag(). So if that is "copied"

I will test the difference related to performance when keeping this part.

Another thing I noticed: In the README of OptimizedRedisCacheBackend it says

For implementing flush(), we just iterate over the relevant part of the keyspace and remove all elements. Before, this was done through a similar logic as explained in FlushByTag, because the Redis Backend was freezable (but we do not need this for Neos Content Cache).

So, if this is case, implementing FreezableBackendInterface should be removed from the backend, no?

But the backend is not only for Neos? So I would leave it in there. Shouldn't hurt to keep it. Doesn't look like I should have a big impact. But will test.

What do you think about increasing the minimum Redis version to the 6.0.0 which is the oldest with EOL?
Then we could also finally add some more advanced features in the nearer future than will also help with performance.

kdambekalns · 2022-03-24T09:09:18Z

I fixed the Psalm complaint, but the test failures look legit…

kdambekalns · 2022-03-24T09:13:13Z

So, if this is case, implementing FreezableBackendInterface should be removed from the backend, no?

But the backend is not only for Neos? So I would leave it in there. Shouldn't hurt to keep it.

That's my point - if it's kept, it should behave like before, but I understand the README says it differs… So either remove freezability or keep behaviour.

What do you think about increasing the minimum Redis version to the 6.0.0 which is the oldest with EOL? Then we could also finally add some more advanced features in the nearer future than will also help with performance.

Fine with me…

Sebobo · 2022-03-24T09:15:44Z

Will check

All Redis versions < 6.0.0 are already EOL. This would allow us to improve the backends behaviour by using more recent features of Redis.

This prevents getting non existent identifiers when calling `findIdentifiersByTag`.

Due to the removal of the `entries` cache entry to track all entries, the iteration must now be done by retrieving all matching keys from the cache.

Sebobo · 2022-03-24T14:03:59Z

Ok a unit test fails now, will check again...

Sebobo · 2022-03-24T15:06:23Z

Everything green, hope I didn't miss anything. But at least I now know 1000% more about Redis than before 😎

Neos.Cache/Classes/Backend/RedisBackend.php

kdambekalns

Two more tiny suggestions… and the class docblock still says:

 * Since Redis < 2.8.0 does not provide a mechanism for iterating over keys,
 * a separate list with all entries is populated

That is not true anymore?! At least the version mentioned is no longer supported, so is there a better way?

Neos.Cache/Classes/Backend/RedisBackend.php

kdambekalns

Notwithstanding my tiny remarks, this seems to work just fine… I only did limited testing (compared to what @Sebobo did), but… ✅

Sebobo · 2022-03-24T19:47:26Z

Two more tiny suggestions… and the class docblock still says:
 * Since Redis < 2.8.0 does not provide a mechanism for iterating over keys,
 * a separate list with all entries is populated
That is not true anymore?! At least the version mentioned is no longer supported, so is there a better way?

You are right, that was my last change to clean that up after I had read it, but forgot to remove the text.
After working on this stuff for hours I'm half braindead 🧟‍♂️

mficzel · 2022-03-24T21:17:06Z

After the updated docblocks i get "Typed property Neos\Cache\Backend\RedisBackend::$frozen must not be accessed before initialization"

Does this rely on features from #2701

Neos.Cache/Classes/Backend/RedisBackend.php

Co-authored-by: Marc Henry Schultz <85400359+mhsdesign@users.noreply.github.com>

kdambekalns · 2022-03-24T22:29:00Z

Neos.Cache/Classes/Backend/RedisBackend.php

-     * @var integer Cursor used for iterating over cache entries
-     */
-    protected $entryCursor = 0;
+    protected ?bool $frozen = null;


Suggested change

protected ?bool $frozen = null;

protected bool $frozen = false;

since the getter declares its return type as bool that would conflict anyway.

I assume it was done like that because when it's null the getter retrieves the value from Redis and "caches" it this way.

albe

Don't know much about redis and I can't process the lua scripts in a way to decide if they are sane or not. So I just left three suggestions to use union false pseudo type instead of the more generic bool: https://www.php.net/manual/de/language.types.declarations.php#language.types.declarations.composite.union.false

albe · 2022-03-25T10:58:32Z

Neos.Cache/Classes/Backend/RedisBackend.php

    }

    /**
     * {@inheritdoc}
     */
-    public function key()
+    public function key(): string|bool


Suggested change

public function key(): string|bool

public function key(): string|false

albe · 2022-03-25T11:05:20Z

Neos.Cache/Classes/Backend/RedisBackend.php

     * @api
     */
-    public function get(string $entryIdentifier)
+    public function get(string $entryIdentifier): string|bool


Suggested change

public function get(string $entryIdentifier): string|bool

public function get(string $entryIdentifier): string|false

albe · 2022-03-25T11:06:48Z

Neos.Cache/Classes/Backend/RedisBackend.php

@@ -310,28 +355,30 @@ public function findIdentifiersByTag(string $tag): array
    /**
     * {@inheritdoc}
     */
-    public function current()
+    public function current(): string|bool


Suggested change

public function current(): string|bool

public function current(): string|false

mficzel

Looks good to me and works on my machine. Am no redis expert

skurfuerst · 2022-03-27T06:04:04Z

awesome work everybody ❤️ ❤️ ❤️

I'll add docs to OptimizedRedisCacheBackend, marking it as obsolete starting with Neos 8.0 💯

skurfuerst · 2022-03-27T06:08:38Z

Neos.Cache/Classes/Backend/RedisBackend.php

+            redis.call('DEL', key)
+        end
+        ";
+        $this->redis->eval($script, [$this->getPrefixedIdentifier('frozen'), $this->getPrefixedIdentifier('')], 1);


$this->getPrefixedIdentifier('frozen') is not needed anymore, is it? At least KEYS[1] is never accessed.

It is still needed, but properly the script is incorrect now. Will check and adjust the test. Thx for noticing!

Ok, you are right, the key is already included in the args with wildcard.

Sebobo · 2022-03-28T08:00:31Z

awesome work everybody ❤️ ❤️ ❤️

I'll add docs to OptimizedRedisCacheBackend, marking it as obsolete starting with Neos 8.0 💯

Hi @skurfuerst, thx.

There is still a difference to your package as I enabled the removal of related tags again, which you didn't do in the "optimised" version.
It might not be a problem as long as flushByTags is used, as its so much faster, but there was a reason for the removal I guess. But you should only mark it obsolete if it behaves same or better in production related to the tag removal behaviour.

There might still be a need for a Neos Content Cache specific Redis backend implementation that does some things a bit differently than the general one.

bwaidelich · 2023-03-21T11:55:46Z

This change made it into neos/flow 7.3.8 but it seems to be missing from 7.3.9

kdambekalns · 2023-03-21T12:26:10Z

Hm.

The diff doesn't confirm: 7.3.8...7.3.9
Same in the split: neos/flow@7.3.8...7.3.9

The 7.3.8 version was tagged on the 8.2 branch AFAICT.

kdambekalns · 2023-03-21T12:30:59Z

And 7.3.7 was tagged in HEAD off of 7.3 and later merged back into 7.3 (which is confusing, but correct.)

Sebobo added the P: Cache label Mar 2, 2022

Sebobo self-assigned this Mar 2, 2022

Sebobo added 3 commits March 18, 2022 23:27

FEATURE: flushByTags for the Redis cache backend

f13ec48

With this change the `flushByTags` is optimised to run in batches. The maximum batch size can and should be configured based on the used data source. This change relies on the interface changes in neos#2718

TASK: Adjust code style for redis cache backend

4f9012d

FEATURE: Redis cache backend optimisations

f9dd34f

WIth this change optimisations from the community package sandstorm/optimized-redis-backend are introduced into the core Redis backend.

Sebobo force-pushed the feature/flush-by-tags-redis branch from 4c03485 to f9dd34f Compare March 18, 2022 22:27

Sebobo changed the title ~~FEATURE: flushByTags for the Redis cache backend~~ FEATURE: Optimised Redis cache backend Mar 21, 2022

Sebobo marked this pull request as ready for review March 21, 2022 08:22

kdambekalns reviewed Mar 23, 2022

View reviewed changes

Merge branch 'master' into feature/flush-by-tags-redis

69dbec5

jonnitto closed this Mar 24, 2022

jonnitto reopened this Mar 24, 2022

TASK: Fix a semi-valid Psalm complaint

0c795d1

kdambekalns requested review from daniellienert, albe and robertlemke March 24, 2022 09:14

Sebobo added 5 commits March 24, 2022 14:47

!!! TASK: Increase minimum Redis version to 6.0.0

e47ab10

All Redis versions < 6.0.0 are already EOL. This would allow us to improve the backends behaviour by using more recent features of Redis.

TASK: Reformat LUA scripts

fceab2e

BUGFIX: Delete related tags during flush

70df7c7

This prevents getting non existent identifiers when calling `findIdentifiersByTag`.

BUGFIX: Make sure Redis cache is unfrozen on flush

b3c378c

BUGFIX: Allow iteration over Redis cache entries again

c64546a

Due to the removal of the `entries` cache entry to track all entries, the iteration must now be done by retrieving all matching keys from the cache.

BUGFIX: Fix failing Redis unit test

98a04bd

Sebobo closed this Mar 24, 2022

Sebobo reopened this Mar 24, 2022

BUGFIX: Solve psalm issues

fb1935b

mhsdesign reviewed Mar 24, 2022

View reviewed changes

Neos.Cache/Classes/Backend/RedisBackend.php Show resolved Hide resolved

mhsdesign reviewed Mar 24, 2022

View reviewed changes

Neos.Cache/Classes/Backend/RedisBackend.php Show resolved Hide resolved

kdambekalns reviewed Mar 24, 2022

View reviewed changes

Neos.Cache/Classes/Backend/RedisBackend.php Show resolved Hide resolved

Neos.Cache/Classes/Backend/RedisBackend.php Show resolved Hide resolved

kdambekalns approved these changes Mar 24, 2022

View reviewed changes

Sebobo added 2 commits March 24, 2022 21:14

TASK: Update docblocks

a14bb85

TASK: Fix Psalm error

6ad44bc

Sebobo force-pushed the feature/flush-by-tags-redis branch from efa53e7 to 6ad44bc Compare March 24, 2022 20:14

Sebobo closed this Mar 24, 2022

Sebobo reopened this Mar 24, 2022

mhsdesign reviewed Mar 24, 2022

View reviewed changes

Neos.Cache/Classes/Backend/RedisBackend.php Outdated Show resolved Hide resolved

TASK: Resolve Psalm error

c0c4970

Co-authored-by: Marc Henry Schultz <85400359+mhsdesign@users.noreply.github.com>

kdambekalns reviewed Mar 24, 2022

View reviewed changes

albe reviewed Mar 25, 2022

View reviewed changes

mficzel approved these changes Mar 25, 2022

View reviewed changes

mficzel merged commit 05e57cc into neos:master Mar 25, 2022

Sebobo deleted the feature/flush-by-tags-redis branch March 25, 2022 12:47

skurfuerst reviewed Mar 27, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FEATURE: Optimised Redis cache backend #2721

FEATURE: Optimised Redis cache backend #2721

Sebobo commented Mar 2, 2022 •

edited

Loading

kitsunet commented Mar 3, 2022

Sebobo commented Mar 3, 2022

Sebobo commented Mar 18, 2022

kdambekalns commented Mar 23, 2022

kdambekalns left a comment

jonnitto commented Mar 24, 2022

Sebobo commented Mar 24, 2022

kdambekalns commented Mar 24, 2022

kdambekalns commented Mar 24, 2022 •

edited

Loading

Sebobo commented Mar 24, 2022

Sebobo commented Mar 24, 2022

Sebobo commented Mar 24, 2022

kdambekalns left a comment •

edited

Loading

kdambekalns left a comment

Sebobo commented Mar 24, 2022

mficzel commented Mar 24, 2022

kdambekalns Mar 24, 2022

Sebobo Mar 24, 2022

albe left a comment

albe Mar 25, 2022

albe Mar 25, 2022

albe Mar 25, 2022

mficzel left a comment

skurfuerst commented Mar 27, 2022

skurfuerst Mar 27, 2022

Sebobo Mar 28, 2022

Sebobo Mar 28, 2022

Sebobo commented Mar 28, 2022

bwaidelich commented Mar 21, 2023

kdambekalns commented Mar 21, 2023 •

edited

Loading

kdambekalns commented Mar 21, 2023

	protected ?bool $frozen = null;
	protected bool $frozen = false;

	public function key(): string\|bool
	public function key(): string\|false

	public function get(string $entryIdentifier): string\|bool
	public function get(string $entryIdentifier): string\|false

	public function current(): string\|bool
	public function current(): string\|false

FEATURE: Optimised Redis cache backend #2721

FEATURE: Optimised Redis cache backend #2721

Conversation

Sebobo commented Mar 2, 2022 • edited Loading

kitsunet commented Mar 3, 2022

Sebobo commented Mar 3, 2022

Sebobo commented Mar 18, 2022

kdambekalns commented Mar 23, 2022

kdambekalns left a comment

Choose a reason for hiding this comment

jonnitto commented Mar 24, 2022

Sebobo commented Mar 24, 2022

kdambekalns commented Mar 24, 2022

kdambekalns commented Mar 24, 2022 • edited Loading

Sebobo commented Mar 24, 2022

Sebobo commented Mar 24, 2022

Sebobo commented Mar 24, 2022

kdambekalns left a comment • edited Loading

Choose a reason for hiding this comment

kdambekalns left a comment

Choose a reason for hiding this comment

Sebobo commented Mar 24, 2022

mficzel commented Mar 24, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

albe left a 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

mficzel left a comment

Choose a reason for hiding this comment

skurfuerst commented Mar 27, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sebobo commented Mar 28, 2022

bwaidelich commented Mar 21, 2023

kdambekalns commented Mar 21, 2023 • edited Loading

kdambekalns commented Mar 21, 2023

Sebobo commented Mar 2, 2022 •

edited

Loading

kdambekalns commented Mar 24, 2022 •

edited

Loading

kdambekalns left a comment •

edited

Loading

kdambekalns commented Mar 21, 2023 •

edited

Loading