From 02c07301fc9021fb433832803c5dcef17f56ee42 Mon Sep 17 00:00:00 2001
From: Alex Pott <alex.a.pott@googlemail.com>
Date: Thu, 22 Feb 2024 15:55:51 +0000
Subject: [PATCH] Issue #3422981 by catch, gorkagr, Wim Leers: Return early in
CacheTagsChecksumTrait::isValid() (and possibly elsewhere) if there are no
tags to check
---
.../Core/Cache/CacheTagsChecksumTrait.php | 5 ++++
.../src/Cache/CacheTagsChecksumDecorator.php | 5 ++++
...nTelemetryAuthenticatedPerformanceTest.php | 2 +-
.../StandardPerformanceTest.php | 10 ++++----
.../Drupal/Tests/PerformanceTestTrait.php | 23 ++++++++++++++-----
5 files changed, 33 insertions(+), 12 deletions(-)
diff --git a/core/lib/Drupal/Core/Cache/CacheTagsChecksumTrait.php b/core/lib/Drupal/Core/Cache/CacheTagsChecksumTrait.php
index 2a14f8a5407e..25bc3673fbb1 100644
--- a/core/lib/Drupal/Core/Cache/CacheTagsChecksumTrait.php
+++ b/core/lib/Drupal/Core/Cache/CacheTagsChecksumTrait.php
@@ -110,6 +110,11 @@ public function getCurrentChecksum(array $tags) {
* Implements \Drupal\Core\Cache\CacheTagsChecksumInterface::isValid()
*/
public function isValid($checksum, array $tags) {
+ // If there are no cache tags, then there is no cache tag to validate,
+ // hence it's always valid.
+ if (empty($tags)) {
+ return TRUE;
+ }
// Any cache reads in this request involving cache tags whose invalidation
// has been delayed due to an in-progress transaction are not allowed to use
// data stored in cache; it must be assumed to be stale. This forces those
diff --git a/core/modules/system/tests/modules/performance_test/src/Cache/CacheTagsChecksumDecorator.php b/core/modules/system/tests/modules/performance_test/src/Cache/CacheTagsChecksumDecorator.php
index 489786abcda0..f1695fd2947f 100644
--- a/core/modules/system/tests/modules/performance_test/src/Cache/CacheTagsChecksumDecorator.php
+++ b/core/modules/system/tests/modules/performance_test/src/Cache/CacheTagsChecksumDecorator.php
@@ -30,6 +30,11 @@ public function getCurrentChecksum(array $tags) {
* {@inheritdoc}
*/
public function isValid($checksum, array $tags) {
+ // If there are no cache tags, the cache item is always valid, and the child
+ // method will be a no-op, so don't log anything.
+ if (empty($tags)) {
+ return $this->checksumInvalidator->isValid($checksum, $tags);
+ }
$start = microtime(TRUE);
$return = $this->checksumInvalidator->isValid($checksum, $tags);
$stop = microtime(TRUE);
diff --git a/core/profiles/demo_umami/tests/src/FunctionalJavascript/OpenTelemetryAuthenticatedPerformanceTest.php b/core/profiles/demo_umami/tests/src/FunctionalJavascript/OpenTelemetryAuthenticatedPerformanceTest.php
index 2612ecf5bac5..3356988bad56 100644
--- a/core/profiles/demo_umami/tests/src/FunctionalJavascript/OpenTelemetryAuthenticatedPerformanceTest.php
+++ b/core/profiles/demo_umami/tests/src/FunctionalJavascript/OpenTelemetryAuthenticatedPerformanceTest.php
@@ -40,7 +40,7 @@ public function testFrontPageAuthenticatedWarmCache(): void {
$this->assertSame(0, $performance_data->getCacheSetCount());
$this->assertSame(0, $performance_data->getCacheDeleteCount());
$this->assertSame(0, $performance_data->getCacheTagChecksumCount());
- $this->assertSame(54, $performance_data->getCacheTagIsValidCount());
+ $this->assertSame(13, $performance_data->getCacheTagIsValidCount());
$this->assertSame(0, $performance_data->getCacheTagInvalidationCount());
}
diff --git a/core/profiles/standard/tests/src/FunctionalJavascript/StandardPerformanceTest.php b/core/profiles/standard/tests/src/FunctionalJavascript/StandardPerformanceTest.php
index 5fece5679a74..1cf1cdf6acb1 100644
--- a/core/profiles/standard/tests/src/FunctionalJavascript/StandardPerformanceTest.php
+++ b/core/profiles/standard/tests/src/FunctionalJavascript/StandardPerformanceTest.php
@@ -60,7 +60,7 @@ public function testAnonymous() {
$this->assertSame(47, $performance_data->getCacheSetCount());
$this->assertSame(0, $performance_data->getCacheDeleteCount());
$this->assertCountBetween(143, 146, $performance_data->getCacheTagChecksumCount());
- $this->assertCountBetween(177, 180, $performance_data->getCacheTagIsValidCount());
+ $this->assertCountBetween(47, 50, $performance_data->getCacheTagIsValidCount());
$this->assertSame(0, $performance_data->getCacheTagInvalidationCount());
// Test node page.
@@ -74,7 +74,7 @@ public function testAnonymous() {
$this->assertSame(16, $performance_data->getCacheSetCount());
$this->assertSame(0, $performance_data->getCacheDeleteCount());
$this->assertCountBetween(79, 80, $performance_data->getCacheTagChecksumCount());
- $this->assertCountBetween(149, 150, $performance_data->getCacheTagIsValidCount());
+ $this->assertCountBetween(41, 42, $performance_data->getCacheTagIsValidCount());
$this->assertSame(0, $performance_data->getCacheTagInvalidationCount());
// Test user profile page.
@@ -87,7 +87,7 @@ public function testAnonymous() {
$this->assertSame(81, $performance_data->getCacheGetCount());
$this->assertSame(16, $performance_data->getCacheSetCount());
$this->assertSame(0, $performance_data->getCacheDeleteCount());
- $this->assertCountBetween(129, 130, $performance_data->getCacheTagIsValidCount());
+ $this->assertCountBetween(36, 37, $performance_data->getCacheTagIsValidCount());
$this->assertSame(0, $performance_data->getCacheTagInvalidationCount());
}
@@ -118,7 +118,7 @@ public function testLogin(): void {
$this->assertSame(1, $performance_data->getCacheSetCount());
$this->assertSame(1, $performance_data->getCacheDeleteCount());
$this->assertSame(1, $performance_data->getCacheTagChecksumCount());
- $this->assertSame(69, $performance_data->getCacheTagIsValidCount());
+ $this->assertSame(28, $performance_data->getCacheTagIsValidCount());
$this->assertSame(0, $performance_data->getCacheTagInvalidationCount());
}
@@ -151,7 +151,7 @@ public function testLoginBlock(): void {
$this->assertSame(1, $performance_data->getCacheSetCount());
$this->assertSame(1, $performance_data->getCacheDeleteCount());
$this->assertSame(1, $performance_data->getCacheTagChecksumCount());
- $this->assertSame(107, $performance_data->getCacheTagIsValidCount());
+ $this->assertSame(31, $performance_data->getCacheTagIsValidCount());
$this->assertSame(0, $performance_data->getCacheTagInvalidationCount());
}
diff --git a/core/tests/Drupal/Tests/PerformanceTestTrait.php b/core/tests/Drupal/Tests/PerformanceTestTrait.php
index b2d05dfe1f08..ce8075de067a 100644
--- a/core/tests/Drupal/Tests/PerformanceTestTrait.php
+++ b/core/tests/Drupal/Tests/PerformanceTestTrait.php
@@ -4,6 +4,7 @@
namespace Drupal\Tests;
+use Drupal\Core\Database\Event\DatabaseEvent;
use Drupal\performance_test\Cache\CacheTagOperation;
use OpenTelemetry\API\Trace\SpanKind;
use OpenTelemetry\Contrib\Otlp\OtlpHttpTransportFactory;
@@ -130,11 +131,7 @@ public function collectPerformanceData(callable $callable, ?string $service_name
foreach ($performance_test_data['database_events'] as $event) {
// Don't log queries from the database cache backend because they're
// logged separately as cache operations.
- $database_cache = FALSE;
- if (isset($event->caller['class'])) {
- $database_cache = is_a(str_replace('\\\\', '\\', $event->caller['class']), '\Drupal\Core\Cache\DatabaseBackend', TRUE) || is_a(str_replace('\\\\', '\\', $event->caller['class']), 'Drupal\Core\Cache\DatabaseCacheTagsChecksum', TRUE);
- }
- if (!$database_cache) {
+ if (!static::isDatabaseCache($event)) {
$query_count++;
}
}
@@ -361,7 +358,7 @@ private function openTelemetryTracing(array $messages, string $service_name): vo
$performance_test_data = $collection->get('performance_test_data');
$query_events = $performance_test_data['database_events'] ?? [];
foreach ($query_events as $key => $event) {
- if (isset($event->caller['class']) && is_a(str_replace('\\\\', '\\', $event->caller['class']), '\Drupal\Core\Cache\DatabaseBackend', TRUE)) {
+ if (static::isDatabaseCache($event)) {
continue;
}
// Use the first part of the database query for the span name.
@@ -460,4 +457,18 @@ protected function assertCountBetween(int $min, int $max, int $actual) {
);
}
+ /**
+ * Checks whether a database event is from the database cache implementation.
+ *
+ * @param Drupal\Core\Database\Event\DatabaseEvent $event
+ * The database event.
+ *
+ * @return bool
+ * Whether the event was triggered by the database cache implementation.
+ */
+ protected static function isDatabaseCache(DatabaseEvent $event): bool {
+ $class = str_replace('\\\\', '\\', $event->caller['class']);
+ return is_a($class, '\Drupal\Core\Cache\DatabaseBackend', TRUE) || is_a($class, '\Drupal\Core\Cache\DatabaseCacheTagsChecksum', TRUE);
+ }
+
}
--
GitLab