MDL-40677 cache: fixed bug with maxsize when setting

moodle · Jul 21, 2013 · 67da599 · 67da599
1 parent 6f6a8dd
commit 67da599
Show file tree

Hide file tree

Showing 4 changed files with 44 additions and 4 deletions.
diff --git a/cache/stores/session/lib.php b/cache/stores/session/lib.php
@@ -278,16 +278,21 @@ public function get_many($keys) {
      * Sets an item in the cache given its key and data value.
      *
      * @param string $key The key to use.
-     * @param mixed $data The data to set.* @param bool $testmaxsize If set to true then we test the maxsize arg and reduce if required.
+     * @param mixed $data The data to set.
+     * @param bool $testmaxsize If set to true then we test the maxsize arg and reduce if required.
      * @return bool True if the operation was a success false otherwise.
      */
     public function set($key, $data, $testmaxsize = true) {
+        $testmaxsize = ($testmaxsize && $this->maxsize !== false);
+        if ($testmaxsize) {
+            $increment = (!isset($this->store[$key]));
+        }
         if ($this->ttl == 0) {
             $this->store[$key][0] = $data;
         } else {
             $this->store[$key] = array($data, cache::now());
         }
-        if ($testmaxsize && $this->maxsize !== false) {
+        if ($testmaxsize && $increment) {
             $this->storecount++;
             if ($this->storecount > $this->maxsize) {
                 $this->reduce_for_maxsize();
@@ -426,13 +431,22 @@ public function purge() {
 
     /**
      * Reduces the size of the array if maxsize has been hit.
+     *
+     * This function reduces the size of the store reducing it by 10% of its maxsize.
+     * It removes the oldest items in the store when doing this.
+     * The reason it does this an doesn't use a least recently used system is purely the overhead such a system
+     * requires. The current approach is focused on speed, MUC already adds enough overhead to static/session caches
+     * and avoiding more is of benefit.
+     *
      * @return int
      */
     protected function reduce_for_maxsize() {
         $diff = $this->storecount - $this->maxsize;
         if ($diff < 1) {
             return 0;
         }
+        // Reduce it by an extra 10% to avoid calling this repetitively if we are in a loop.
+        $diff += floor($this->maxsize / 10);
         $this->store = array_slice($this->store, $diff, null, true);
         $this->storecount -= $diff;
         return $diff;

diff --git a/cache/stores/session/tests/session_test.php b/cache/stores/session/tests/session_test.php
@@ -92,6 +92,12 @@ public function test_maxsize() {
         $this->assertTrue($instance->set('key7', 'value7'));
         $this->assertEquals('value4', $instance->get('key4'));
 
+        // Set the same key three times to make sure it doesn't count overrides.
+        for ($i = 0; $i < 3; $i++) {
+            $this->assertTrue($instance->set('key8', 'value8'));
+        }
+        $this->assertEquals('value7', $instance->get('key7'), 'Overrides are incorrectly incrementing size');
+
         // Test adding many.
         $this->assertEquals(3, $instance->set_many(array(
             array('key' => 'keyA', 'value' => 'valueA'),

diff --git a/cache/stores/static/lib.php b/cache/stores/static/lib.php
@@ -279,12 +279,16 @@ public function get_many($keys) {
      * @return bool True if the operation was a success false otherwise.
      */
     public function set($key, $data, $testmaxsize = true) {
+        $testmaxsize = ($testmaxsize && $this->maxsize !== false);
+        if ($testmaxsize) {
+            $increment = (!isset($this->store[$key]));
+        }
         if ($this->ttl == 0) {
             $this->store[$key][0] = $data;
         } else {
             $this->store[$key] = array($data, cache::now());
         }
-        if ($testmaxsize && $this->maxsize !== false) {
+        if ($testmaxsize && $increment) {
             $this->storecount++;
             if ($this->storecount > $this->maxsize) {
                 $this->reduce_for_maxsize();
@@ -425,13 +429,22 @@ public function purge() {
 
     /**
      * Reduces the size of the array if maxsize has been hit.
+     *
+     * This function reduces the size of the store reducing it by 10% of its maxsize.
+     * It removes the oldest items in the store when doing this.
+     * The reason it does this an doesn't use a least recently used system is purely the overhead such a system
+     * requires. The current approach is focused on speed, MUC already adds enough overhead to static/session caches
+     * and avoiding more is of benefit.
+     *
      * @return int
      */
     protected function reduce_for_maxsize() {
         $diff = $this->storecount - $this->maxsize;
         if ($diff < 1) {
             return 0;
         }
+        // Reduce it by an extra 10% to avoid calling this repetitively if we are in a loop.
+        $diff += floor($this->maxsize / 10);
         $this->store = array_slice($this->store, $diff, null, true);
         $this->storecount -= $diff;
         return $diff;

diff --git a/cache/stores/static/tests/static_test.php b/cache/stores/static/tests/static_test.php
@@ -1,5 +1,5 @@
 <?php
-// This static is part of Moodle - http://moodle.org/
+// This file is part of Moodle - http://moodle.org/
 //
 // Moodle is free software: you can redistribute it and/or modify
 // it under the terms of the GNU General Public License as published by
@@ -92,6 +92,13 @@ public function test_maxsize() {
         $this->assertTrue($instance->set('key7', 'value7'));
         $this->assertEquals('value4', $instance->get('key4'));
 
+        // Set the same key three times to make sure it doesn't count overrides.
+        for ($i = 0; $i < 3; $i++) {
+            $this->assertTrue($instance->set('key8', 'value8'));
+        }
+
+        $this->assertEquals('value7', $instance->get('key7'), 'Overrides are incorrectly incrementing size');
+
         // Test adding many.
         $this->assertEquals(3, $instance->set_many(array(
             array('key' => 'keyA', 'value' => 'valueA'),