From a9e634afa6c070714dfdb9bda545207032cc56a2 Mon Sep 17 00:00:00 2001 From: Damyon Wiese Date: Thu, 6 Mar 2014 14:00:38 +0800 Subject: [PATCH 1/2] MDL-43721 Assign: Static cache for plugin is_enabled and is_visible because they get used alot --- mod/assign/assignmentplugin.php | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) diff --git a/mod/assign/assignmentplugin.php b/mod/assign/assignmentplugin.php index 52ab2f90487..d128a26ecdf 100644 --- a/mod/assign/assignmentplugin.php +++ b/mod/assign/assignmentplugin.php @@ -42,7 +42,10 @@ abstract class assign_plugin { private $type = ''; /** @var string $error error message */ private $error = ''; - + /** @var boolean|null $enabledcache Cached lookup of the is_enabled function */ + private $enabledcache = null; + /** @var boolean|null $enabledcache Cached lookup of the is_visible function */ + private $visiblecache = null; /** * Constructor for the abstract plugin type class @@ -203,6 +206,7 @@ abstract class assign_plugin { * @return bool */ public final function enable() { + $this->enabledcache = true; return $this->set_config('enabled', 1); } @@ -212,6 +216,7 @@ abstract class assign_plugin { * @return bool */ public final function disable() { + $this->enabledcache = false; return $this->set_config('enabled', 0); } @@ -221,7 +226,10 @@ abstract class assign_plugin { * @return bool - if false - this plugin will not accept submissions / feedback */ public function is_enabled() { - return $this->get_config('enabled'); + if ($this->enabledcache === null) { + $this->enabledcache = $this->get_config('enabled'); + } + return $this->enabledcache; } @@ -282,8 +290,11 @@ abstract class assign_plugin { * @return bool */ public final function is_visible() { - $disabled = get_config($this->get_subtype() . '_' . $this->get_type(), 'disabled'); - return !$disabled; + if ($this->visiblecache === null) { + $disabled = get_config($this->get_subtype() . '_' . $this->get_type(), 'disabled'); + $this->visiblecache = !$disabled; + } + return $this->visiblecache; } From c46db93c28cbaef276c1fad6bc5331784d850998 Mon Sep 17 00:00:00 2001 From: Damyon Wiese Date: Wed, 26 Feb 2014 16:29:48 +0800 Subject: [PATCH 2/2] MDL-43721 Assign + groups: Improve performance of assign grading table Add a function to the groups lib to filter a list of users down to the ones who can see the module. Required because calling groups_course_module_visible() for a list of users is too slow and we shouldn't spread group logic outside of grouplib.php. Using it in the assign grading table reduces DB queries from 6198/1 to 256/3. This is 12secs down to 2.5secs. --- lib/grouplib.php | 71 +++++++++++++++++++++++++++++++++++++++++ mod/assign/locallib.php | 27 ++++++++++------ 2 files changed, 88 insertions(+), 10 deletions(-) diff --git a/lib/grouplib.php b/lib/grouplib.php index c698bf00f23..ac8c7d6ffda 100644 --- a/lib/grouplib.php +++ b/lib/grouplib.php @@ -848,6 +848,77 @@ function groups_get_activity_allowed_groups($cm,$userid=0) { } } +/** + * Filter a user list and return only the users that can see the course module based on + * groups/permissions etc. It is assumed that the users are pre-filtered to those who are enrolled in the course. + * + * @category group + * @param stdClass $cm The course module + * @param array $users An array of users, indexed by userid + * @return array A filtered list of users that can see the module, indexed by userid. + */ +function groups_filter_users_by_course_module_visible($cm, $users) { + global $CFG, $DB; + + if (empty($CFG->enablegroupmembersonly)) { + return $users; + } + if (empty($cm->groupmembersonly)) { + return $users; + } + list($usql, $uparams) = $DB->get_in_or_equal(array_keys($users), SQL_PARAMS_NAMED, 'userid', true); + + // Group membership sub-query. + if ($cm->groupingid) { + // Find out if member of any group in selected activity grouping. + $igsql = "SELECT gm.userid + FROM {groups_members} gm + LEFT JOIN {groupings_groups} gg + ON gm.groupid = gg.groupid + WHERE gm.userid $usql AND gg.groupingid = :groupingid"; + $igparams = array_merge($uparams, array('groupingid' => $cm->groupingid)); + + } else { + // No grouping used - check all groups in course. + $igsql = "SELECT gm.userid + FROM {groups_members} gm + LEFT JOIN {groups} g + ON gm.groupid = g.id + WHERE gm.userid $usql AND g.courseid = :courseid"; + $igparams = array_merge($uparams, array('courseid' => $cm->course)); + } + + $context = context_module::instance($cm->id); + + // Get the list of users in a valid group. + $usersingroup = $DB->get_records_sql($igsql, $igparams); + if (!$usersingroup) { + $usersingroup = array(); + } else { + $usersingroup = array_keys($usersingroup); + } + + // Get the list of users who can access all groups. + list($accessallgroupssql, $accessallgroupsparams) = get_enrolled_sql($context, 'moodle/site:accessallgroups'); + + $userswithaccessallgroups = $DB->get_records_sql($accessallgroupssql, $accessallgroupsparams); + if (!$userswithaccessallgroups) { + $userswithaccessallgroups = array(); + } else { + $userswithaccessallgroups = array_keys($userswithaccessallgroups); + } + + // Explaining this array mangling: + // $users is an array of $user[$userid] => stdClass etc + // $userswithaccessallgroups is an array of $userswithaccessallgroups[random int] = $userid + // $usersingroup is an array of $usersingroup[random int] = $userid + // so - the inner array_merge combines the values of the 2 arrays disregarding the keys (because they are ints) + // this is then flipped so the values become the keys (and the values are now nonsense) + // this is then intersected with the users array only by looking at the keys - this + // returns only the users from the original array that had a value in one of the two $usersxxx lists. + return array_intersect_key($users, array_flip(array_merge($userswithaccessallgroups, $usersingroup))); +} + /** * Determine if a course module is currently visible to a user * diff --git a/mod/assign/locallib.php b/mod/assign/locallib.php index c4e555ec144..7c1bc49aca0 100644 --- a/mod/assign/locallib.php +++ b/mod/assign/locallib.php @@ -131,6 +131,9 @@ class assign { /** @var array list of suspended user IDs in form of ([id1] => id1) */ public $susers = null; + /** @var array cached list of participants for this assignment. The cache key will be group, showactive and the context id */ + private $participants = array(); + /** * Constructor for the base assign class. * @@ -1313,22 +1316,26 @@ class assign { * @return array List of user records */ public function list_participants($currentgroup, $idsonly) { - if ($idsonly) { - $users = get_enrolled_users($this->context, 'mod/assign:submit', $currentgroup, 'u.id', null, null, null, - $this->show_only_active_users()); - } else { + $key = $this->context->id . '-' . $currentgroup . '-' . $this->show_only_active_users(); + if (!isset($this->participants[$key])) { $users = get_enrolled_users($this->context, 'mod/assign:submit', $currentgroup, 'u.*', null, null, null, $this->show_only_active_users()); + + $cm = $this->get_course_module(); + $users = groups_filter_users_by_course_module_visible($cm, $users); + + $this->participants[$key] = $users; } - $cm = $this->get_course_module(); - foreach ($users as $userid => $user) { - if (!groups_course_module_visible($cm, $userid)) { - unset($users[$userid]); + if ($idsonly) { + $idslist = array(); + foreach ($this->participants[$key] as $id => $user) { + $idslist[$id] = new stdClass(); + $idslist[$id]->id = $id; } + return $idslist; } - - return $users; + return $this->participants[$key]; } /**