Moodle
  1. Moodle
  2. MDL-18808

print_category_info and so course/index.php calls 'select count(*) from mdl_course' multiple times rather than being passed to print_category_info

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Won't Fix
    • Affects Version/s: 1.9.4, 1.9.5, 2.0
    • Fix Version/s: None
    • Component/s: General, Performance
    • Database:
      PostgreSQL
    • Difficulty:
      Moderate
    • Affected Branches:
      MOODLE_19_STABLE, MOODLE_20_STABLE
    • Rank:
      16327

      Description

      Currently when you view course/index.php the print_category_info function is called for every category on the site.
      this function performs 'select count from mdl_course' every time it is called.

      This is not required and should ideally be passed as an argument to the function.
      this doesn't scale particularly well.

      1. count_record_patch.txt
        2 kB
        Mayank Jain
      2. moodle_2_count_record_performance.patch
        1.0 kB
        Jonathan Champ

        Issue Links

          Activity

          Rod Norfor created issue -
          Hide
          Mayank Jain added a comment -

          The number of courses is passed as a parameter to the function print_category_info.

          The number of courses is calculated only once during the first call to the recursive function print_whole_courses_list.

          Show
          Mayank Jain added a comment - The number of courses is passed as a parameter to the function print_category_info. The number of courses is calculated only once during the first call to the recursive function print_whole_courses_list.
          Mayank Jain made changes -
          Field Original Value New Value
          Attachment count_record_patch.txt [ 16906 ]
          Martin Dougiamas made changes -
          Workflow jira [ 31412 ] MDL Workflow [ 44757 ]
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Raising to critical, hoping to get this included in next performance-focussed sprint. Assigning to Sam, because there is also another issue potentially related with this about categories/courses performance in front page.

          Note this needs fix both for 1.9.x and 2.0.x.

          Thanks for the report, ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Raising to critical, hoping to get this included in next performance-focussed sprint. Assigning to Sam, because there is also another issue potentially related with this about categories/courses performance in front page. Note this needs fix both for 1.9.x and 2.0.x. Thanks for the report, ciao
          Eloy Lafuente (stronk7) made changes -
          Labels performance triaged
          Assignee moodle.com [ moodle.com ] Sam Hemelryk [ samhemelryk ]
          Fix Version/s STABLE backlog [ 10463 ]
          Priority Minor [ 4 ] Critical [ 2 ]
          Difficulty Difficult
          Component/s Performance [ 10221 ]
          Martin Dougiamas made changes -
          Fix Version/s STABLE Sprint 4 [ 10500 ]
          Fix Version/s STABLE backlog [ 10463 ]
          Rossiani Wijaya made changes -
          Assignee Sam Hemelryk [ samhemelryk ] Rossiani Wijaya [ rwijaya ]
          Hide
          Rossiani Wijaya added a comment -

          Working progress patch for 1.9 is available at: https://github.com/rwijaya/moodle/compare/MOODLE_19_STABLE...MDL-18808_m19.

          Show
          Rossiani Wijaya added a comment - Working progress patch for 1.9 is available at: https://github.com/rwijaya/moodle/compare/MOODLE_19_STABLE...MDL-18808_m19 .
          Rossiani Wijaya made changes -
          Status Open [ 1 ] In Progress [ 3 ]
          Martin Dougiamas made changes -
          Fix Version/s STABLE Sprint 5 [ 10520 ]
          Fix Version/s STABLE Sprint 4 [ 10500 ]
          Hide
          Rossiani Wijaya added a comment -

          add Sam to review the patch.

          Show
          Rossiani Wijaya added a comment - add Sam to review the patch.
          Hide
          Sam Marshall added a comment -

          Hi, patch for 1.9 looks good except note whitespace error here:

          function print_category_info($category, $depth, $showcourses = false,$numofcourses=0) {
                                                                              ^^^  
          

          HOWEVER you are right, it will only actually call this once anyhow because there is a static cache. So I'm not sure whether this is a performance problem after all ie maybe there is no need for this change? Or am I missing something...

          Show
          Sam Marshall added a comment - Hi, patch for 1.9 looks good except note whitespace error here: function print_category_info($category, $depth, $showcourses = false ,$numofcourses=0) { ^^^ HOWEVER you are right, it will only actually call this once anyhow because there is a static cache. So I'm not sure whether this is a performance problem after all ie maybe there is no need for this change? Or am I missing something...
          Hide
          Sam Marshall added a comment -

          The static cache was added in MDL-19179, a month after this issue was reported. Maybe this will be adequated, i.e. resolve this issue as dup of that one?

          Show
          Sam Marshall added a comment - The static cache was added in MDL-19179 , a month after this issue was reported. Maybe this will be adequated, i.e. resolve this issue as dup of that one?
          Hide
          Rossiani Wijaya added a comment -

          Thanks Sam for reviewing the patch.

          I fixed the patch for 1.9 and create patch for 2.0.

          git diff 1.9: https://github.com/rwijaya/moodle/compare/MOODLE_19_STABLE...MDL-18808_m19

          git diff 2.0: https://github.com/rwijaya/moodle/compare/master...MDL-18808-m20

          Show
          Rossiani Wijaya added a comment - Thanks Sam for reviewing the patch. I fixed the patch for 1.9 and create patch for 2.0. git diff 1.9: https://github.com/rwijaya/moodle/compare/MOODLE_19_STABLE...MDL-18808_m19 git diff 2.0: https://github.com/rwijaya/moodle/compare/master...MDL-18808-m20
          Hide
          Rossiani Wijaya added a comment -

          This issue is deferred.

          Show
          Rossiani Wijaya added a comment - This issue is deferred.
          Rossiani Wijaya made changes -
          Status In Progress [ 3 ] Ready for review [ 10010 ]
          Resolution Deferred [ 6 ]
          Rossiani Wijaya made changes -
          Link This issue has a non-specific relationship to MDL-26233 [ MDL-26233 ]
          Hide
          Jonathan Champ added a comment -

          It seems like count from any table should be static cached (and handled by the function that can determine whether it can be cached). This patch (moodle_2_count_record_performance) does exactly that by moving the static cache to the count_record() function itself. This will allow all secondary calls for all tables to be cached when they provide no conditions.

          Show
          Jonathan Champ added a comment - It seems like count from any table should be static cached (and handled by the function that can determine whether it can be cached). This patch (moodle_2_count_record_performance) does exactly that by moving the static cache to the count_record() function itself. This will allow all secondary calls for all tables to be cached when they provide no conditions.
          Jonathan Champ made changes -
          Hide
          Helen Foster added a comment -

          Jonathan, thanks for your comment.

          Rosi, just wondering whether this issue should be reopened?

          Show
          Helen Foster added a comment - Jonathan, thanks for your comment. Rosi, just wondering whether this issue should be reopened?
          Hide
          Petr Škoda added a comment - - edited

          Static caching inside instance methods is not acceptable, that would break really quickly:
          1/ the caches must be invalidated after any insert, delete or execution of sql
          2/ the cache must be separate for different driver instances
          3/ we have to deals with changes from different requests too, it would probably require an optional $cached parameter and use it wisely in code

          This leads to new protected property in driver which can be accessed by all member methods....

          Show
          Petr Škoda added a comment - - edited Static caching inside instance methods is not acceptable, that would break really quickly: 1/ the caches must be invalidated after any insert, delete or execution of sql 2/ the cache must be separate for different driver instances 3/ we have to deals with changes from different requests too, it would probably require an optional $cached parameter and use it wisely in code This leads to new protected property in driver which can be accessed by all member methods....
          Hide
          Petr Škoda added a comment -

          reopening and returning into stable backlog

          Show
          Petr Škoda added a comment - reopening and returning into stable backlog
          Petr Škoda made changes -
          Resolution Deferred [ 6 ]
          Status Ready for review [ 10010 ] Reopened [ 4 ]
          Assignee Rossiani Wijaya [ rwijaya ] moodle.com [ moodle.com ]
          Petr Škoda made changes -
          Fix Version/s STABLE backlog [ 10463 ]
          Difficulty Difficult Moderate
          Hide
          Petr Škoda added a comment -

          ooops, I meant DEV backlog

          Show
          Petr Škoda added a comment - ooops, I meant DEV backlog
          Petr Škoda made changes -
          Fix Version/s DEV backlog [ 10464 ]
          Fix Version/s STABLE backlog [ 10463 ]
          Eloy Lafuente (stronk7) made changes -
          Fix Version/s STABLE Sprint 5 [ 10520 ]
          Martin Dougiamas made changes -
          Workflow MDL Workflow [ 44757 ] MDL Full Workflow [ 76821 ]
          Hide
          Michael de Raadt added a comment -

          Thanks for reporting this issue.

          We have detected that this issue has been inactive for over a year. It was reported as affecting versions that are no longer supported.

          If you believe that this issue is still relevant to current versions (2.5 and beyond), please comment on the issue. Issues left inactive for a further month will be closed.

          Michael d.

          TW9vZGxlDQo=

          Show
          Michael de Raadt added a comment - Thanks for reporting this issue. We have detected that this issue has been inactive for over a year. It was reported as affecting versions that are no longer supported. If you believe that this issue is still relevant to current versions (2.5 and beyond), please comment on the issue. Issues left inactive for a further month will be closed. Michael d. TW9vZGxlDQo=
          Hide
          Michael de Raadt added a comment -

          I'm closing this issue as it has been inactive for over a year has been recorded as affecting versions that are no longer supported.

          This is being done as part of a bulk annual clean-up of issues.

          If you still believe this is an issue in supported versions, please create a new issue.

          Show
          Michael de Raadt added a comment - I'm closing this issue as it has been inactive for over a year has been recorded as affecting versions that are no longer supported. This is being done as part of a bulk annual clean-up of issues. If you still believe this is an issue in supported versions, please create a new issue.
          Michael de Raadt made changes -
          Status Reopened [ 4 ] Closed [ 6 ]
          Assignee moodle.com [ moodle.com ]
          Resolution Won't Fix [ 2 ]
          Fix Version/s DEV backlog [ 10464 ]

            People

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

              Dates

              • Created:
                Updated:
                Resolved: