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

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

          Issue Links

            Activity

            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.
            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
            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 .
            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.
            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.
            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
            Hide
            skodak Petr Skoda added a comment -

            ooops, I meant DEV backlog

            Show
            skodak Petr Skoda added a comment - ooops, I meant DEV backlog
            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.

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved: