Moodle
  1. Moodle
  2. MDL-39536

Extending navigation from report progress is inefficient

    Details

    • Rank:
      50215

      Description

      While debugging MDL-37773, I came across report_progress_extend_navigation_course() which is highly inefficient when it comes to adding nodes to the navigation.

      If activity completion is enabled on the course, then it gets all the activities just to ensure that at least one has completion enabled. This is a massive processing where a simple DB->record_exists would be just enough.

      $showonnavigation = ($showonnavigation && $completion->is_enabled() && ($completion->get_activities())>0);
      

      Will call:

          public function get_activities($modinfo=null) {
              global $DB;
      
              // Obtain those activities which have completion turned on
              $withcompletion = $DB->get_records_select('course_modules', 'course='.$this->course->id.
                ' AND completion<>'.COMPLETION_TRACKING_NONE);
              if (!$withcompletion) {
                  return array();
              }
      
              // Use modinfo to get section order and also add in names
              if (empty($modinfo)) {
                  $modinfo = get_fast_modinfo($this->course);
              }
              $result = array();
              foreach ($modinfo->sections as $sectioncms) {
                  foreach ($sectioncms as $cmid) {
                      if (array_key_exists($cmid, $withcompletion)) {
                          $result[$cmid] = $withcompletion[$cmid];
                          $result[$cmid]->modname = $modinfo->cms[$cmid]->modname;
                          $result[$cmid]->name    = $modinfo->cms[$cmid]->name;
                      }
                  }
              }
      
              return $result;
          }
      

      loading modinfo for each module.

        Issue Links

          Activity

          Hide
          Frédéric Massart added a comment -

          I've added a patch for master, but before pushing it forward I would like to create unit tests for it. Though it's a bit mysterious and so I'll delay that to later. Meantime I'll leave the default assignee as it had been set.

          Show
          Frédéric Massart added a comment - I've added a patch for master, but before pushing it forward I would like to create unit tests for it. Though it's a bit mysterious and so I'll delay that to later. Meantime I'll leave the default assignee as it had been set.
          Hide
          Sam Marshall added a comment -

          Hi, I agree this is really bad - don't understand why it was done that way. I don't think the suggested fix is the best one though.

          What I don't understand is - that function already uses get_fast_modinfo - modinfo includes the completion information that it's checking, so why isn't it using that? That would be much faster than the record_exists, never mind the get_records_sql, as it doesn't need to make any database queries because get_fast_modinfo will already be cached.

          I think the best fix would probably be:

          1. Change get_activities so that it uses modinfo to do that first check instead of the db query.
          2. Create has_activities function as per your patch, but make this just call get_activities now that it probably isn't a performance problem. (Then it can easily be optimised later if profiling identifies a problem).

          Agree that a unit test would be a good idea.

          Do you want me to code this or are you happy to keep working on it?

          Show
          Sam Marshall added a comment - Hi, I agree this is really bad - don't understand why it was done that way. I don't think the suggested fix is the best one though. What I don't understand is - that function already uses get_fast_modinfo - modinfo includes the completion information that it's checking, so why isn't it using that? That would be much faster than the record_exists, never mind the get_records_sql, as it doesn't need to make any database queries because get_fast_modinfo will already be cached. I think the best fix would probably be: 1. Change get_activities so that it uses modinfo to do that first check instead of the db query. 2. Create has_activities function as per your patch, but make this just call get_activities now that it probably isn't a performance problem. (Then it can easily be optimised later if profiling identifies a problem). Agree that a unit test would be a good idea. Do you want me to code this or are you happy to keep working on it?
          Hide
          Frédéric Massart added a comment -

          I don't mind fixing this, but you seem to know much more about the area and modinfo than I do. If you don't have much time, would you mind guiding me a bit into fixing that? I understand that modinfo is the way to go. Would you just foreach over the modules of the course and check if there is any condition set on a (visible?) module? Should this logic be part of modinfo, or get_activities() is the right place?

          Cheers,
          Fred

          Show
          Frédéric Massart added a comment - I don't mind fixing this, but you seem to know much more about the area and modinfo than I do. If you don't have much time, would you mind guiding me a bit into fixing that? I understand that modinfo is the way to go. Would you just foreach over the modules of the course and check if there is any condition set on a (visible?) module? Should this logic be part of modinfo, or get_activities() is the right place? Cheers, Fred
          Hide
          Sam Marshall added a comment -

          I wrote the current version of modinfo (obviously based on what was there before, so it was more restructuring than writing from scratch), so yes I do know a fair bit about it, although my memory isn't great!

          I think get_activities is the right place for it as the change is basically information that's needed as part of the completion system.

          What I was thinking of was just to call $modinfo->get_cms(), loop through the result, and see which have ->completion set to anything other than zero (COMPLETION_TRACKING_NONE); put those in the array it was making. Even if there are on average 100 activities per course, this loop should probably still be faster than a database query, and it's better to keep the number of database queries down because in large performance-sensitive Moodle deployments, it's easy to use multiple web servers but more difficult to scale the database server.

          Does that make sense?

          Show
          Sam Marshall added a comment - I wrote the current version of modinfo (obviously based on what was there before, so it was more restructuring than writing from scratch), so yes I do know a fair bit about it, although my memory isn't great! I think get_activities is the right place for it as the change is basically information that's needed as part of the completion system. What I was thinking of was just to call $modinfo->get_cms(), loop through the result, and see which have ->completion set to anything other than zero (COMPLETION_TRACKING_NONE); put those in the array it was making. Even if there are on average 100 activities per course, this loop should probably still be faster than a database query, and it's better to keep the number of database queries down because in large performance-sensitive Moodle deployments, it's easy to use multiple web servers but more difficult to scale the database server. Does that make sense?
          Hide
          Frédéric Massart added a comment - - edited

          Thanks Sam, it seems to make sense. I assigned this issue to me but I don't think I'll have time to work on it during the following days. Feel free to grab it if you need/want to. Cheers!

          Show
          Frédéric Massart added a comment - - edited Thanks Sam, it seems to make sense. I assigned this issue to me but I don't think I'll have time to work on it during the following days. Feel free to grab it if you need/want to. Cheers!
          Hide
          Frédéric Massart added a comment -

          Requesting peer review.

          So here are 4 branches with unit tests attached. I have created a new test file because I could not make use of the existing one, unless I rewrote the whole class not to use DB mock stuff. I have also added/improved has_activities() so that it does its own loop, and return true as soon as it can. And the whole thing makes use of the recommended method of modinfo.

          Cheers,
          Fred

          Show
          Frédéric Massart added a comment - Requesting peer review. So here are 4 branches with unit tests attached. I have created a new test file because I could not make use of the existing one, unless I rewrote the whole class not to use DB mock stuff. I have also added/improved has_activities() so that it does its own loop, and return true as soon as it can. And the whole thing makes use of the recommended method of modinfo. Cheers, Fred
          Hide
          Sam Marshall added a comment -

          Thanks, I have looked through the code and I didn't spot anything wrong at all!

          Just one comment about the manual testing instructions - aren't these incorrect i.e. the report should only appear if there is actually at least one activity that actually uses completion, so adding an activity or changing activity settings should be part of the test?

          The unit tests pass (I ran both the old and new tests), so please just fix the manual instructions in this issue, then go ahead and submit for integration.

          [Y] Syntax
          [-] Output
          [Y] Whitespace
          [-] Language
          [-] Databases
          [N] Testing - Y once manual instructions corrected
          [-] Security
          [-] Documentation
          [Y] Git
          [Y] Sanity check

          Please go ahead and submit for integration.

          Show
          Sam Marshall added a comment - Thanks, I have looked through the code and I didn't spot anything wrong at all! Just one comment about the manual testing instructions - aren't these incorrect i.e. the report should only appear if there is actually at least one activity that actually uses completion, so adding an activity or changing activity settings should be part of the test? The unit tests pass (I ran both the old and new tests), so please just fix the manual instructions in this issue, then go ahead and submit for integration. [Y] Syntax [-] Output [Y] Whitespace [-] Language [-] Databases [N] Testing - Y once manual instructions corrected [-] Security [-] Documentation [Y] Git [Y] Sanity check Please go ahead and submit for integration.
          Hide
          Dan Poltawski added a comment -

          (I see Fred updated the testing instructions but didn't comment )

          Show
          Dan Poltawski added a comment - (I see Fred updated the testing instructions but didn't comment )
          Hide
          Dan Poltawski added a comment - - edited

          Hi Fred,

          global $DB in has_activities()?

          Show
          Dan Poltawski added a comment - - edited Hi Fred, global $DB in has_activities()?
          Hide
          Frédéric Massart added a comment -

          Thanks Dan, I've amended the patches.

          Show
          Frédéric Massart added a comment - Thanks Dan, I've amended the patches.
          Hide
          Dan Poltawski added a comment -

          Integrated to master, 25, 24 and 23 - thanks Fred

          Show
          Dan Poltawski added a comment - Integrated to master, 25, 24 and 23 - thanks Fred
          Hide
          Andrew Davis added a comment -

          Works as described. Passing.

          Show
          Andrew Davis added a comment - Works as described. Passing.
          Hide
          Dan Poltawski added a comment -
          Feature: Thanks to our superb contributors
            In order to make Moodle better
            As an integrator
            I need to thank all our contributors
          
            Scenario: Dan thanks you all
              Given I log in as "dan"
              And I see "lots of fixed issues"
              When I follow "Close integrated issues"
              Then I should see "Lots of thanks to all our contributors"
          

          Your changes are upstream

          Show
          Dan Poltawski added a comment - Feature: Thanks to our superb contributors In order to make Moodle better As an integrator I need to thank all our contributors Scenario: Dan thanks you all Given I log in as "dan" And I see "lots of fixed issues" When I follow "Close integrated issues" Then I should see "Lots of thanks to all our contributors" Your changes are upstream

            People

            • Votes:
              1 Vote for this issue
              Watchers:
              5 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: