Moodle
  1. Moodle
  2. MDL-41403

restore_search_base::search should use a recordset

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.5.1
    • Fix Version/s: 2.6
    • Component/s: Backup
    • Labels:
    • Testing Instructions:
      Hide
      1. Perform the first steps without the patch applied (eg. on stable).
      2. Create a site that contains 5000 courses and 1000 course categories.
      3. Visit <yoursite>/admin/settings.php?section=debugging and ensure 'Performance info' is checked (debugging should obviously also be on as well), so that you can see "RAM peak" in page footers.
      4. Backup one of the courses.
      5. Start a restore process using the backup you just created.
      6. When you're on the step "2 -> Destination" page, note the RAM peak.
      7. Reload the page a couple of times and note the RAM peak.
      8. Stay on that page and open another tab.
      9. Upgrade your branch to the latest integration and run the update in the tab you just opened.
      10. Once the update has finished you can close the tab and reload the step "2 -> Destination" page, note the RAM peak.
      11. Reload the page a few times and note the RAM peak.
      12. Confirm that the RAM peak is lower after applying the patch.
      13. Confirm that you can restore the course.
      Show
      Perform the first steps without the patch applied (eg. on stable). Create a site that contains 5000 courses and 1000 course categories. Visit <yoursite>/admin/settings.php?section=debugging and ensure 'Performance info' is checked (debugging should obviously also be on as well), so that you can see "RAM peak" in page footers. Backup one of the courses. Start a restore process using the backup you just created. When you're on the step "2 -> Destination" page, note the RAM peak. Reload the page a couple of times and note the RAM peak. Stay on that page and open another tab. Upgrade your branch to the latest integration and run the update in the tab you just opened. Once the update has finished you can close the tab and reload the step "2 -> Destination" page, note the RAM peak. Reload the page a few times and note the RAM peak. Confirm that the RAM peak is lower after applying the patch. Confirm that you can restore the course.
    • Affected Branches:
      MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_26_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      mdl26_MDL-41403_backup_reduce_memory_usage
    • Rank:
      52410

      Description

      Currently, search() calls $DB->get_records_sql with a limit of 5000. On Moodles that have 1000s of courses, this uses a chunk of memory.

      The proposed improvement uses get_recordset_sql instead, to keep memory usage down.

      On the Moodle instance I tested on (> 5000 courses, > 1200 course categories), this reduced PEAK RAM usage for this page from 118 MB to 101 MB - about 15% less memory.

        Activity

        Hide
        Brian King added a comment -

        If whitespace is not considered, the actual diff for the commit is quite small (and it's easier to see what's changed): https://github.com/brki/moodle/compare/mdl26_MDL-41403_backup_reduce_memory_usage?w=1

        Show
        Brian King added a comment - If whitespace is not considered, the actual diff for the commit is quite small (and it's easier to see what's changed): https://github.com/brki/moodle/compare/mdl26_MDL-41403_backup_reduce_memory_usage?w=1
        Hide
        Mark Nelson added a comment -

        Hi Brian,

        Thanks for your contribution.

        Two things -

        1. I made a few changes to your testing instructions to make it clearer on what had to be done. If you can check that they are accurate that would be great.
        2. Please put a full stop after the comments in the code. I know that these are not strings that you introduced, but since the diff does affect them it would be good if you could make these change so that they conform to Moodle coding guidelines. Note - "get it form first element" should be "get it from first element" (small typo).

        Other than those trivial points it looks good!

        Thanks again.

        Mark

        Show
        Mark Nelson added a comment - Hi Brian, Thanks for your contribution. Two things - I made a few changes to your testing instructions to make it clearer on what had to be done. If you can check that they are accurate that would be great. Please put a full stop after the comments in the code. I know that these are not strings that you introduced, but since the diff does affect them it would be good if you could make these change so that they conform to Moodle coding guidelines. Note - "get it form first element" should be "get it from first element" (small typo). Other than those trivial points it looks good! Thanks again. Mark
        Hide
        Mark Nelson added a comment -

        Also, because this is an improvement I believe it will only be applied to master.

        Show
        Mark Nelson added a comment - Also, because this is an improvement I believe it will only be applied to master.
        Hide
        Brian King added a comment -

        Hi Mark,

        thanks for looking at it and adjusting the testing instructions.

        I've deleted and recreated my master-based branch, it now also adjusts the existing comments as you mentioned.

        I've also removed the 2.4 and 2.5 based branches. It's easy to backport, let me know if they are wanted after all.

        Show
        Brian King added a comment - Hi Mark, thanks for looking at it and adjusting the testing instructions. I've deleted and recreated my master-based branch, it now also adjusts the existing comments as you mentioned. I've also removed the 2.4 and 2.5 based branches. It's easy to backport, let me know if they are wanted after all.
        Hide
        Mark Nelson added a comment -

        Thanks Brian! Pushing to integration review.

        Show
        Mark Nelson added a comment - Thanks Brian! Pushing to integration review.
        Hide
        Damyon Wiese added a comment -

        Thanks Brian,

        This looks correct to me. Integrated to master only.

        Show
        Damyon Wiese added a comment - Thanks Brian, This looks correct to me. Integrated to master only.
        Hide
        David Monllaó added a comment -

        Hi Jason,

        To create that much data you can use the new site generator (admin/tool/generator/maketestsite.php) although I would say that you might be interested in changing the following admin/tool/generator/classes/course_backend.php private attributes, otherwise you will need a whole day to generate that amount of data:

        • $paramsmallfilecount = array(1, 1, 1, 1, 1, 1);
        • $paramsmallfilesize = array(1, 1, 1, 1, 1, 1);
        • $parambigfilecount = array(1, 1, 1, 1, 1, 1);
        • $parambigfilesize = array(1, 1, 1, 1, 1, 1);
        Show
        David Monllaó added a comment - Hi Jason, To create that much data you can use the new site generator (admin/tool/generator/maketestsite.php) although I would say that you might be interested in changing the following admin/tool/generator/classes/course_backend.php private attributes, otherwise you will need a whole day to generate that amount of data: $paramsmallfilecount = array(1, 1, 1, 1, 1, 1); $paramsmallfilesize = array(1, 1, 1, 1, 1, 1); $parambigfilecount = array(1, 1, 1, 1, 1, 1); $parambigfilesize = array(1, 1, 1, 1, 1, 1);
        Hide
        Jason Fowler added a comment -

        14MB drop in memory for me, 123MB, down from 137MB. Good work. Thanks Brian.

        Show
        Jason Fowler added a comment - 14MB drop in memory for me, 123MB, down from 137MB. Good work. Thanks Brian.
        Hide
        Dan Poltawski added a comment -

        Congratulations! This change has been integrated upstream and is now available from our git and download mirrors. To celebrate, here is a joke:

        A SQL query goes into a bar, walks up to two tables and asks, "Can I join you?"

        Show
        Dan Poltawski added a comment - Congratulations! This change has been integrated upstream and is now available from our git and download mirrors. To celebrate, here is a joke: A SQL query goes into a bar, walks up to two tables and asks, "Can I join you?"

          People

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

            Dates

            • Created:
              Updated:
              Resolved: