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
    • Rank:
      17536

      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..

        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: