From 0cc9d709abb0df15066baba96579aa79db23e5fc Mon Sep 17 00:00:00 2001 From: Marina Glancy Date: Thu, 26 Mar 2015 13:56:03 +0800 Subject: [PATCH] MDL-46960 completionlib: adjustments to caching --- lib/completionlib.php | 56 +++++++++++++++++++++++++------- lib/db/caches.php | 4 ++- lib/tests/completionlib_test.php | 30 +++++++++-------- 3 files changed, 64 insertions(+), 26 deletions(-) diff --git a/lib/completionlib.php b/lib/completionlib.php index b7af0b2783b..62afaf59929 100644 --- a/lib/completionlib.php +++ b/lib/completionlib.php @@ -244,7 +244,9 @@ class completion_info { * Constructs with course details. * * When instantiating a new completion info object you must provide a course - * object with at least id, and enablecompletion properties. + * object with at least id, and enablecompletion properties. Property + * cacherev is needed if you check completion of the current user since + * it is used for cache validation. * * @param stdClass $course Moodle course object. */ @@ -284,7 +286,7 @@ class completion_info { // Load data if we do not have enough if (!isset($this->course->enablecompletion)) { - $this->course->enablecompletion = $DB->get_field('course', 'enablecompletion', array('id' => $this->course->id)); + $this->course = get_course($this->course_id); } // Check course completion @@ -777,6 +779,9 @@ class completion_info { $DB->delete_records('course_completions', array('course' => $this->course_id)); $DB->delete_records('course_completion_crit_compl', array('course' => $this->course_id)); + + // Difficult to find affected users, just purge all completion cache. + cache::make('core', 'completion')->purge(); } /** @@ -825,6 +830,9 @@ class completion_info { $DB->delete_records('course_completion_crit_compl', array('course' => $this->course_id, 'criteriaid' => $acriteria->id)); $DB->delete_records('course_completions', array('course' => $this->course_id)); } + + // Difficult to find affected users, just purge all completion cache. + cache::make('core', 'completion')->purge(); } /** @@ -895,9 +903,21 @@ class completion_info { $userid = $USER->id; } - // See if requested data is present in cache - if ($cacheddata = $completioncache->get($userid . '_' . $this->course->id . '_' . $cm->id)) { - return $cacheddata; + // See if requested data is present in cache (use cache for current user only). + $usecache = $userid == $USER->id; + $cacheddata = array(); + if ($usecache) { + if (!isset($this->course->cacherev)) { + $this->course = get_course($this->course_id); + } + if ($cacheddata = $completioncache->get($userid . '_' . $this->course->id)) { + if ($cacheddata['cacherev'] != $this->course->cacherev) { + // Course structure has been changed since the last caching, forget the cache. + $cacheddata = array(); + } else if (array_key_exists($cm->id, $cacheddata)) { + return $cacheddata[$cm->id]; + } + } } // Not there, get via SQL @@ -937,13 +957,12 @@ class completion_info { $data->viewed = 0; $data->timemodified = 0; } - $completioncache->set($userid . '_' . $this->course->id . '_' . $othercm->id, $data); + $cacheddata[$othercm->id] = $data; } - if (!$completiondata = $completioncache->get($userid . '_' . $this->course->id . '_' . $cm->id)) { + if (!isset($cacheddata[$cm->id])) { $this->internal_systemerror("Unexpected error: course-module {$cm->id} could not be found on course {$this->course->id}"); } - return $completiondata; } else { // Get single record @@ -960,10 +979,14 @@ class completion_info { } // Put in cache - $completioncache->set($userid . '_' . $this->course->id . '_' . $cm->id, $data); + $cacheddata[$cm->id] = $data; } - return $data; + if ($usecache) { + $cacheddata['cacherev'] = $this->course->cacherev; + $completioncache->set($userid . '_' . $this->course->id, $cacheddata); + } + return $cacheddata[$cm->id]; } /** @@ -1009,11 +1032,20 @@ class completion_info { $event->trigger(); $completioncache = cache::make('core', 'completion'); - $completioncache->set($data->userid . '_' . $cm->course . '_' . $cm->id, $data); - // TODO under what circumstances should I call get_fast_modinfo()? if ($data->userid == $USER->id) { + // Update module completion in user's cache. + if (!($cachedata = $completioncache->get($data->userid . '_' . $cm->course)) + || $cachedata['cacherev'] != $this->course->cacherev) { + $cachedata = array('cacherev' => $this->course->cacherev); + } + $cachedata[$cm->id] = $data; + $completioncache->set($data->userid . '_' . $cm->course, $cachedata); + // reset modinfo for user (no need to call rebuild_course_cache()) get_fast_modinfo($cm->course, 0, true); + } else { + // Remove another user's completion cache for this course. + $completioncache->delete($data->userid . '_' . $cm->course); } } diff --git a/lib/db/caches.php b/lib/db/caches.php index 2b08e21f4c2..98dadff4601 100644 --- a/lib/db/caches.php +++ b/lib/db/caches.php @@ -217,7 +217,9 @@ $definitions = array( 'completion' => array( 'mode' => cache_store::MODE_APPLICATION, 'simplekeys' => true, - 'ttl' => 10*60, + 'ttl' => 3600, + 'staticacceleration' => true, + 'staticaccelerationsize' => 2, // Should be current course and site course. ), // A simple cache that stores whether a user can expand a course in the navigation. diff --git a/lib/tests/completionlib_test.php b/lib/tests/completionlib_test.php index 927ae65c6c2..0484b7cc3ad 100644 --- a/lib/tests/completionlib_test.php +++ b/lib/tests/completionlib_test.php @@ -386,7 +386,7 @@ class core_completionlib_testcase extends advanced_testcase { $cache = cache::make('core', 'completion'); - $c = new completion_info((object)array('id'=>42)); + $c = new completion_info((object)array('id'=>42, 'cacherev'=>1)); $cm = (object)array('id'=>13, 'course'=>42); // 1. Not current user, record exists. @@ -399,7 +399,7 @@ class core_completionlib_testcase extends advanced_testcase { ->will($this->returnValue($sillyrecord)); $result = $c->get_data($cm, false, 123); $this->assertEquals($sillyrecord, $result); - $this->assertEquals($cache->get('123_42_13'), $sillyrecord); + $this->assertEquals(false, $cache->get('123_42')); // Not current user is not cached. // 2. Not current user, default record, whole course. $cache->purge(); @@ -412,7 +412,7 @@ class core_completionlib_testcase extends advanced_testcase { $this->assertEquals((object)array( 'id'=>'0', 'coursemoduleid'=>13, 'userid'=>123, 'completionstate'=>0, 'viewed'=>0, 'timemodified'=>0), $result); - $this->assertEquals($cache->get('123_42_13'), $result); + $this->assertEquals(false, $cache->get('123_42')); // Not current user is not cached. // 3. Current user, single record, not from cache. $DB->expects($this->at(0)) @@ -421,7 +421,8 @@ class core_completionlib_testcase extends advanced_testcase { ->will($this->returnValue($sillyrecord)); $result = $c->get_data($cm); $this->assertEquals($sillyrecord, $result); - $this->assertEquals($sillyrecord, $cache->get('314159_42_13')); + $cachevalue = $cache->get('314159_42'); + $this->assertEquals($sillyrecord, $cachevalue[13]); // 4. Current user, 'whole course', but from cache. $result = $c->get_data($cm, true); @@ -445,10 +446,11 @@ class core_completionlib_testcase extends advanced_testcase { $this->assertEquals($basicrecord, $result); // Check the cache contents. - $this->assertEquals($basicrecord, $cache->get('314159_42_13')); - $this->assertEquals((object)array('id'=>'0', 'coursemoduleid'=>14, + $cachevalue = $cache->get('314159_42'); + $this->assertEquals($basicrecord, $cachevalue[13]); + $this->assertEquals((object)array('id'=>'0', 'coursemoduleid'=>14, 'userid'=>314159, 'completionstate'=>0, 'viewed'=>0, 'timemodified'=>0), - $cache->get('314159_42_14')); + $cachevalue[14]); } public function test_internal_set_data() { @@ -474,8 +476,9 @@ class core_completionlib_testcase extends advanced_testcase { $d1 = $DB->get_field('course_modules_completion', 'id', array('coursemoduleid' => $cm->id)); $this->assertEquals($d1, $data->id); $cache = cache::make('core', 'completion'); - $this->assertEquals($cache->get($data->userid . '_' . $cm->course . '_' . $cm->id), - $data); + // Cache was not set for another user. + $this->assertEquals(array('cacherev' => $this->course->cacherev, $cm->id => $data), + $cache->get($data->userid . '_' . $cm->course)); // 2) Test with existing data and for different user. $forum2 = $this->getDataGenerator()->create_module('forum', array('course' => $this->course->id), $completionauto); @@ -490,10 +493,11 @@ class core_completionlib_testcase extends advanced_testcase { $d2->timemodified = time(); $d2->viewed = COMPLETION_NOT_VIEWED; $c->internal_set_data($cm2, $d2); - $this->assertEquals($cache->get($data->userid . '_' . $cm->course . '_' . $cm->id), - $data); - $this->assertEquals($cache->get($d2->userid . '_' . $cm2->course . '_' . $cm2->id), - $d2); + // Cache for current user returns the data. + $cachevalue = $cache->get($data->userid . '_' . $cm->course); + $this->assertEquals($data, $cachevalue[$cm->id]); + // Cache for another user is not filled. + $this->assertEquals(false, $cache->get($d2->userid . '_' . $cm2->course)); // 3) Test where it THINKS the data is new (from cache) but actually // in the database it has been set since.