Moodle
  1. Moodle
  2. MDL-36035

reduce database queries made by print_whole_category_list()

    Details

    • Testing Instructions:
      Hide

      Test functionality:

      • Requires a site with several categories and courses
      1. Note down the category structure and which courses are in each category before the patch.
      2. Add course categories, courses, and combo list to your front page.
      3. Update the site with the patch.
      4. Check that everything is in the exact same place it was before.

      Test performance.

      • Create 100+ course categories, several levels deep.
      • Generate courses for each (if you are able to)
      • Set maxcategorydepth = Unlimited
      • Set the front page setting to show course categories, courses and combo list on the front page.
      • Compare database query count with and without this patch

      Regarding data generation if you don't have a large database already you may want to script the generation. You can make use of the PHPUnit data generator if you want.

      Show
      Test functionality: Requires a site with several categories and courses Note down the category structure and which courses are in each category before the patch. Add course categories, courses, and combo list to your front page. Update the site with the patch. Check that everything is in the exact same place it was before. Test performance. Create 100+ course categories, several levels deep. Generate courses for each (if you are able to) Set maxcategorydepth = Unlimited Set the front page setting to show course categories, courses and combo list on the front page. Compare database query count with and without this patch Regarding data generation if you don't have a large database already you may want to script the generation. You can make use of the PHPUnit data generator if you want.
    • Affected Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE, MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE
    • Pull Master Branch:
      wip-MDL-36035-m25
    • Rank:
      44796

      Description

      Currently, print_whole_category_list can use one database query per category that is displayed (it calls get_courses()).

      https://github.com/brki/moodle/tree/mdl23-reduce-db-queries-for-print_whole_category_list provides a solution that significantly reduces database queries for sites that have a large number of course categories.

      On a Moodle with 1252 course categories and maxcategorydepth = Unlimited, this reduces the database reads from 1313 to 62 for course/index.php.

        Activity

        Hide
        Brian King added a comment -

        Added 2.3 diff URL.

        Show
        Brian King added a comment - Added 2.3 diff URL.
        Hide
        Petr Škoda added a comment -

        Thanks for the report and proposed patch.

        Show
        Petr Škoda added a comment - Thanks for the report and proposed patch.
        Hide
        Brian King added a comment -

        Could someone please review this code?

        Show
        Brian King added a comment - Could someone please review this code?
        Hide
        Rex Lorenzo added a comment -

        Brian, rather than creating new functions to do the flattening, take a look at the array_walk_recursive method (http://stackoverflow.com/a/1320156/6001).

        Show
        Rex Lorenzo added a comment - Brian, rather than creating new functions to do the flattening, take a look at the array_walk_recursive method ( http://stackoverflow.com/a/1320156/6001 ).
        Hide
        Brian King added a comment - - edited

        Hi Rex,

        thanks for the suggestion. I looked at that, but it doesn't work well, as far I can tell, with array_walk_recursive, because the structure here is a recursive array of objects with arrays of objects etc., and not pure arrays.

        I also looked at array_walk using a recursive function, but I had to use some ugly non-reader-friendly syntax (e.g. the function has to accept the third parameter by reference - ok, but it doesn't work if that third parameter is directly an array, only if it's an object containing an array).

        Show
        Brian King added a comment - - edited Hi Rex, thanks for the suggestion. I looked at that, but it doesn't work well, as far I can tell, with array_walk_recursive, because the structure here is a recursive array of objects with arrays of objects etc., and not pure arrays. I also looked at array_walk using a recursive function, but I had to use some ugly non-reader-friendly syntax (e.g. the function has to accept the third parameter by reference - ok, but it doesn't work if that third parameter is directly an array, only if it's an object containing an array).
        Hide
        Sam Hemelryk added a comment -

        Thanks Brian.

        I've looked this over and it does look like a clear improvement.
        Shortly after this I'll push up a branch I have created during my peer-review to submit for integration.
        It takes what you've done and makes just a couple of small changes in order to get it in line with our style guidelines.
        Changes are as follows:

        1. Renamed the new functions.
        2. Tidied up the PHPdocs for each function.
        3. Fixed up whitespace issues.

        I should also mention that as this is an improvement it will likely only land in master.
        Once it has been integrated if we want we can create a new issue to backport this after it has been in core for a while and has been proved to not cause regressions.
        Given this appears to be a good performance win I will do just that.

        Many thanks
        Sam

        Show
        Sam Hemelryk added a comment - Thanks Brian. I've looked this over and it does look like a clear improvement. Shortly after this I'll push up a branch I have created during my peer-review to submit for integration. It takes what you've done and makes just a couple of small changes in order to get it in line with our style guidelines. Changes are as follows: Renamed the new functions. Tidied up the PHPdocs for each function. Fixed up whitespace issues. I should also mention that as this is an improvement it will likely only land in master. Once it has been integrated if we want we can create a new issue to backport this after it has been in core for a while and has been proved to not cause regressions. Given this appears to be a good performance win I will do just that. Many thanks Sam
        Hide
        Sam Hemelryk added a comment -

        Up for integration now.

        Integrators. Master only at the moment, once integrated I will create a new issue to consider this for backporting.

        Show
        Sam Hemelryk added a comment - Up for integration now. Integrators. Master only at the moment, once integrated I will create a new issue to consider this for backporting.
        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
        Petr Škoda added a comment -

        nice!

        Show
        Petr Škoda added a comment - nice!
        Hide
        Dan Poltawski added a comment -

        Integrated to master, thanks a lot Brian & Sam!

        Show
        Dan Poltawski added a comment - Integrated to master, thanks a lot Brian & Sam!
        Hide
        Frédéric Massart added a comment -

        I think this issue is causing the following error on /course/index.php:

        Fatal error: Call to undefined function get_category_courses() in /home/fred/www/repositories/im/moodle/course/lib.php on line 1583 Call Stack: 0.0013 904984 1. {main}() /home/fred/www/repositories/im/moodle/course/index.php:0 0.2490 55206384 2. print_whole_category_list() /home/fred/www/repositories/im/moodle/course/index.php:87 
        
        Show
        Frédéric Massart added a comment - I think this issue is causing the following error on /course/index.php: Fatal error: Call to undefined function get_category_courses() in /home/fred/www/repositories/im/moodle/course/lib.php on line 1583 Call Stack: 0.0013 904984 1. {main}() /home/fred/www/repositories/im/moodle/course/index.php:0 0.2490 55206384 2. print_whole_category_list() /home/fred/www/repositories/im/moodle/course/index.php:87
        Hide
        Jason Fowler added a comment -

        Wasn't able to test performance, but the regression testing passed.

        Show
        Jason Fowler added a comment - Wasn't able to test performance, but the regression testing passed.
        Hide
        Rajesh Taneja added a comment -

        I am also seeing error reported by Fred.

        Fatal error: Call to undefined function get_category_courses() in /var/www/im/course/lib.php on line 1428 Call Stack: 0.0006 905320 1. {main}() /var/www/im/course/index.php:0 0.8359 79730480 2. print_whole_category_list() /var/www/im/course/index.php:87 
        

        Seems it's not fixed yet.
        Steps to reproduce:

        1. Log in as admin
        2. Click on courses or go to MOODLE_SITE/course/index.php
        Show
        Rajesh Taneja added a comment - I am also seeing error reported by Fred. Fatal error: Call to undefined function get_category_courses() in /var/www/im/course/lib.php on line 1428 Call Stack: 0.0006 905320 1. {main}() /var/www/im/course/index.php:0 0.8359 79730480 2. print_whole_category_list() /var/www/im/course/index.php:87 Seems it's not fixed yet. Steps to reproduce: Log in as admin Click on courses or go to MOODLE_SITE/course/index.php
        Hide
        Frédéric Massart added a comment -

        I'm assuming get_category_courses() have to be renamed to get_category_courses_array().

        Show
        Frédéric Massart added a comment - I'm assuming get_category_courses() have to be renamed to get_category_courses_array().
        Hide
        Sam Hemelryk added a comment -

        Hi guys,

        It was indeed a typo for which I've integrated a fix now.
        Could you please retest for me and just check things are now fine.

        Many thanks
        Sam

        Show
        Sam Hemelryk added a comment - Hi guys, It was indeed a typo for which I've integrated a fix now. Could you please retest for me and just check things are now fine. Many thanks Sam
        Hide
        Jason Fowler added a comment -

        Testing again now. Not sure what I did that avoided the problem, but I'll test more areas this time.

        Show
        Jason Fowler added a comment - Testing again now. Not sure what I did that avoided the problem, but I'll test more areas this time.
        Hide
        Jason Fowler added a comment -

        From what I can see the problem has been fixed. Again, still have no idea what I did that avoided the problem, I had been creating courses and categories during testing yesterday.

        Show
        Jason Fowler added a comment - From what I can see the problem has been fixed. Again, still have no idea what I did that avoided the problem, I had been creating courses and categories during testing yesterday.
        Hide
        Dan Poltawski added a comment -

        Hurray! We did it! Thanks to all the reporters, testers, user and watchers for a bumper week of Moodling!

        Show
        Dan Poltawski added a comment - Hurray! We did it! Thanks to all the reporters, testers, user and watchers for a bumper week of Moodling!

          People

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

            Dates

            • Created:
              Updated:
              Resolved: