Skip to content

Commit

Permalink
MDL-78466 cache: Perform strict test on static cache values
Browse files Browse the repository at this point in the history 
If a statically accelerated cache returns an empty array then the value
was still fetched from the non-static cache store.

The check of the `$result` should be strictly checked against `false`,
which is the value used if no value was found.
  • Loading branch information
@andrewnicols
andrewnicols committed Jun 14, 2023
1 parent 76fe404 commit 02652c7
Show file tree
Hide file tree
Showing 2 changed files with 160 additions and 3 deletions.
6 changes: 3 additions & 3 deletions cache/classes/loaders.php
View file
Expand Up @@ -457,7 +457,7 @@ protected static function check_version($result, int $requiredversion): bool {
}
} else {
// If there's no result, obviously it doesn't meet the required version.
if (!$result) {
if ($result === false) {
return false;
}
if (!($result instanceof \core_cache\version_wrapper)) {
Expand Down Expand Up @@ -490,7 +490,7 @@ protected function get_implementation($key, int $requiredversion, int $strictnes

if ($usesstaticacceleration) {
$result = $this->static_acceleration_get($key);
if ($result && self::check_version($result, $requiredversion)) {
if ($result !== false && self::check_version($result, $requiredversion)) {
if ($requiredversion === self::VERSION_NONE) {
return $result;
} else {
Expand All @@ -505,7 +505,7 @@ protected function get_implementation($key, int $requiredversion, int $strictnes

// 3. Get it from the store. Obviously wasn't in the static acceleration array.
$result = $this->store->get($parsedkey);
if ($result) {
if ($result !== false) {
// Check the result has at least the required version.
try {
$validversion = self::check_version($result, $requiredversion);
Expand Down
157 changes: 157 additions & 0 deletions cache/tests/cache_test.php
View file
Expand Up @@ -50,6 +50,7 @@
* @copyright 2012 Sam Hemelryk
* @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
* @coversDefaultClass \cache
* @covers \cache
*/
class cache_test extends \advanced_testcase {

Expand Down Expand Up @@ -2827,6 +2828,162 @@ public function test_performance_debug() {
$startstats[$requestid]['stores']['default_request']['sets']);
}

/**
* Data provider for static acceleration performance tests.
*
* @return array
*/
public function static_acceleration_performance_provider(): array {
// Note: These are the delta values, not the absolute values.
// Also note that the set will actually store the valuein the static cache immediately.
$validfirst = [
'default_application' => [
'hits' => 1,
'misses' => 0,
],
cache_store::STATIC_ACCEL => [
'hits' => 0,
'misses' => 1,
],
];

$validsecond = [
'default_application' => [
'hits' => 0,
'misses' => 0,
],
cache_store::STATIC_ACCEL => [
'hits' => 1,
'misses' => 0,
],
];

$invalidfirst = [
'default_application' => [
'hits' => 0,
'misses' => 1,
],
cache_store::STATIC_ACCEL => [
'hits' => 0,
'misses' => 1,
],
];
$invalidsecond = [
'default_application' => [
'hits' => 0,
'misses' => 1,
],
cache_store::STATIC_ACCEL => [
'hits' => 0,
'misses' => 1,
],
];;

return [
'Truthy' => [
true,
$validfirst,
$validsecond,
],
'Null' => [
null,
$validfirst,
$validsecond,
],
'Empty Array' => [
[],
$validfirst,
$validsecond,
],
'Empty String' => [
'',
$validfirst,
$validsecond,
],
'False' => [
false,
$invalidfirst,
$invalidsecond,
],
];
}

/**
* Test performance of static acceleration caches with values which are frequently confused with missing values.
*
* @dataProvider static_acceleration_performance_provider
* @param mixed $value The value to test
* @param array $firstfetchstats The expected stats on the first fetch
* @param array $secondfetchstats The expected stats on the subsequent fetch
*/
public function test_static_acceleration_values_performance(
$value,
array $firstfetchstats,
array $secondfetchstats
): void {
// Note: We need to modify perfdebug to test this.
global $CFG;
$this->resetAfterTest(true);
$CFG->perfdebug = 15;

$instance = cache_config_testing::instance();
$instance->phpunit_add_definition('phpunit/accelerated', [
'mode' => cache_store::MODE_APPLICATION,
'component' => 'phpunit',
'area' => 'accelerated',
'staticacceleration' => true,
'staticaccelerationsize' => 1,
]);

$cache = cache::make('phpunit', 'accelerated');
$this->assertInstanceOf(cache_phpunit_application::class, $cache);

$this->assertTrue($cache->set('value', $value));

$checkstats = function(
array $start,
array $expectedstats
): array {
$applicationid = 'phpunit/accelerated';
$endstats = cache_helper::get_stats();

$start = $start[$applicationid]['stores'];
$end = $endstats[$applicationid]['stores'];

foreach ($expectedstats as $cachename => $expected) {
foreach ($expected as $type => $value) {
$startvalue = array_key_exists($cachename, $start) ? $start[$cachename][$type] : 0;
$endvalue = array_key_exists($cachename, $end) ? $end[$cachename][$type] : 0;
$diff = $endvalue - $startvalue;
$this->assertEquals(
$value,
$diff,
"Expected $cachename $type to be $value, got $diff"
);
}
}

return $endstats;
};

// Reset the cache factory so that we can get the stats from a fresh instance.
$factory = cache_factory::instance();
$factory->reset_cache_instances();
$cache = cache::make('phpunit', 'accelerated');

// Get the initial stats.
$startstats = cache_helper::get_stats();

// Fetching the value the first time should seed the static cache from the application cache.
$this->assertEquals($value, $cache->get('value'));
$startstats = $checkstats($startstats, $firstfetchstats);

// Fetching the value should only hit the static cache.
$this->assertEquals($value, $cache->get('value'));
$checkstats($startstats, $secondfetchstats);
}


public function test_static_cache() {
global $CFG;
$this->resetAfterTest(true);
Expand Down

0 comments on commit 02652c7

Please sign in to comment.