Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-31918

Course Completion report performs recursive db queries

    Details

    • Testing Instructions:
      Hide

      Note to tester: If this is integrated at the same time as MDL-31914, the test instructions for that bug should mostly cover testing here. Functionality should not change with implementation of this change, just reduced database query load.

      • Enable 'Completion Tracking' in the Site Administration -> Advanced Features page
      • In a course, choose Edit Settings, and under 'Student Progress' change Completion tracking to Enabled and save changed
      • In an activity, choose Edit settings and, under 'Activity Completion', change Completion tracking to 'Show activity as complete when conditions are met', tick the 'Require view' checkbox and save changed
      • Open Settings Block -> Course Administration -> Completion Tracking
        • Under 'Activites Completed' check the new activity you just enabled
        • save changed
      • Open Reports -> Course Completion
      • Confirm:
        • there are no errors
        • That the module meta-data is correct (e.g. name, id)

      Functionality should not have changed with this patch

      Show
      Note to tester: If this is integrated at the same time as MDL-31914 , the test instructions for that bug should mostly cover testing here. Functionality should not change with implementation of this change, just reduced database query load. Enable 'Completion Tracking' in the Site Administration -> Advanced Features page In a course, choose Edit Settings, and under 'Student Progress' change Completion tracking to Enabled and save changed In an activity, choose Edit settings and, under 'Activity Completion', change Completion tracking to 'Show activity as complete when conditions are met', tick the 'Require view' checkbox and save changed Open Settings Block -> Course Administration -> Completion Tracking Under 'Activites Completed' check the new activity you just enabled save changed Open Reports -> Course Completion Confirm: there are no errors That the module meta-data is correct (e.g. name, id) Functionality should not have changed with this patch
    • Affected Branches:
      MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-31918-master-2

      Description

      Line 581 of report/completion/index.php calls get_record inside two foreach loops.

      This could be better achieved with something more like:

      $modinfo = get_fast_modinfo($course);
       
      foreach ($progress as $user) {
        foreach($criteria as $criterion) {
          $activity = $modinfo->cms[$criterion->moduleinstance];
        }
      }

        Gliffy Diagrams

          Attachments

            Activity

            Hide
            dobedobedoh Andrew Nicols added a comment - - edited

            This should reduce the number of queries by (count($progress) - a bit) queries or thereabouts where $progress is an array of users on the course

            On a course we have with 16 criteria and 486 users, we're seeing 1468 queries.
            Applying this patch reduces this to 1043.

            It should also be possibly to remove another (count($progress) queries by caching the result of $criterion->get_mod_instance() which is called in loops twice for activity criteria but I'm not sure on the best way to go about caching this. On one hand it would be helpful to have a completionlib function similar to get_fast_modinfo, but on the other, we can just do something like:

            $criteriacache = array();
            ...
            foreach ($criteria as $criterion) {
              switch ($criterion->criteriatype) {
                case COMPLETION_CRITERIA_TYPE_ACTIVITY:
                  $activity = $criterion->get_mod_instance();
                  $criteriacache[$criterion->criteriatype]->{$criterion->id} = $activity;
                  break;
            ...
              }
            }
             
            foreach ($progress as $user) {
              foreach ($criteria as $criterion) {
                $mod = $criteriacache[$criterion->criteriatype]->{$criterion->id};
              }
            }

            Show
            dobedobedoh Andrew Nicols added a comment - - edited This should reduce the number of queries by (count($progress) - a bit) queries or thereabouts where $progress is an array of users on the course On a course we have with 16 criteria and 486 users, we're seeing 1468 queries. Applying this patch reduces this to 1043. It should also be possibly to remove another (count($progress) queries by caching the result of $criterion->get_mod_instance() which is called in loops twice for activity criteria but I'm not sure on the best way to go about caching this. On one hand it would be helpful to have a completionlib function similar to get_fast_modinfo, but on the other, we can just do something like: $criteriacache = array(); ... foreach ($criteria as $criterion) { switch ($criterion->criteriatype) { case COMPLETION_CRITERIA_TYPE_ACTIVITY: $activity = $criterion->get_mod_instance(); $criteriacache[$criterion->criteriatype]->{$criterion->id} = $activity; break; ... } }   foreach ($progress as $user) { foreach ($criteria as $criterion) { $mod = $criteriacache[$criterion->criteriatype]->{$criterion->id}; } }
            Hide
            dobedobedoh Andrew Nicols added a comment -

            The patch so far should be good to go.

            If anyone has any thoughts on the criteria caching that would also be good though it may be more appropriate to create a new issue for this element.

            Show
            dobedobedoh Andrew Nicols added a comment - The patch so far should be good to go. If anyone has any thoughts on the criteria caching that would also be good though it may be more appropriate to create a new issue for this element.
            Hide
            poltawski Dan Poltawski added a comment -

            Hi Andrew,

            Have you tested that this is returning he same results and that fast_modinfo has the right info? It looks like your change has a regression as you are using $criterion->id rather than $criterion->moduleinstance to retrieve the $cm info.

            Show
            poltawski Dan Poltawski added a comment - Hi Andrew, Have you tested that this is returning he same results and that fast_modinfo has the right info? It looks like your change has a regression as you are using $criterion->id rather than $criterion->moduleinstance to retrieve the $cm info.
            Hide
            dobedobedoh Andrew Nicols added a comment -

            Thanks for that Dan - indeed, I had made a mistake there and should have been using $criterion->moduleinstance.

            I've updated the patch to remove further queries. On double-checking that the data supplied by get_fast_modinfo was sufficient:

            Data about a single module on a course. This contains most of the fields in the course_modules
            table, plus additional data when required.

            I additionally discovered that the function returns the module name, so there's no need to call $criterion->get_mod_instance() just to fetch the ->name on it. This removes another count($progress) queries.

            On our staging environment this brings page queries to 602 down from 1468.

            Show
            dobedobedoh Andrew Nicols added a comment - Thanks for that Dan - indeed, I had made a mistake there and should have been using $criterion->moduleinstance. I've updated the patch to remove further queries. On double-checking that the data supplied by get_fast_modinfo was sufficient: Data about a single module on a course. This contains most of the fields in the course_modules table, plus additional data when required. I additionally discovered that the function returns the module name, so there's no need to call $criterion->get_mod_instance() just to fetch the ->name on it. This removes another count($progress) queries. On our staging environment this brings page queries to 602 down from 1468.
            Hide
            dobedobedoh Andrew Nicols added a comment -

            Incidentally, these are the values of $activity that are used:

            1919 moodle:MDL-31918-master-2> git grep '$activity' report/completion/index.php
            report/completion/index.php:            $activity = $modinfo->cms[$criterion->moduleinstance];
            report/completion/index.php:            if (array_key_exists($activity->id,$user->progress)) {
            report/completion/index.php:                $thisprogress=$user->progress[$activity->id];
            report/completion/index.php:                ($activity->completion==COMPLETION_TRACKING_AUTOMATIC ? 'auto' : 'manual').
            report/completion/index.php:            $a->activity=strip_tags($activity->name);

            So that's:

            • id
            • completion
            • name

            All three are present in the data returned by get_fast_modinfo

            Show
            dobedobedoh Andrew Nicols added a comment - Incidentally, these are the values of $activity that are used: 1919 moodle:MDL-31918-master-2> git grep '$activity' report/completion/index.php report/completion/index.php: $activity = $modinfo->cms[$criterion->moduleinstance]; report/completion/index.php: if (array_key_exists($activity->id,$user->progress)) { report/completion/index.php: $thisprogress=$user->progress[$activity->id]; report/completion/index.php: ($activity->completion==COMPLETION_TRACKING_AUTOMATIC ? 'auto' : 'manual'). report/completion/index.php: $a->activity=strip_tags($activity->name); So that's: id completion name All three are present in the data returned by get_fast_modinfo
            Hide
            dobedobedoh Andrew Nicols added a comment -

            Hi Dan,

            I've fixed the regression you found but I believe that otherwise get_fast_modinfo is more than sufficient.

            I've updated the patch slightly to use the new $modinfo object to reduce queries further so I'd appreciate if you could give it another glance before I submit for IR.

            Andrew

            Show
            dobedobedoh Andrew Nicols added a comment - Hi Dan, I've fixed the regression you found but I believe that otherwise get_fast_modinfo is more than sufficient. I've updated the patch slightly to use the new $modinfo object to reduce queries further so I'd appreciate if you could give it another glance before I submit for IR. Andrew
            Hide
            poltawski Dan Poltawski added a comment -

            Looks good - please submit thanks!

            Show
            poltawski Dan Poltawski added a comment - Looks good - please submit thanks!
            Hide
            dobedobedoh Andrew Nicols added a comment -

            This should cherry-pick cleanly to:

            • master
            • MOODLE_22_STABLE
            • MOODLE_21_STABLE
            Show
            dobedobedoh Andrew Nicols added a comment - This should cherry-pick cleanly to: master MOODLE_22_STABLE MOODLE_21_STABLE
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Some hours ago...

            the main moodle.git repository has been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

            TIA and ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Some hours ago... the main moodle.git repository has been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
            Hide
            samhemelryk Sam Hemelryk added a comment -

            Thanks Andrew, this has been integrated now!

            Show
            samhemelryk Sam Hemelryk added a comment - Thanks Andrew, this has been integrated now!
            Hide
            phalacee Jason Fowler added a comment -

            All good

            Show
            phalacee Jason Fowler added a comment - All good
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            FCT (fixed, closing, thanks). Ciao

            "I feel a very unusual sensation - if it is not indigestion, I think it must be gratitude!"
            ~ Benjamin Disraeli

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - FCT (fixed, closing, thanks). Ciao "I feel a very unusual sensation - if it is not indigestion, I think it must be gratitude!" ~ Benjamin Disraeli

              People

              • Votes:
                0 Vote for this issue
                Watchers:
                2 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  14/May/12