From 690b27c832009ce770b21e388dbc3961b6559de8 Mon Sep 17 00:00:00 2001 From: Mark Johnson Date: Wed, 11 Oct 2023 09:12:18 +0100 Subject: [PATCH] MDL-79615 course: Improve efficency of cache rebuilds When bulk-updating course module visibility, set_coursemodule_visibily was triggering a partial cache purge and rebuild for each course module. This potentially led to 2 cache sets each requiring a lock to be acquired and released per course module, plus any other cache updates for other changes to the course in the same request. This adds a new $rebuildcache paramter to the set_coursemodule_visibilty, which is true by default to retain the existing behaviour. If set false, it will skip doing the partial purge and rebuild for that course module, and it is up to the calling code to ensure the cache is updated as requried. To assist with this, there is a new course_modinfo::purge_course_module_cache_multiple() method, which allows multiple course modules to be purged from the cache in a single cache set. --- course/format/classes/stateactions.php | 7 ++- course/lib.php | 21 +++++-- course/tests/courselib_test.php | 47 +++++++++++++++ course/upgrade.txt | 17 ++++++ lib/modinfolib.php | 36 ++++++++--- lib/tests/modinfolib_test.php | 82 ++++++++++++++++++++++++++ lib/upgrade.txt | 4 ++ 7 files changed, 200 insertions(+), 14 deletions(-) diff --git a/course/format/classes/stateactions.php b/course/format/classes/stateactions.php index 2110cfd96af..46c83443f6d 100644 --- a/course/format/classes/stateactions.php +++ b/course/format/classes/stateactions.php @@ -530,9 +530,14 @@ class stateactions { $allowstealth = !empty($CFG->allowstealth) && $format->allow_stealth_module_visibility($cm, $section); $coursevisible = ($allowstealth) ? 0 : 1; } - set_coursemodule_visible($cm->id, $visible, $coursevisible); + set_coursemodule_visible($cm->id, $visible, $coursevisible, false); $modcontext = context_module::instance($cm->id); course_module_updated::create_from_cm($cm, $modcontext)->trigger(); + } + course_modinfo::purge_course_modules_cache($course->id, $ids); + rebuild_course_cache($course->id, false, true); + + foreach ($cms as $cm) { $updates->add_cm_put($cm->id); } } diff --git a/course/lib.php b/course/lib.php index a45f4fe9f7d..b5e0a6fffd1 100644 --- a/course/lib.php +++ b/course/lib.php @@ -709,12 +709,17 @@ function set_downloadcontent(int $id, bool $downloadcontent): bool { * has been moved to {@link set_section_visible()} which was the only place from which * the parameter was used. * + * If $rebuildcache is set to false, the calling code is responsible for ensuring the cache is purged + * and rebuilt as appropriate. Consider using this if set_coursemodule_visible is called multiple times + * (e.g. in a loop). + * * @param int $id of the module * @param int $visible state of the module * @param int $visibleoncoursepage state of the module on the course page + * @param bool $rebuildcache If true (default), perform a partial cache purge and rebuild. * @return bool false when the module was not found, true otherwise */ -function set_coursemodule_visible($id, $visible, $visibleoncoursepage = 1) { +function set_coursemodule_visible($id, $visible, $visibleoncoursepage = 1, bool $rebuildcache = true) { global $DB, $CFG; require_once($CFG->libdir.'/gradelib.php'); require_once($CFG->dirroot.'/calendar/lib.php'); @@ -771,8 +776,10 @@ function set_coursemodule_visible($id, $visible, $visibleoncoursepage = 1) { } } - \course_modinfo::purge_course_module_cache($cm->course, $cm->id); - rebuild_course_cache($cm->course, false, true); + if ($rebuildcache) { + \course_modinfo::purge_course_module_cache($cm->course, $cm->id); + rebuild_course_cache($cm->course, false, true); + } return true; } @@ -1457,20 +1464,22 @@ function course_update_section($course, $section, $data) { // If section visibility was changed, hide the modules in this section too. if ($changevisibility && !empty($section->sequence)) { $modules = explode(',', $section->sequence); + $cmids = []; foreach ($modules as $moduleid) { if ($cm = get_coursemodule_from_id(null, $moduleid, $courseid)) { + $cmids[] = $cm->id; if ($data['visible']) { // As we unhide the section, we use the previously saved visibility stored in visibleold. - set_coursemodule_visible($moduleid, $cm->visibleold, $cm->visibleoncoursepage); + set_coursemodule_visible($moduleid, $cm->visibleold, $cm->visibleoncoursepage, false); } else { // We hide the section, so we hide the module but we store the original state in visibleold. - set_coursemodule_visible($moduleid, 0, $cm->visibleoncoursepage); + set_coursemodule_visible($moduleid, 0, $cm->visibleoncoursepage, false); $DB->set_field('course_modules', 'visibleold', $cm->visible, ['id' => $moduleid]); - \course_modinfo::purge_course_module_cache($cm->course, $cm->id); } \core\event\course_module_updated::create_from_cm($cm)->trigger(); } } + \course_modinfo::purge_course_modules_cache($courseid, $cmids); rebuild_course_cache($courseid, false, true); } } diff --git a/course/tests/courselib_test.php b/course/tests/courselib_test.php index f972ed5c346..728736f476f 100644 --- a/course/tests/courselib_test.php +++ b/course/tests/courselib_test.php @@ -1333,6 +1333,53 @@ class courselib_test extends advanced_testcase { } } + /** + * Test rebuildcache = false behaviour. + * + * When we pass rebuildcache = false to set_coursemodule_visible, the corusemodinfo cache will still contain + * the original visibility until we trigger a rebuild. + * + * @return void + * @covers ::set_coursemodule_visible + */ + public function test_module_visibility_no_rebuild(): void { + $this->setAdminUser(); + $this->resetAfterTest(true); + + // Create course and modules. + $course = $this->getDataGenerator()->create_course(['numsections' => 5]); + $forum = $this->getDataGenerator()->create_module('forum', ['course' => $course->id]); + $assign = $this->getDataGenerator()->create_module('assign', ['duedate' => time(), 'course' => $course->id]); + $modules = compact('forum', 'assign'); + + // Hiding the modules. + foreach ($modules as $mod) { + set_coursemodule_visible($mod->cmid, 0, 1, false); + // The modinfo cache still has the original visibility until we manually trigger a rebuild. + $cm = get_fast_modinfo($mod->course)->get_cm($mod->cmid); + $this->assertEquals(1, $cm->visible); + } + + rebuild_course_cache($course->id); + + foreach ($modules as $mod) { + $this->check_module_visibility($mod, 0, 0); + } + + // Showing the modules. + foreach ($modules as $mod) { + set_coursemodule_visible($mod->cmid, 1, 1, false); + $cm = get_fast_modinfo($mod->course)->get_cm($mod->cmid); + $this->assertEquals(0, $cm->visible); + } + + rebuild_course_cache($course->id); + + foreach ($modules as $mod) { + $this->check_module_visibility($mod, 1, 1); + } + } + public function test_section_visibility_events() { $this->setAdminUser(); $this->resetAfterTest(true); diff --git a/course/upgrade.txt b/course/upgrade.txt index d6d60e0ddb9..36531e83710 100644 --- a/course/upgrade.txt +++ b/course/upgrade.txt @@ -1,6 +1,23 @@ This files describes API changes in /course/*, information provided here is intended especially for developers. +=== 4.3.1 === +* set_coursemodule_visible() has a new $rebuildcache parameter. If this is being called multiple times in the same request, + consider passing `false` for this parameter and rebuilding the cache once after all the course modules have been updated. + See course_update_section() for an example. + +=== 4.3 === +* The `core_course_renderer::course_section_cm_completion` method has been removed, and can no longer be used +* External function core_course_external::get_course_contents() now returns a new field activitybadge with the data to display + the activity badge when the module implements it. +* prepare_new_moduleinfo_data() now accepts a parameter "suffix" that will be added to the name of the completion rules. +* The method core_course_bulk_activity_completion_renderer:: edit_default_completion() has been deprecated and will be removed. +* The `core_course\output\activity_information` output class has been deprecated. Use `core_course\output\activity_completion` + and `core_course\output\activity_dates` instead. + Alongside with that, the following methods have been deprecated too: + - `core_course_renderer::render_activity_information()` + - `renderer_base::activity_information()` + === 4.2 === * course/mod.php now accepts parameter beforemod for adding course modules. It contains the course module id of an existing course module. The new module is inserted before this module. diff --git a/lib/modinfolib.php b/lib/modinfolib.php index 5f573d472c3..fb7e6edf367 100644 --- a/lib/modinfolib.php +++ b/lib/modinfolib.php @@ -742,19 +742,41 @@ class course_modinfo { * @param int $cmid Course module id */ public static function purge_course_module_cache(int $courseid, int $cmid): void { + self::purge_course_modules_cache($courseid, [$cmid]); + } + + /** + * Purge the cache of multiple course modules. + * + * @param int $courseid Course id + * @param int[] $cmids List of course module ids + * @return void + */ + public static function purge_course_modules_cache(int $courseid, array $cmids): void { $course = get_course($courseid); $cache = cache::make('core', 'coursemodinfo'); $cachekey = $course->id; $cache->acquire_lock($cachekey); - $coursemodinfo = $cache->get_versioned($cachekey, $course->cacherev); - $hascache = ($coursemodinfo !== false) && array_key_exists($cmid, $coursemodinfo->modinfo); - if ($hascache) { - $coursemodinfo->cacherev = -1; - unset($coursemodinfo->modinfo[$cmid]); - $cache->set_versioned($cachekey, $course->cacherev, $coursemodinfo); + try { $coursemodinfo = $cache->get_versioned($cachekey, $course->cacherev); + $hascache = ($coursemodinfo !== false); + $updatedcache = false; + if ($hascache) { + foreach ($cmids as $cmid) { + if (array_key_exists($cmid, $coursemodinfo->modinfo)) { + unset($coursemodinfo->modinfo[$cmid]); + $updatedcache = true; + } + } + if ($updatedcache) { + $coursemodinfo->cacherev = -1; + $cache->set_versioned($cachekey, $course->cacherev, $coursemodinfo); + $cache->get_versioned($cachekey, $course->cacherev); + } + } + } finally { + $cache->release_lock($cachekey); } - $cache->release_lock($cachekey); } /** diff --git a/lib/tests/modinfolib_test.php b/lib/tests/modinfolib_test.php index f99049a1d1f..fbd2ed54278 100644 --- a/lib/tests/modinfolib_test.php +++ b/lib/tests/modinfolib_test.php @@ -1099,6 +1099,88 @@ class modinfolib_test extends advanced_testcase { $this->assertEquals(-1, $coursemodinfo->cacherev); } + /** + * Purge a single course module from the cache. + * + * @return void + * @covers \course_modinfo::purge_course_module_cache + */ + public function test_purge_course_module(): void { + $this->resetAfterTest(); + $this->setAdminUser(); + $cache = cache::make('core', 'coursemodinfo'); + + // Generate the course and pre-requisite section. + $course = $this->getDataGenerator()->create_course(); + $cm1 = $this->getDataGenerator()->create_module('page', ['course' => $course]); + $cm2 = $this->getDataGenerator()->create_module('page', ['course' => $course]); + $cm3 = $this->getDataGenerator()->create_module('page', ['course' => $course]); + $cm4 = $this->getDataGenerator()->create_module('page', ['course' => $course]); + // Reset course cache. + rebuild_course_cache($course->id, true); + // Build course cache. + get_fast_modinfo($course->id); + // Get the course modinfo cache. + $coursemodinfo = $cache->get_versioned($course->id, $course->cacherev); + $this->assertCount(4, $coursemodinfo->modinfo); + $this->assertArrayHasKey($cm1->cmid, $coursemodinfo->modinfo); + $this->assertArrayHasKey($cm2->cmid, $coursemodinfo->modinfo); + $this->assertArrayHasKey($cm3->cmid, $coursemodinfo->modinfo); + $this->assertArrayHasKey($cm4->cmid, $coursemodinfo->modinfo); + + course_modinfo::purge_course_module_cache($course->id, $cm1->cmid); + + $coursemodinfo = $cache->get_versioned($course->id, $course->cacherev); + $this->assertCount(3, $coursemodinfo->modinfo); + $this->assertArrayNotHasKey($cm1->cmid, $coursemodinfo->modinfo); + $this->assertArrayHasKey($cm2->cmid, $coursemodinfo->modinfo); + $this->assertArrayHasKey($cm3->cmid, $coursemodinfo->modinfo); + $this->assertArrayHasKey($cm4->cmid, $coursemodinfo->modinfo); + // Make sure that the cacherev will be reset. + $this->assertEquals(-1, $coursemodinfo->cacherev); + } + + /** + * Purge a multiple course modules from the cache. + * + * @return void + * @covers \course_modinfo::purge_course_modules_cache + */ + public function test_purge_multiple_course_modules(): void { + $this->resetAfterTest(); + $this->setAdminUser(); + $cache = cache::make('core', 'coursemodinfo'); + + // Generate the course and pre-requisite section. + $course = $this->getDataGenerator()->create_course(); + $cm1 = $this->getDataGenerator()->create_module('page', ['course' => $course]); + $cm2 = $this->getDataGenerator()->create_module('page', ['course' => $course]); + $cm3 = $this->getDataGenerator()->create_module('page', ['course' => $course]); + $cm4 = $this->getDataGenerator()->create_module('page', ['course' => $course]); + // Reset course cache. + rebuild_course_cache($course->id, true); + // Build course cache. + get_fast_modinfo($course->id); + // Get the course modinfo cache. + $coursemodinfo = $cache->get_versioned($course->id, $course->cacherev); + $this->assertCount(4, $coursemodinfo->modinfo); + $this->assertArrayHasKey($cm1->cmid, $coursemodinfo->modinfo); + $this->assertArrayHasKey($cm2->cmid, $coursemodinfo->modinfo); + $this->assertArrayHasKey($cm3->cmid, $coursemodinfo->modinfo); + $this->assertArrayHasKey($cm4->cmid, $coursemodinfo->modinfo); + + course_modinfo::purge_course_modules_cache($course->id, [$cm2->cmid, $cm3->cmid]); + + $coursemodinfo = $cache->get_versioned($course->id, $course->cacherev); + $this->assertCount(2, $coursemodinfo->modinfo); + $this->assertArrayHasKey($cm1->cmid, $coursemodinfo->modinfo); + $this->assertArrayNotHasKey($cm2->cmid, $coursemodinfo->modinfo); + $this->assertArrayNotHasKey($cm3->cmid, $coursemodinfo->modinfo); + $this->assertArrayHasKey($cm4->cmid, $coursemodinfo->modinfo); + // Make sure that the cacherev will be reset. + $this->assertEquals(-1, $coursemodinfo->cacherev); + } + /** * Test get_cm() method to output course module id in the exception text. * diff --git a/lib/upgrade.txt b/lib/upgrade.txt index ac98d3a5ab1..fca092d669e 100644 --- a/lib/upgrade.txt +++ b/lib/upgrade.txt @@ -1,6 +1,10 @@ This files describes API changes in core libraries and APIs, information provided here is intended especially for developers. +=== 4.2.4 === +* course_modinfo now has a purge_course_modules_cache() method, which takes a list of cmids and purges + them all in a single cache set. + === 4.2.3 === * \moodle_page::set_title() has been updated to append the site name depending on the value of $CFG->sitenameintitle and whether the site's fullname/shortname has been set. So there's no need to manually add the site name whenever calling $PAGE->set_title().