From fd6b14286ac367999da17649d05820a944fe29f6 Mon Sep 17 00:00:00 2001 From: Benjamin Walker Date: Fri, 2 May 2025 18:37:45 +1000 Subject: [PATCH 1/2] MDL-82511 cache: Improve performance of cleanup in cache_cron_task --- cache/classes/helper.php | 52 +++++++++++++++++++-- cache/tests/cache_helper_test.php | 77 +++++++++++++++++++++++++++++++ 2 files changed, 124 insertions(+), 5 deletions(-) diff --git a/cache/classes/helper.php b/cache/classes/helper.php index 5a07028765d..9a66d347025 100644 --- a/cache/classes/helper.php +++ b/cache/classes/helper.php @@ -756,10 +756,16 @@ class helper { debugging('Cache stores used for session definitions should ideally be searchable.', DEBUG_DEVELOPER); continue; } + + // Load all of the keys into memory so we can compare against multiple prefixes. + $keys = $store->find_all(); + sort($keys); + // Get all of the last access keys. - $keys = $store->find_by_prefix(session_cache::LASTACCESS); + $lastaccess = array_filter($keys, fn($key) => str_starts_with($key, session_cache::LASTACCESS)); $todelete = []; - foreach ($store->get_many($keys) as $key => $value) { + $prefixtodelete = []; + foreach ($store->get_many($lastaccess) as $key => $value) { $expiresvalue = 0; if ($value instanceof ttl_wrapper) { $expiresvalue = $value->data; @@ -771,11 +777,16 @@ class helper { $expires = (int) $expiresvalue; if ($expires > 0 && $expires < $purgetime) { - $prefix = substr($key, strlen(session_cache::LASTACCESS)); - $foundbyprefix = $store->find_by_prefix($prefix); - $todelete = array_merge($todelete, [$key], $foundbyprefix); + $todelete[] = $key; + $prefixtodelete[] = substr($key, strlen(session_cache::LASTACCESS)); } } + + // Match all of the prefixes to delete to keys to delete. + if ($prefixtodelete) { + sort($prefixtodelete); + $todelete = array_merge($todelete, self::filter_sorted_keys_by_prefixes($keys, $prefixtodelete)); + } if ($todelete) { $outcome = (int) $store->delete_many($todelete); if ($output) { @@ -788,6 +799,37 @@ class helper { } } + /** + * Filters a sorted list of keys by a sorted list of prefixes. + * This relies on the sorting to reduce the number of comparisons. + * + * @param array $keys a sorted array of keys + * @param array $prefixes a sorted array of prefixes + * @return array of keys containing any of the prefixes + */ + public static function filter_sorted_keys_by_prefixes(array $keys, array $prefixes): array { + // Reverse the prefixes to allow for simple processing. + $prefixes = array_reverse($prefixes); + + $matches = []; + $prefix = array_pop($prefixes); + foreach ($keys as $key) { + // The keys and prefixes are sorted so we only need to compare against one prefix at a time. + // This is done inside a loop to check the next prefix at break points. + while ($prefix) { + if (str_starts_with($key, $prefix)) { + $matches[] = $key; + } else if ($prefix < $key) { + // The key has moved past the current prefix alphabetically, so check the next. + $prefix = array_pop($prefixes); + continue; + } + break; + } + } + return $matches; + } + /** * Returns an array of stores that would meet the requirements for every definition. * diff --git a/cache/tests/cache_helper_test.php b/cache/tests/cache_helper_test.php index 135e3bf9494..5d36e3416d7 100644 --- a/cache/tests/cache_helper_test.php +++ b/cache/tests/cache_helper_test.php @@ -60,4 +60,81 @@ final class cache_helper_test extends \advanced_testcase { [new \stdClass(), true], ]; } + + /** + * Test the filter_sorted_keys_by_prefixes method. + * + * @param array $keys + * @param array $prefixes + * @param array $expected + * @dataProvider filter_sorted_keys_by_prefixes_provider + */ + public function test_filter_sorted_keys_by_prefixes(array $keys, array $prefixes, array $expected): void { + $this->assertEquals($expected, helper::filter_sorted_keys_by_prefixes($keys, $prefixes)); + } + + /** + * Data provider for filter_sorted_keys_by_prefixes tests. + * + * @return array + */ + public static function filter_sorted_keys_by_prefixes_provider(): array { + return [ + 'simple match' => [ + 'keys' => ['aa', 'ab', 'ba', 'bb'], + 'prefixes' => ['a'], + 'expected' => ['aa', 'ab'], + ], + 'multiple prefixes match' => [ + 'keys' => ['aa', 'ab', 'ba', 'bb', 'ca', 'cb'], + 'prefixes' => ['a', 'c'], + 'expected' => ['aa', 'ab', 'ca', 'cb'], + ], + 'consecutive prefixes match' => [ + 'keys' => ['aa', 'ab', 'ba', 'bb', 'ca', 'cb'], + 'prefixes' => ['a', 'b'], + 'expected' => ['aa', 'ab', 'ba', 'bb'], + ], + 'overlapping prefixes' => [ + 'keys' => ['a', 'ab', 'abc', 'abcd'], + 'prefixes' => ['ab', 'abc'], + 'expected' => ['ab', 'abc', 'abcd'], + ], + 'exact match' => [ + 'keys' => ['a', 'b', 'c'], + 'prefixes' => ['a', 'c'], + 'expected' => ['a', 'c'], + ], + 'duplicate keys' => [ + 'keys' => ['a', 'a', 'b', 'c'], + 'prefixes' => ['a'], + 'expected' => ['a', 'a'], + ], + 'duplicate prefixes' => [ + 'keys' => ['a', 'b', 'c'], + 'prefixes' => ['a', 'a', 'b'], + 'expected' => ['a', 'b'], + ], + 'unsorted keys boundry' => [ + 'keys' => ['c', 'b', 'a'], + 'prefixes' => ['a'], + 'expected' => [], + ], + 'unsorted prefixes boundry' => [ + 'keys' => ['a', 'b', 'c'], + 'prefixes' => ['d', 'a'], + 'expected' => [], + ], + 'empty keys' => [ + 'keys' => [], + 'prefixes' => ['a'], + 'expected' => [], + ], + 'empty prefixes' => [ + 'keys' => ['a', 'b', 'c'], + 'prefixes' => [], + 'expected' => [], + ], + ]; + } } From 724e5b45e3e4a399c434140e2b493437c532e57e Mon Sep 17 00:00:00 2001 From: Benjamin Walker Date: Fri, 2 May 2025 20:17:39 +1000 Subject: [PATCH 2/2] MDL-82511 cachestore_redis: Reduce blocking when finding keys --- cache/stores/redis/lib.php | 31 +++++++++++++++++++++++-------- 1 file changed, 23 insertions(+), 8 deletions(-) diff --git a/cache/stores/redis/lib.php b/cache/stores/redis/lib.php index 769303ffd0a..a7f26287f86 100644 --- a/cache/stores/redis/lib.php +++ b/cache/stores/redis/lib.php @@ -735,13 +735,34 @@ class cachestore_redis extends store implements return false; } + /** + * Finds all of the keys being used by this cache store instance using a scan. + * This is preferred over keys to avoid blocking the server for a long time. + * + * @param string $prefix + * @return array of all matching keys in the hash as a numbered array. + */ + protected function scan_keys($prefix = '') { + $return = []; + $iterator = null; + do { + $results = $this->redis->hScan($this->hash, $iterator, "$prefix*", 1000); + if ($results !== false) { + foreach ($results as $key => $value) { + $return[] = $key; + } + } + } while ($iterator != 0); + return $return; + } + /** * Finds all of the keys being used by this cache store instance. * * @return array of all keys in the hash as a numbered array. */ public function find_all() { - return $this->redis->hKeys($this->hash); + return $this->scan_keys(); } /** @@ -752,13 +773,7 @@ class cachestore_redis extends store implements * @return array List of keys that match this prefix. */ public function find_by_prefix($prefix) { - $return = []; - foreach ($this->find_all() as $key) { - if (strpos($key, $prefix) === 0) { - $return[] = $key; - } - } - return $return; + return $this->scan_keys($prefix); } /**