Moodle
  1. Moodle
  2. MDL-31867

coursetag_get_tagged_courses is horribly inefficient

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.2.1, 2.3
    • Fix Version/s: 2.3
    • Component/s: Tags
    • Labels:
      None
    • Testing Instructions:
      Hide
      1. This test instruction is basically for testing for regressions, there is no easy way to measure the performance here.
      2. Goto admin/search.php?query=tag and enable "enable site tags" and "Show course tags" if not already enabled
      3. Goto site>pages>tags>manage tags> Add around 15-20 different tags of different type
      4. Goto site home page and add a "tags" block. Goto settings of the block and configure it to be displayed "through out the site"
      5. Goto atleast 4 different courses and randomly add a few tags to each of them from the list or you can use new ones if you like
      6. Go back to tags search page and search for some of the tags that you added to the course.
      7. click on the tag and view the result.
      8. Make sure you see all courses that has that tag listed there and you get no errors or warnings.
      Show
      This test instruction is basically for testing for regressions, there is no easy way to measure the performance here. Goto admin/search.php?query=tag and enable "enable site tags" and "Show course tags" if not already enabled Goto site>pages>tags>manage tags> Add around 15-20 different tags of different type Goto site home page and add a "tags" block. Goto settings of the block and configure it to be displayed "through out the site" Goto atleast 4 different courses and randomly add a few tags to each of them from the list or you can use new ones if you like Go back to tags search page and search for some of the tags that you added to the course. click on the tag and view the result. Make sure you see all courses that has that tag listed there and you get no errors or warnings.
    • Affected Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE
    • Pull Master Branch:
      MDL-31867-master
    • Rank:
      38510

      Description

      While reviewing MDL-31466 I noted that the coursetag_get_tagged_courses function was using way more queries than it would required.
      Quoting my comment there (because I am lazy):

      Just noting that I will open a new issue to improve this bit of code which is truly awful.
      It is selecting all tag instances used in a course and then for each is fetching the course referenced by the itemid (the only field used) after which if the course is hidden it matches another query to get the context.
      Presently the number of queries = 1 + number of courses + hidden courses.
      I'm 99% positive you could reduce this to a single query by joining to the course table in the initial select and then pre-loading the contexts in the same query.

        Activity

        Hide
        Sam Hemelryk added a comment -

        Ok I havn't tested it but I spent 2 minutes and wrote the code:

        /**
         * Get courses tagged with a tag
         *
         * @global moodle_database $DB
         * @package  core_tag
         * @category tag
         * @param int $tagid
         * @return array of course objects
         */
        function coursetag_get_tagged_courses($tagid) {
            global $DB;
        
            $courses = array();
        
            $ctxselect = context_helper::get_preload_record_columns_sql('ctx');
        
            $sql = "SELECT c.*, $ctxselect
                      FROM {course} c
                      JOIN {tag_instance} t ON t.itemid = c.id
                      JOIN {context} ctx ON ctx.instanceid = c.id
                     WHERE t.tagid = :tagid AND
                           t.itemtype = 'course' AND
                           ctx.contextlevel = :contextlevel
                  ORDER BY c.sortorder ASC";
            $params = array(
                'tagid' => $tagid,
                'contextlevel' => CONTEXT_COURSE
            );
            $rs = $DB->get_recordset_sql($sql, $params);
            foreach ($rs as $course) {
                context_helper::preload_from_record($course);
                if ($course->visible == 1 || has_capability('moodle/course:viewhiddencourses', context_course::instance($course->id))) {
                    $courses[$c->itemid] = $course;
                }
            }
            return $courses;
        }
        

        Should just about do it!

        Show
        Sam Hemelryk added a comment - Ok I havn't tested it but I spent 2 minutes and wrote the code: /** * Get courses tagged with a tag * * @global moodle_database $DB * @ package core_tag * @category tag * @param int $tagid * @ return array of course objects */ function coursetag_get_tagged_courses($tagid) { global $DB; $courses = array(); $ctxselect = context_helper::get_preload_record_columns_sql('ctx'); $sql = "SELECT c.*, $ctxselect FROM {course} c JOIN {tag_instance} t ON t.itemid = c.id JOIN {context} ctx ON ctx.instanceid = c.id WHERE t.tagid = :tagid AND t.itemtype = 'course' AND ctx.contextlevel = :contextlevel ORDER BY c.sortorder ASC"; $params = array( 'tagid' => $tagid, 'contextlevel' => CONTEXT_COURSE ); $rs = $DB->get_recordset_sql($sql, $params); foreach ($rs as $course) { context_helper::preload_from_record($course); if ($course->visible == 1 || has_capability('moodle/course:viewhiddencourses', context_course::instance($course->id))) { $courses[$c->itemid] = $course; } } return $courses; } Should just about do it!
        Hide
        Ankit Agarwal added a comment -

        Thanks Sam for explaining context_helper stuff.
        Patch looks great.
        will do some testing just to make sure nothing is broken
        Thanks

        Show
        Ankit Agarwal added a comment - Thanks Sam for explaining context_helper stuff. Patch looks great. will do some testing just to make sure nothing is broken Thanks
        Hide
        Ankit Agarwal added a comment -

        Hi Sam,
        I made a few small changes to your patch to fix some minor issues.

        Now submitting this for peer-review
        Thanks

        Show
        Ankit Agarwal added a comment - Hi Sam, I made a few small changes to your patch to fix some minor issues. Now submitting this for peer-review Thanks
        Hide
        Rossiani Wijaya added a comment -

        This looks good Ankit.
        +1 for integration.

        Show
        Rossiani Wijaya added a comment - This looks good Ankit. +1 for integration.
        Hide
        Ankit Agarwal added a comment -

        Great.
        Thanks Rosie for the review.
        Submitting for integration.
        @integrators
        Master only

        Thanks

        Show
        Ankit Agarwal added a comment - Great. Thanks Rosie for the review. Submitting for integration. @integrators Master only Thanks
        Hide
        Eloy Lafuente (stronk7) added a comment -

        The main moodle.git repository has just 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
        Eloy Lafuente (stronk7) added a comment - The main moodle.git repository has just 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
        Ankit Agarwal added a comment -

        rebased
        Thanks

        Show
        Ankit Agarwal added a comment - rebased Thanks
        Hide
        Aparup Banerjee added a comment -

        Thanks, this looks like a definite improvement, integrated into master only and ready for testing.

        Show
        Aparup Banerjee added a comment - Thanks, this looks like a definite improvement, integrated into master only and ready for testing.
        Hide
        Dan Poltawski added a comment -

        Found some strict standards errors unrelated to this change, but otherwise find. Passing

        Show
        Dan Poltawski added a comment - Found some strict standards errors unrelated to this change, but otherwise find. Passing
        Hide
        Aparup Banerjee added a comment -

        The code here has been spread to upstream moodle repositories and mirrors for anyone to use .

        Closing, have a good weekend!

        Show
        Aparup Banerjee added a comment - The code here has been spread to upstream moodle repositories and mirrors for anyone to use . Closing, have a good weekend!

          People

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

            Dates

            • Created:
              Updated:
              Resolved: