Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: 2.4.3
    • Fix Version/s: 2.4.4
    • Component/s: Course
    • Labels:
    • Testing Instructions:
      Hide

      1. Run the unit tests.

      2. Do some basic CRUD operations on some courses in a range of formats, to ensure there are not obvious regressions.

      Test 3 (performance):

      1. Create course in topics format (having the default format weeks)
      2. View the course, make sure that performance info on course/view.php is better than before change
      Show
      1. Run the unit tests. 2. Do some basic CRUD operations on some courses in a range of formats, to ensure there are not obvious regressions. Test 3 (performance): Create course in topics format (having the default format weeks) View the course, make sure that performance info on course/view.php is better than before change
    • Affected Branches:
      MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE
    • Pull from Repository:
    • Pull 2.4 Branch:
    • Pull Master Branch:
    • Rank:
      50110

      Description

      There are hundreds of calls to format_base::instance on a typical course, and that function is very expensive (despite its internal caching).

        Issue Links

          Activity

          Hide
          Tim Hunt added a comment -

          Marina, please could you take a look at this.

          Performance change looking a a moderately sized course page as student:

          Before: 135 calls to format_base::get_format_or_default, 8.5% of the page-load time.

          After: 135 calls to format_base::get_format_or_default, 0.7% of the page-load time.

          Show
          Tim Hunt added a comment - Marina, please could you take a look at this. Performance change looking a a moderately sized course page as student: Before: 135 calls to format_base::get_format_or_default, 8.5% of the page-load time. After: 135 calls to format_base::get_format_or_default, 0.7% of the page-load time.
          Hide
          Gareth J Barnard added a comment - - edited

          Tested with V2.5.0.3 of Collapsed Topics on a 52 section course and the page load is around 1 second quicker on rough average. Using FireBug.

          Show
          Gareth J Barnard added a comment - - edited Tested with V2.5.0.3 of Collapsed Topics on a 52 section course and the page load is around 1 second quicker on rough average. Using FireBug.
          Hide
          Damyon Wiese added a comment -

          Nice patch - this is the kind of thing that is hard to pick up on a dev site - but on a real site chock full of data really helps.

          Maybe it could be even faster still:

          $plugins = get_sorted_course_formats();
          if (in_array($format, $plugins)) {
          + self::$classesforformat[$format] = $format;

          Why not cache all the $plugins so get_sorted_course_formats only gets called once?

          Show
          Damyon Wiese added a comment - Nice patch - this is the kind of thing that is hard to pick up on a dev site - but on a real site chock full of data really helps. Maybe it could be even faster still: $plugins = get_sorted_course_formats(); if (in_array($format, $plugins)) { + self::$classesforformat [$format] = $format; Why not cache all the $plugins so get_sorted_course_formats only gets called once?
          Hide
          Marina Glancy added a comment -

          Hi!
          Thanks for noticing it Tim.
          Do I understand correctly that get_sorted_course_formats() is causing problems?
          If yes, I would better do caching inside it.

          Show
          Marina Glancy added a comment - Hi! Thanks for noticing it Tim. Do I understand correctly that get_sorted_course_formats() is causing problems? If yes, I would better do caching inside it.
          Hide
          Tim Hunt added a comment -

          Damyon, good idea, I will amend that patch.

          Also, this was really easy to pick up on a dev site. That is where I found it. You just had to look at the profiling data.

          Marina, actually, I did that first, then changed it. Think about what the two functions are doing:

          1. format_base::get_format_or_default -> given then name of a selected course format from the DB, decide which real course format to instantiate.

          2. get_sorted_course_formats -> get the list of all installed course formats, in order, which involves scanning a directory, and applying a lot of unnecessary configuration.

          Really, there is no need to be using 2. to do 1. I guess that was just a quick and easy way to write the code. 1. is the think that needs to be fast, so it was easiest to put the static cache there.

          1. should really just check the one requested course format. It should not load all of them. If you did move the caching back into 2. then 1. would continue to load the list of all course formats every time, which is silly. So, anyway, I think the caching belongs in 1.

          Show
          Tim Hunt added a comment - Damyon, good idea, I will amend that patch. Also, this was really easy to pick up on a dev site. That is where I found it. You just had to look at the profiling data. Marina, actually, I did that first, then changed it. Think about what the two functions are doing: 1. format_base::get_format_or_default -> given then name of a selected course format from the DB, decide which real course format to instantiate. 2. get_sorted_course_formats -> get the list of all installed course formats, in order, which involves scanning a directory, and applying a lot of unnecessary configuration. Really, there is no need to be using 2. to do 1. I guess that was just a quick and easy way to write the code. 1. is the think that needs to be fast, so it was easiest to put the static cache there. 1. should really just check the one requested course format. It should not load all of them. If you did move the caching back into 2. then 1. would continue to load the list of all course formats every time, which is silly. So, anyway, I think the caching belongs in 1.
          Hide
          Marina Glancy added a comment -

          Hi Tim,
          ok. I really think that the checking availability and enable-ability is the slow point, not sorting. But since this function is not used often except here this is up to you to decide.

          Show
          Marina Glancy added a comment - Hi Tim, ok. I really think that the checking availability and enable-ability is the slow point, not sorting. But since this function is not used often except here this is up to you to decide.
          Hide
          Marina Glancy added a comment -

          which reminds me of MDL-35727 where I plan to get rid of static cache...

          Show
          Marina Glancy added a comment - which reminds me of MDL-35727 where I plan to get rid of static cache...
          Hide
          Tim Hunt added a comment -

          Commits amended based on Damyon's comments. Submitting for integration now.

          Show
          Tim Hunt added a comment - Commits amended based on Damyon's comments. Submitting for integration now.
          Hide
          Dan Poltawski added a comment -

          The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

          TIA and ciao

          Show
          Dan Poltawski added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
          Hide
          Damyon Wiese added a comment -

          The patch looks fine Tim, but I couldn't get that warning to display using those instructions - I think you might even have to do some database twiddling to get that to happen. They are not important to the patch because it looks to have the same behaviour as before so I have removed those last 2 parts form the instructions.

          Show
          Damyon Wiese added a comment - The patch looks fine Tim, but I couldn't get that warning to display using those instructions - I think you might even have to do some database twiddling to get that to happen. They are not important to the patch because it looks to have the same behaviour as before so I have removed those last 2 parts form the instructions.
          Hide
          Marina Glancy added a comment -

          Damyon, it was my testing instruction. Yes probably one needs to physically remove format from disc to get the warning

          Show
          Marina Glancy added a comment - Damyon, it was my testing instruction. Yes probably one needs to physically remove format from disc to get the warning
          Hide
          Tim Hunt added a comment -

          Yes, I was going to say, thanks Maria for extending the testing instructions.

          Show
          Tim Hunt added a comment - Yes, I was going to say, thanks Maria for extending the testing instructions.
          Hide
          Damyon Wiese added a comment -

          Thanks Tim, this has been integrated to 2.4 and master.

          Show
          Damyon Wiese added a comment - Thanks Tim, this has been integrated to 2.4 and master.
          Hide
          Andrew Davis added a comment -

          Its difficult to be entirely sure but, as far as I can tell, this is working fine. Passing.

          Show
          Andrew Davis added a comment - Its difficult to be entirely sure but, as far as I can tell, this is working fine. Passing.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Did you think this day was not going to arrive ever?

          Your patience has been rewarded, yay, sent upstream, thanks!

          Closing...ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Did you think this day was not going to arrive ever? Your patience has been rewarded, yay, sent upstream, thanks! Closing...ciao

            People

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

              Dates

              • Created:
                Updated:
                Resolved: