Uploaded image for project: '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
    • Status: Closed
    • Priority: 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

      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.

        Gliffy Diagrams

          Attachments

            Issue Links

              Activity

              rtcn2 Rod Norfor created issue -
              Hide
              mayankjain20 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
              mayankjain20 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.
              mayankjain20 Mayank Jain made changes -
              Field Original Value New Value
              Attachment count_record_patch.txt [ 16906 ]
              dougiamas Martin Dougiamas made changes -
              Workflow jira [ 31412 ] MDL Workflow [ 44757 ]
              Hide
              stronk7 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
              stronk7 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
              stronk7 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 ]
              dougiamas Martin Dougiamas made changes -
              Fix Version/s STABLE Sprint 4 [ 10500 ]
              Fix Version/s STABLE backlog [ 10463 ]
              rwijaya Rossiani Wijaya made changes -
              Assignee Sam Hemelryk [ samhemelryk ] Rossiani Wijaya [ rwijaya ]
              Hide
              rwijaya 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
              rwijaya 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 .
              rwijaya Rossiani Wijaya made changes -
              Status Open [ 1 ] In Progress [ 3 ]
              dougiamas Martin Dougiamas made changes -
              Fix Version/s STABLE Sprint 5 [ 10520 ]
              Fix Version/s STABLE Sprint 4 [ 10500 ]
              Hide
              rwijaya Rossiani Wijaya added a comment -

              add Sam to review the patch.

              Show
              rwijaya Rossiani Wijaya added a comment - add Sam to review the patch.
              Hide
              quen 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
              quen 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
              quen 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
              quen 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
              rwijaya 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
              rwijaya 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
              rwijaya Rossiani Wijaya added a comment -

              This issue is deferred.

              Show
              rwijaya Rossiani Wijaya added a comment - This issue is deferred.
              rwijaya Rossiani Wijaya made changes -
              Status In Progress [ 3 ] Ready for review [ 10010 ]
              Resolution Deferred [ 6 ]
              rwijaya Rossiani Wijaya made changes -
              Link This issue has a non-specific relationship to MDL-26233 [ MDL-26233 ]
              Hide
              jrchamp 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
              jrchamp 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.
              jrchamp Jonathan Champ made changes -
              Hide
              tsala Helen Foster added a comment -

              Jonathan, thanks for your comment.

              Rosi, just wondering whether this issue should be reopened?

              Show
              tsala Helen Foster added a comment - Jonathan, thanks for your comment. Rosi, just wondering whether this issue should be reopened?
              Hide
              skodak Petr Skoda 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
              skodak Petr Skoda 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
              skodak Petr Skoda added a comment -

              reopening and returning into stable backlog

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

              ooops, I meant DEV backlog

              Show
              skodak Petr Skoda added a comment - ooops, I meant DEV backlog
              skodak Petr Skoda made changes -
              Fix Version/s DEV backlog [ 10464 ]
              Fix Version/s STABLE backlog [ 10463 ]
              stronk7 Eloy Lafuente (stronk7) made changes -
              Fix Version/s STABLE Sprint 5 [ 10520 ]
              dougiamas Martin Dougiamas made changes -
              Workflow MDL Workflow [ 44757 ] MDL Full Workflow [ 76821 ]
              Hide
              salvetore 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
              salvetore 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
              salvetore 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
              salvetore 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.
              salvetore 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: