redis: use atomic operations everywhere #37758

pulsejet · 2023-04-16T20:38:01Z

This removes a lot of acrobatics in the code and does each operation
atomically using a lua script. This also reduces several round trips
to the server, and the scripts are compiled and cached server-side.

Notably, since all operations work only on a single key (except clear,
which is broken anyway and shouldn't be used), they will continue to
function and be atomic for Redis cluster.

EDIT: I did some simple benchmark with a single client process. Looks like locking is approximately 2x faster (probably even better with multiple clients). This is with Redis on the same host, so gains will be better when latency is higher between the servers.

<?php
        $lock = \OC::$server->get(\OCP\Lock\ILockingProvider::class);

        $t1 = microtime(true);
        for($i = 0; $i < 20000; $i++) {
            $lock->acquireLock('benchmark', \OCP\Lock\ILockingProvider::LOCK_EXCLUSIVE);
            $lock->releaseLock('benchmark', \OCP\Lock\ILockingProvider::LOCK_EXCLUSIVE);
        }
        $t2 = microtime(true);
        $output->writeln("Total: " . ($t2 - $t1) . "s");

Before: 5.590616941452s
After: 2.6423881053925s

On a side note, using DB for locking is a joke:

MySQL: 232.49977707863s

pulsejet · 2023-04-16T20:49:27Z

lib/private/Memcache/Redis.php

@@ -54,18 +54,19 @@ public function getCache() {

 	public function get($key) {
 		$result = $this->getCache()->get($this->getPrefix() . $key);
-		if ($result === false && !$this->getCache()->exists($this->getPrefix() . $key)) {


This check is useless. Redis does not support boolean types, so getting a false means it did not exist.

pulsejet · 2023-04-16T20:50:04Z

lib/private/Memcache/Redis.php

@@ -82,6 +83,7 @@ public function remove($key) {
 	}

 	public function clear($prefix = '') {
+		// TODO: this is slow and would fail with Redis cluster
 		$prefix = $this->getPrefix() . $prefix . '*';
 		$keys = $this->getCache()->keys($prefix);
 		$deleted = $this->getCache()->del($keys);


This will fail for redis cluster and a large number of keys. The solution is to replace this with a lua script and run it on all masters.

pulsejet · 2023-04-16T20:51:12Z

lib/private/Memcache/Redis.php

-				->exec();
-			return $result !== false;
-		}
-		$this->getCache()->unwatch();


5 statements here vs 1 now. Ditto for cad.

pulsejet · 2023-04-16T20:52:48Z

lib/private/Memcache/Redis.php

+
+		return $this->getCache()->eval(
+			'if redis.call("get", KEYS[1]) == ARGV[1] then
+				return redis.call("del", KEYS[1])


Using unlink here is a premature optimization that's probably harmful. In fact, unless the values are huge, unlink internally calls del after checking the size, so we just did those checks for no reason...

This removes a lot of acrobatics in the code and does each operation atomically using a lua script. This also reduces several round trips to the server, and the scripts are compiled and cached server-side. Notably, since all operations work only on a single key (except clear, which is broken anyway and shouldn't be used), they will continue to function and be atomic for Redis cluster. Signed-off-by: Varun Patil <varunpatil@ucla.edu>

Signed-off-by: Robin Appelman <robin@icewind.nl>

icewind1991 · 2023-04-17T14:20:23Z

Added a unit test to validate the script hashes to make sure we don't forget to update them if we ever change the scripts

pulsejet · 2023-04-17T15:58:26Z

Added a unit test to validate the script hashes to make sure we don't forget to update them if we ever change the scripts

Good point, thanks!

pulsejet · 2023-05-08T04:59:02Z

Bump?

CarlSchwan

This is pretty neat. I didn't knew that redis allowed to run lua script

tests/lib/Memcache/RedisTest.php

lib/private/Memcache/Redis.php

juliushaertl

Minor comments, otherwise this seems like a nice improvement.

Is there any minimum version requirement for this on the redis side?

pulsejet · 2023-05-10T06:34:28Z

Minor comments, otherwise this seems like a nice improvement.

Thanks, will fix these tomorrow.

Is there any minimum version requirement for this on the redis side?

Redis 2.6, released ~10 years ago.

Signed-off-by: Varun Patil <varunpatil@ucla.edu>

pulsejet · 2023-05-16T18:55:24Z

CI failure unrelated

juliushaertl

Let's get this in for 28 👍

ChristophWurst · 2023-05-30T15:13:49Z

Massive kudos for this patch @pulsejet. We ran into a production instance with incomprehensive file locking issues with a Redis cluster memcache backend. Sporadically unlocked files were still locked inside a single request. Only with this patch we could eliminate the issue.

@AndyScherzinger this is not only a performance improvement but fixes Redis cluster inconsistencies. Would this change be ok to backport?

pulsejet · 2023-05-30T18:14:35Z

Great! A possible reason I see is that Redis::dec wasn't atomic earlier, maybe that led to a race. But transaction support with cluster is iffy to begin with anyway.

AndyScherzinger · 2023-05-31T05:49:54Z

@ChristophWurst yes, backporting would be fine from my pov 👍

ChristophWurst · 2023-05-31T06:18:31Z

/backport to stable27

ChristophWurst · 2023-05-31T06:18:36Z

/backport to stable26

solracsf · 2023-05-31T07:35:32Z

@ChristophWurst any specific reason why not backporting to 25 also?

ChristophWurst · 2023-05-31T07:55:38Z

Wrongly got the impression 25 is EOL but it's only 24

ChristophWurst · 2023-05-31T07:55:58Z

I think I'll backport the 26 backport because there are conflicts to resolve

ChristophWurst · 2023-05-31T10:05:38Z

#38539 (comment)

pulsejet added 3. to review Waiting for reviews feature: caching Related to our caching system: scssCacher, jsCombiner... performance 🚀 labels Apr 16, 2023

pulsejet requested review from juliushaertl, skjnldsv, CarlSchwan and szaimen April 16, 2023 20:38

szaimen added this to the Nextcloud 27 milestone Apr 16, 2023

szaimen requested review from a team, ArtificialOwl and icewind1991 and removed request for szaimen and a team April 16, 2023 20:39

pulsejet force-pushed the redis-atomic branch from c3f70d7 to b7332ae Compare April 16, 2023 20:45

pulsejet commented Apr 16, 2023

View reviewed changes

pulsejet force-pushed the redis-atomic branch 2 times, most recently from 3671f8d to 39e805f Compare April 17, 2023 03:19

add test to verify redis lua script hashes

0375c98

Signed-off-by: Robin Appelman <robin@icewind.nl>

icewind1991 approved these changes Apr 17, 2023

View reviewed changes

skjnldsv mentioned this pull request May 3, 2023

27.0.0 beta 1 #38038

Merged

szaimen requested a review from a team May 5, 2023 11:00

CarlSchwan approved these changes May 8, 2023

View reviewed changes

CarlSchwan reviewed May 8, 2023

View reviewed changes

tests/lib/Memcache/RedisTest.php Outdated Show resolved Hide resolved

skjnldsv added this to the Nextcloud 28 milestone May 9, 2023

pulsejet requested a review from CarlSchwan May 9, 2023 21:26

juliushaertl reviewed May 10, 2023

View reviewed changes

lib/private/Memcache/Redis.php Outdated Show resolved Hide resolved

juliushaertl reviewed May 10, 2023

View reviewed changes

lib/private/Memcache/Redis.php Outdated Show resolved Hide resolved

juliushaertl reviewed May 10, 2023

View reviewed changes

lib/private/Memcache/Redis.php Outdated Show resolved Hide resolved

juliushaertl reviewed May 10, 2023

View reviewed changes

redis: move lua scripts to class and add type hints

c6cee28

Signed-off-by: Varun Patil <varunpatil@ucla.edu>

pulsejet force-pushed the redis-atomic branch from f606228 to c6cee28 Compare May 11, 2023 00:37

pulsejet requested a review from juliushaertl May 11, 2023 00:38

pulsejet enabled auto-merge May 16, 2023 18:53

juliushaertl approved these changes May 17, 2023

View reviewed changes

szaimen disabled auto-merge May 19, 2023 21:29

szaimen merged commit 3d2d1c1 into master May 19, 2023

szaimen deleted the redis-atomic branch May 19, 2023 21:29

backportbot-nextcloud bot added the backport-request label May 31, 2023

This was referenced May 31, 2023

[stable27] redis: use atomic operations everywhere #38538

Merged

[stable26] redis: use atomic operations everywhere #38539

Merged

backportbot-nextcloud bot removed the backport-request label May 31, 2023

kesselb mentioned this pull request Jun 13, 2023

[Bug]: APCu support gone in 27.0.0 #38796

Closed

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

redis: use atomic operations everywhere #37758

redis: use atomic operations everywhere #37758

pulsejet commented Apr 16, 2023 •

edited

Loading

pulsejet Apr 16, 2023

pulsejet Apr 16, 2023

pulsejet Apr 16, 2023

pulsejet Apr 16, 2023

icewind1991 commented Apr 17, 2023

pulsejet commented Apr 17, 2023

pulsejet commented May 8, 2023

CarlSchwan left a comment

juliushaertl left a comment

pulsejet commented May 10, 2023

pulsejet commented May 16, 2023

juliushaertl left a comment

ChristophWurst commented May 30, 2023

pulsejet commented May 30, 2023

AndyScherzinger commented May 31, 2023

ChristophWurst commented May 31, 2023

ChristophWurst commented May 31, 2023

solracsf commented May 31, 2023

ChristophWurst commented May 31, 2023

ChristophWurst commented May 31, 2023

ChristophWurst commented May 31, 2023

redis: use atomic operations everywhere #37758

redis: use atomic operations everywhere #37758

Conversation

pulsejet commented Apr 16, 2023 • edited Loading

pulsejet Apr 16, 2023

Choose a reason for hiding this comment

pulsejet Apr 16, 2023

Choose a reason for hiding this comment

pulsejet Apr 16, 2023

Choose a reason for hiding this comment

pulsejet Apr 16, 2023

Choose a reason for hiding this comment

icewind1991 commented Apr 17, 2023

pulsejet commented Apr 17, 2023

pulsejet commented May 8, 2023

CarlSchwan left a comment

Choose a reason for hiding this comment

juliushaertl left a comment

Choose a reason for hiding this comment

pulsejet commented May 10, 2023

pulsejet commented May 16, 2023

juliushaertl left a comment

Choose a reason for hiding this comment

ChristophWurst commented May 30, 2023

pulsejet commented May 30, 2023

AndyScherzinger commented May 31, 2023

ChristophWurst commented May 31, 2023

ChristophWurst commented May 31, 2023

solracsf commented May 31, 2023

ChristophWurst commented May 31, 2023

ChristophWurst commented May 31, 2023

ChristophWurst commented May 31, 2023

pulsejet commented Apr 16, 2023 •

edited

Loading