Moodle
  1. Moodle
  2. MDL-11938

make_categories_list is expensive in the case of large category trees

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor 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

          Activity

          Hide
          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 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
          Martín Langhoff added a comment -
          Show
          Martín Langhoff added a comment - This may also be relevant http://moodle.org/mod/forum/discuss.php?d=78366
          Hide
          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
          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 Marier added a comment -

          Patch committed to 1.9 and trunk.

          Show
          Francois Marier added a comment - Patch committed to 1.9 and trunk.
          Hide
          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
          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: