Moodle
  1. Moodle
  2. MDL-34785

Course overview block courselimit > MAX_MODINFO_CACHE_SIZE and causes frequent partial resets of the get_fast_modinfo cache

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: 2.1, 2.2, 2.3.6, 2.4, 2.5
    • Fix Version/s: 2.4.6, 2.5.2
    • Component/s: Blocks, Course, My home
    • Labels:
    • Testing Instructions:
      Hide
      1. Create a course with several modules in it, such as assignments, forums, quizzes, chat, lesson
      2. Enrol student and make sure that student's /my/ page shows activities requiring attention.
      3. Duplicate the course or create several similar ones. Should be at least 5 courses but the more the better.
      4. Add in your config.php
        define('MAX_MODINFO_CACHE_SIZE', 10);
      5. Repeat this step 3 times by setting the value to more than, equal to and less than half the number of courses on student's /my/ page, make sure the content of the page is not changing. Copy and save the page performance information.
      6. Revert the patch and compare the performance information with the previously saved results (with the same MAX_MODINFO_CACHE_SIZE setting)
      Show
      Create a course with several modules in it, such as assignments, forums, quizzes, chat, lesson Enrol student and make sure that student's /my/ page shows activities requiring attention. Duplicate the course or create several similar ones. Should be at least 5 courses but the more the better. Add in your config.php define('MAX_MODINFO_CACHE_SIZE', 10); Repeat this step 3 times by setting the value to more than, equal to and less than half the number of courses on student's /my/ page, make sure the content of the page is not changing. Copy and save the page performance information. Revert the patch and compare the performance information with the previously saved results (with the same MAX_MODINFO_CACHE_SIZE setting)
    • Workaround:
      Hide

      Add

      define('MAX_MODINFO_CACHE_SIZE', 21);
      

      in config.php

      Show
      Add define('MAX_MODINFO_CACHE_SIZE', 21); in config.php
    • Affected Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE, MOODLE_23_STABLE, MOODLE_24_STABLE, MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE, MOODLE_25_STABLE
    • Pull 2.4 Branch:
      wip-MDL-34785-m24
    • Pull 2.5 Branch:
      wip-MDL-34785-m25
    • Pull Master Branch:
      wip-MDL-34785-master
    • Rank:
      43276

      Description

      The course overview block used frequently on the /my page has a default $courseslimit of 21 which means that it will show up to 21 courses on the /my page at a time.

      The way in which this works means that get_fast_modinfo is called several times for each course (foreach module; foreach course), however, get_fast_modinfo tries to ensure that it doesn't run away with RAM and after MAX_MODINFO_CACHE_SIZE courses are found, it starts to remove entries.

      The default value for MAX_MODINFO_CACHE_SIZE is 10, which means that if a user is enrolled on > 9 courses (+ the site itself), then the get_fast_modinfo cache is partially reset frequently, and on the next iteration, the data has to be re-requested massively reducing performance.

      We should either:

      • increase MAX_MODINFO_CACHE_SIZE; or
      • decrease $courseslimit
      1. master_2_wo_patch.png
        48 kB
      2. master_2.png
        49 kB
      3. master_20_wo_patch.png
        48 kB
      4. master_20.png
        49 kB
      5. master_5_wo_patch.png
        48 kB
      6. master_5.png
        49 kB

        Issue Links

          Activity

          Hide
          Andrew Nicols added a comment -

          Raising priority of this because it has a massive effect on performance on larger sites.

          Show
          Andrew Nicols added a comment - Raising priority of this because it has a massive effect on performance on larger sites.
          Hide
          Sam Marshall added a comment -

          Note: The largest reason for the MAX_MODINFO_CACHE_SIZE limit is to avoid memory problems in cron, although it also affects some other areas. For this restriction, a limit of 20 (or even 21... why 21?) would probably be adequate.

          I would suggest that, using one of these /my/ pages and with performance info turned on, hopefully on a real 'larger site' or copy of one, somebody looks at the max memory usage with MAX_MODINFO_CACHE set to 10 and then with it set to 21. If the amount of extra memory used is basically OK, could just change it to 21. (Had a quick look at our sites but the /my/ page is disabled on ours, as is the mentioned block, so would be easier for somebody else to do this test.)

          At the time of changing it, it might also be worth changing the course overview block so that it is specifically defined to rely on the same constant and nobody changes them independently in future. (Although this is subject to change when the caching is rewritten.) Or if this is not done, then at least a comment.

          Show
          Sam Marshall added a comment - Note: The largest reason for the MAX_MODINFO_CACHE_SIZE limit is to avoid memory problems in cron, although it also affects some other areas. For this restriction, a limit of 20 (or even 21... why 21?) would probably be adequate. I would suggest that, using one of these /my/ pages and with performance info turned on, hopefully on a real 'larger site' or copy of one, somebody looks at the max memory usage with MAX_MODINFO_CACHE set to 10 and then with it set to 21. If the amount of extra memory used is basically OK, could just change it to 21. (Had a quick look at our sites but the /my/ page is disabled on ours, as is the mentioned block, so would be easier for somebody else to do this test.) At the time of changing it, it might also be worth changing the course overview block so that it is specifically defined to rely on the same constant and nobody changes them independently in future. (Although this is subject to change when the caching is rewritten.) Or if this is not done, then at least a comment.
          Hide
          Nathan Mares added a comment -

          Here's some stats from a large site (25000+ courses) where I've made this change to 21. Big improvement in terms of database queries but it certainly costs a bit on the memory front.

          Before:

          RAM: 118.4MB
          RAM peak: 160.5MB
          Included 185 files
          Contexts for which filters were loaded: 1
          Filters created: 3
          Pieces of content filtered: 0
          Strings filtered: 0
          get_string calls: 341
          strings mem cache hits: 311
          strings disk cache hits: 30
          DB reads/writes: 37919/0
          ticks: 3841 user: 2279 sys: 372 cuser: 0 csys: 0
          Load average: 0.10
          Session: 153.8KB

          After:

          RAM: 186.5MB
          RAM peak: 191.8MB
          Included 185 files
          Contexts for which filters were loaded: 1
          Filters created: 3
          Pieces of content filtered: 0
          Strings filtered: 0
          get_string calls: 341
          strings mem cache hits: 311
          strings disk cache hits: 30
          DB reads/writes: 685/0
          ticks: 662 user: 230 sys: 32 cuser: 0 csys: 0
          Load average: 0.04
          Session: 153.8KB

          There's a comment in the code at the moment suggesting the number of courses should be a config setting for the block. The reason it fetches 21 at the moment is so that it knows whether there are more courses than the 20 that get displayed.

          Show
          Nathan Mares added a comment - Here's some stats from a large site (25000+ courses) where I've made this change to 21. Big improvement in terms of database queries but it certainly costs a bit on the memory front. Before: RAM: 118.4MB RAM peak: 160.5MB Included 185 files Contexts for which filters were loaded: 1 Filters created: 3 Pieces of content filtered: 0 Strings filtered: 0 get_string calls: 341 strings mem cache hits: 311 strings disk cache hits: 30 DB reads/writes: 37919/0 ticks: 3841 user: 2279 sys: 372 cuser: 0 csys: 0 Load average: 0.10 Session: 153.8KB After: RAM: 186.5MB RAM peak: 191.8MB Included 185 files Contexts for which filters were loaded: 1 Filters created: 3 Pieces of content filtered: 0 Strings filtered: 0 get_string calls: 341 strings mem cache hits: 311 strings disk cache hits: 30 DB reads/writes: 685/0 ticks: 662 user: 230 sys: 32 cuser: 0 csys: 0 Load average: 0.04 Session: 153.8KB There's a comment in the code at the moment suggesting the number of courses should be a config setting for the block. The reason it fetches 21 at the moment is so that it knows whether there are more courses than the 20 that get displayed.
          Hide
          Nathan Mares added a comment -

          Actually no - it does print out 21, not 20.. I'm also curious as to why 21

          Show
          Nathan Mares added a comment - Actually no - it does print out 21, not 20.. I'm also curious as to why 21
          Hide
          Russell Smith added a comment -

          All versions still suffer from this issue. 2.4 and 2.5 now have a configuration item to set the number of courses to show when printing the overview block. If that number is greater than the current 10 for MAX_MODINFO_CACHE_SIZE, then you will see a slow course overview page.

          Options were discussed earlier in the comment thread. What are the best options now?

          The course overview block will ideally need the number of cached entries that match the number of courses displayed. MDL-19430 made the limit configurable, how does that tie into resolving this issue?

          Show
          Russell Smith added a comment - All versions still suffer from this issue. 2.4 and 2.5 now have a configuration item to set the number of courses to show when printing the overview block. If that number is greater than the current 10 for MAX_MODINFO_CACHE_SIZE, then you will see a slow course overview page. Options were discussed earlier in the comment thread. What are the best options now? The course overview block will ideally need the number of cached entries that match the number of courses displayed. MDL-19430 made the limit configurable, how does that tie into resolving this issue?
          Hide
          Russell Smith added a comment -

          MDL-36248 is the exact same issue as here. We are just describing the symptoms slightly differently.

          Show
          Russell Smith added a comment - MDL-36248 is the exact same issue as here. We are just describing the symptoms slightly differently.
          Hide
          Marina Glancy added a comment - - edited

          Hi. MAX_MODINFO_CACHE_SIZE will be removed in 2.6 under MDL-34397 because this cache is moved to MUC. (edited 29/07: it most likely won't be removed)
          In versions 2.3-2.5 admin can set MAX_MODINFO_CACHE_SIZE in config.php
          It is not only defined in modinfolib if it was not defined before, see https://github.com/moodle/moodle/blob/MOODLE_23_STABLE/lib/modinfolib.php#L27 for version 2.3, the code is the same till now.

          Show
          Marina Glancy added a comment - - edited Hi. MAX_MODINFO_CACHE_SIZE will be removed in 2.6 under MDL-34397 because this cache is moved to MUC. (edited 29/07: it most likely won't be removed) In versions 2.3-2.5 admin can set MAX_MODINFO_CACHE_SIZE in config.php It is not only defined in modinfolib if it was not defined before, see https://github.com/moodle/moodle/blob/MOODLE_23_STABLE/lib/modinfolib.php#L27 for version 2.3, the code is the same till now.
          Hide
          Marina Glancy added a comment -

          My solution is to split the list of courses in the block into bunches with no more than MAX_MODINFO_CACHE_SIZE courses in each bunch and then run modules callbacks for each bunch.

          The attached patch makes performance better... usually.
          It is definitely better for mod_assign, mod_chat, mod_lesson, mod_quiz, mod_scorm.
          Not so awesome for mod_assignment and mod_forum because they make queries for the whole list of courses, so when we split the course list in bunches they have to make those queries again (but for another list of courses).

          Overall I think it is better than it is now for stable versions. I'm not submitting it for integration yet because there may be better solution for master inside MDL-34397

          Show
          Marina Glancy added a comment - My solution is to split the list of courses in the block into bunches with no more than MAX_MODINFO_CACHE_SIZE courses in each bunch and then run modules callbacks for each bunch. The attached patch makes performance better... usually. It is definitely better for mod_assign, mod_chat, mod_lesson, mod_quiz, mod_scorm. Not so awesome for mod_assignment and mod_forum because they make queries for the whole list of courses, so when we split the course list in bunches they have to make those queries again (but for another list of courses). Overall I think it is better than it is now for stable versions. I'm not submitting it for integration yet because there may be better solution for master inside MDL-34397
          Hide
          Russell Smith added a comment - - edited

          I've run up some performance numbers on a user that was failing with OOM on their /my/ page. I will warn, I back ported this to 2.3 as that is our current environment. The following shows the basic results;

          All data as reported by XHProf, - for Batch size means before batches code was implemented.

          CACHE_SIZE Batch Size Runtime Memory
          10 - 40487.536 ms 58207.555 KB
          21 - 4865.916 ms 121615.578 KB
          10 10 6295.027 ms 58868 KB
          21 21 4991.463 ms 121650 KB

          So we get an increase in the load time. However it is of very little consequence compared with the thrashing time of the original implementation. Memory is much more constrained when you limit the cache to 10. Which has an especially large impact in the course overview block. But it should help constrain memory usage in other places as well.

          I've also confirmed the order of display of items with the course batches is correct as Marina indicated. I vote this is a very nice solution to the problem presented.

          Show
          Russell Smith added a comment - - edited I've run up some performance numbers on a user that was failing with OOM on their /my/ page. I will warn, I back ported this to 2.3 as that is our current environment. The following shows the basic results; All data as reported by XHProf, - for Batch size means before batches code was implemented. CACHE_SIZE Batch Size Runtime Memory 10 - 40487.536 ms 58207.555 KB 21 - 4865.916 ms 121615.578 KB 10 10 6295.027 ms 58868 KB 21 21 4991.463 ms 121650 KB So we get an increase in the load time. However it is of very little consequence compared with the thrashing time of the original implementation. Memory is much more constrained when you limit the cache to 10. Which has an especially large impact in the course overview block. But it should help constrain memory usage in other places as well. I've also confirmed the order of display of items with the course batches is correct as Marina indicated. I vote this is a very nice solution to the problem presented.
          Hide
          Russell Smith added a comment -

          My only other peer review comment is;

           for ($i = 0; $i * MAX_MODINFO_CACHE_SIZE < count($courses); $i++) { 
          

          I thought count() in a loop was frowned upon because of the cost of a count and the fact it's evaluated every loop. A variable might be better.

          Show
          Russell Smith added a comment - My only other peer review comment is; for ($i = 0; $i * MAX_MODINFO_CACHE_SIZE < count($courses); $i++) { I thought count() in a loop was frowned upon because of the cost of a count and the fact it's evaluated every loop. A variable might be better.
          Hide
          Marina Glancy added a comment -

          Hi Russell, thanks for test. 40s for /my/ page is just killing! But 5-6 is a lot as well... I wonder what can we do about it.

          So you have 21 courses on /my/ page, is that right? Why is there any difference then there at all after the patch in case of MAX_MODINFO_CACHE_SIZE = 21 ?

          Show
          Marina Glancy added a comment - Hi Russell, thanks for test. 40s for /my/ page is just killing! But 5-6 is a lot as well... I wonder what can we do about it. So you have 21 courses on /my/ page, is that right? Why is there any difference then there at all after the patch in case of MAX_MODINFO_CACHE_SIZE = 21 ?
          Hide
          Marina Glancy added a comment -

          I assume that it was peer reviewed then. Even though the button was not actually pressed in the tracker. I changed count($courses) to the variable, thanks Russell. Submitting for integration.

          Show
          Marina Glancy added a comment - I assume that it was peer reviewed then. Even though the button was not actually pressed in the tracker. I changed count($courses) to the variable, thanks Russell. Submitting for integration.
          Hide
          Sam Hemelryk added a comment -

          Hi guys,

          After looking at this assuming I'm reading it correctly my only qualm is why you didn't use array_chunk?
          I'd much rather see array_chunk being used rather than array_slice within a for loop unless there is a reason for the latter. However this isn't a requirement, I do think it would help readability but as its a performance improvement it is fine to land it as is.
          Leaving this in integration review in progress, let me know if I've mis-understood, and if not whether you'd like to fix it or whether you'd like it to go in as it is.

          Many thanks
          Sam

          Show
          Sam Hemelryk added a comment - Hi guys, After looking at this assuming I'm reading it correctly my only qualm is why you didn't use array_chunk? I'd much rather see array_chunk being used rather than array_slice within a for loop unless there is a reason for the latter. However this isn't a requirement, I do think it would help readability but as its a performance improvement it is fine to land it as is. Leaving this in integration review in progress, let me know if I've mis-understood, and if not whether you'd like to fix it or whether you'd like it to go in as it is. Many thanks Sam
          Hide
          Marina Glancy added a comment -

          Hi Sam, thanks, I just forgot about this function. Commits corrected.

          Show
          Marina Glancy added a comment - Hi Sam, thanks, I just forgot about this function. Commits corrected.
          Hide
          Sam Hemelryk added a comment -

          Thanks Marina - this has been integrated now

          Show
          Sam Hemelryk added a comment - Thanks Marina - this has been integrated now
          Hide
          Russell Smith added a comment -

          It may be a bit late now as it's been integrated, but I'll say it anyway.

          array_chunk handles all cases whether the input array is bigger, or smaller than the chunk size. I believe the constant MAX_MODINFO_CACHE_SIZE must be defined, or other things break.

          So doesn't;

           $batches = array_chunk($courses, MAX_MODINFO_CACHE_SIZE, true);
          

          Do the job without the if, count of courses and the like. It's much simpler code again and provides the required results.

          Show
          Russell Smith added a comment - It may be a bit late now as it's been integrated, but I'll say it anyway. array_chunk handles all cases whether the input array is bigger, or smaller than the chunk size. I believe the constant MAX_MODINFO_CACHE_SIZE must be defined, or other things break. So doesn't; $batches = array_chunk($courses, MAX_MODINFO_CACHE_SIZE, true ); Do the job without the if, count of courses and the like. It's much simpler code again and provides the required results.
          Hide
          Mark Nelson added a comment -

          I have attached my results, though really I think my data size was too small to indicate any success. What I can say is that it didn't break.

          The name of the files start with the version I tested on (only attached master, if you want the results for 2.5 and 2.4 I can attach as well) and then the value I set MAX_MODINFO_CACHE_SIZE to. The wording 'wo_patch' indicates it was tested without the patch. I had 5 courses with 5 activities in each.

          Show
          Mark Nelson added a comment - I have attached my results, though really I think my data size was too small to indicate any success. What I can say is that it didn't break. The name of the files start with the version I tested on (only attached master, if you want the results for 2.5 and 2.4 I can attach as well) and then the value I set MAX_MODINFO_CACHE_SIZE to. The wording 'wo_patch' indicates it was tested without the patch. I had 5 courses with 5 activities in each.
          Hide
          Mark Nelson added a comment -

          Sorry, I uploaded the wrong screen shots.

          Show
          Mark Nelson added a comment - Sorry, I uploaded the wrong screen shots.
          Hide
          Sam Hemelryk added a comment -

          Huzzah, your code made it into Moodle. Perhaps now things are ever so slightly better!

          "The ship can't take this much pressure. Sometimes it falls apart just sitting in the hangar."
          ~ Professor Farnsworth

          Show
          Sam Hemelryk added a comment - Huzzah, your code made it into Moodle. Perhaps now things are ever so slightly better! "The ship can't take this much pressure. Sometimes it falls apart just sitting in the hangar." ~ Professor Farnsworth

            People

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

              Dates

              • Created:
                Updated:
                Resolved: