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

make_categories_list is expensive in the case of large category trees

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 1.9
    • Fix Version/s: 1.9, 2.0
    • Component/s: Libraries
    • Labels:
      None
    • Affected Branches:
      MOODLE_19_STABLE
    • Fixed Branches:
      MOODLE_19_STABLE, MOODLE_20_STABLE

      Description

      make_categories_list is used a lot for simply populating a drop down list of categories in an incredibly expensive recursive fashion (150 database queries for me). Its used all over the shop, and notably on the course edit screen.

      get_categories can get near enough the same data with one query, so should look to change to use this or a new query..

        Gliffy Diagrams

          Attachments

            Activity

            Hide
            francois Francois Marier added a comment - - edited

            Here is a patch which uses a static variable to only do the category lookup once and to cache the categories in a hash of parent_category_id => array of child categories.

            On a site with 24 categories and 143 courses, I get the following reduction in total queries:

            • homepage: from 154 to 84 queries
            • course edit page: from 219 to 170 queries
            Show
            francois Francois Marier added a comment - - edited Here is a patch which uses a static variable to only do the category lookup once and to cache the categories in a hash of parent_category_id => array of child categories. On a site with 24 categories and 143 courses, I get the following reduction in total queries: homepage: from 154 to 84 queries course edit page: from 219 to 170 queries
            Hide
            martinlanghoff Martín Langhoff added a comment -
            Show
            martinlanghoff Martín Langhoff added a comment - This may also be relevant http://moodle.org/mod/forum/discuss.php?d=78366
            Hide
            martinlanghoff Martín Langhoff added a comment -

            Reviewed,I think this patch is good.

            From a "even better code" POV, I would prefer a patch that undid the silly recursion of make_categories_list... from a private discussion with Francois:

            I was thinking "call get_recordset() on the course_categories table, ordered by depth ASC, parent, ASC, sortorder ASC and just run a for loop", all happening straight in make_categories_list No recursion whatsoever.

            But we can't have the world at our feet every day and on every patch. This patch is good stuff.

            Show
            martinlanghoff Martín Langhoff added a comment - Reviewed,I think this patch is good. From a "even better code" POV, I would prefer a patch that undid the silly recursion of make_categories_list... from a private discussion with Francois: I was thinking "call get_recordset() on the course_categories table, ordered by depth ASC, parent, ASC, sortorder ASC and just run a for loop", all happening straight in make_categories_list No recursion whatsoever. But we can't have the world at our feet every day and on every patch. This patch is good stuff.
            Hide
            francois Francois Marier added a comment -

            Patch committed to 1.9 and trunk.

            Show
            francois Francois Marier added a comment - Patch committed to 1.9 and trunk.
            Hide
            wagner139 Andreas Wagner added a comment - - edited

            Great work, thank you.

            I found, that in the file course/index.php we have the function "print_category_edit()", which prints the categories recursively, when you do Site Administration->Courses->Add/Edit courses.

            Shoudn't we change the Line:

            if ($categories = get_categories($category->id)) { // Print all the children recursively

            to

            if ($categories = get_child_categories($category->id)) { // Print all the children recursively

            too?

            Show
            wagner139 Andreas Wagner added a comment - - edited Great work, thank you. I found, that in the file course/index.php we have the function "print_category_edit()", which prints the categories recursively, when you do Site Administration->Courses->Add/Edit courses. Shoudn't we change the Line: if ($categories = get_categories($category->id)) { // Print all the children recursively to if ($categories = get_child_categories($category->id)) { // Print all the children recursively too?

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  3/Mar/08