Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.4.5, 2.5
    • Fix Version/s: 2.4.7, 2.5.3
    • Component/s: Backup
    • Labels:
    • Testing Instructions:
      Hide

      1. Create a backup of a course using default settings. I used the 'S' test course from MDL-38197.

      2. Restore the course using default settings, selecting to restore as a new course. Continue to stage 4 (schema).

      EXPECTED: The schema stage should display the same as it used to, including some fields at the top and then all the sections and activities.

      3. Under one of the sections, turn off one of the activities using the left-hand checkbox. (Make a note of which activity you turned off.)

      EXPECTED: The right-hand checkbox should be greyed out. (This checks that disabledif still works.)

      4. For another activity (make a note of which), turn off user data.
      5. In the fields at the top, change the course name to something amusing (the name must be amusing or this test will fail).
      6. Click Next.

      EXPECTED: The settings changes you made in steps 3, 4, and 5 above should all be reflected on the stage 5 'Review' page.

      7. Finish the restore.
      8. Check the activities on the course page.

      EXPECTED: The activity you turned off earlier should not be included.

      9. Check the name of the course.

      EXPECTED: The name should match the one you set (possibly with 'copy (N)' appended).

      (end of test script)

      NOTE 1: These test instructions confirm that the restore has not been broken by this change. They do not test the actual performance improvement, because it is impossible to test this without a stack of other changes to make backup/restore work. However I have tested the performance improvement and can confirm that the time taken to display this stage on the 'L' test course drops from 692 to 33 seconds.

      NOTE 2: I was going to include a change of the course start date in this test script, but it ignores this option (even before this change, I mean).

      Show
      1. Create a backup of a course using default settings. I used the 'S' test course from MDL-38197 . 2. Restore the course using default settings, selecting to restore as a new course. Continue to stage 4 (schema). EXPECTED: The schema stage should display the same as it used to, including some fields at the top and then all the sections and activities. 3. Under one of the sections, turn off one of the activities using the left-hand checkbox. (Make a note of which activity you turned off.) EXPECTED: The right-hand checkbox should be greyed out. (This checks that disabledif still works.) 4. For another activity (make a note of which), turn off user data. 5. In the fields at the top, change the course name to something amusing (the name must be amusing or this test will fail) . 6. Click Next. EXPECTED: The settings changes you made in steps 3, 4, and 5 above should all be reflected on the stage 5 'Review' page. 7. Finish the restore. 8. Check the activities on the course page. EXPECTED: The activity you turned off earlier should not be included. 9. Check the name of the course. EXPECTED: The name should match the one you set (possibly with 'copy (N)' appended). (end of test script) NOTE 1: These test instructions confirm that the restore has not been broken by this change. They do not test the actual performance improvement, because it is impossible to test this without a stack of other changes to make backup/restore work. However I have tested the performance improvement and can confirm that the time taken to display this stage on the 'L' test course drops from 692 to 33 seconds. NOTE 2: I was going to include a change of the course start date in this test script, but it ignores this option (even before this change, I mean).
    • Affected Branches:
      MOODLE_24_STABLE, MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE, MOODLE_25_STABLE
    • Pull Master Branch:
      MDL-41163-master

      Description

      I am trying to restore a backup of the 'L' size course from MDL-38197 (900MB).

      After I fixed MDL-41147, and incorporating the memory limit fix of MDL-38191, I was able to progress through the settings page (3) but it fails with a time limit exceeded on the schema page (4).

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            Sam Marshall added a comment -

            (Thanks to mr-russ for pointing this out to me.)

            The fundamental cause of this problem is poor performance in the forms library, MDL-30680. While we could resolve this by displaying a progress bar, it might be better to fix the underlying problem.

            Show
            Sam Marshall added a comment - (Thanks to mr-russ for pointing this out to me.) The fundamental cause of this problem is poor performance in the forms library, MDL-30680 . While we could resolve this by displaying a progress bar, it might be better to fix the underlying problem.
            Hide
            Sam Marshall added a comment -

            I removed the 'blocking' issue links - the linked issues are not necessary for testing this issue now that we have been able to profile and identify the problem. I've written a test script that can be done without increased memory limit etc.

            Show
            Sam Marshall added a comment - I removed the 'blocking' issue links - the linked issues are not necessary for testing this issue now that we have been able to profile and identify the problem. I've written a test script that can be done without increased memory limit etc.
            Hide
            Sam Marshall added a comment -

            I think this backup issue is ready for review.

            As it's a serious performance problem (a large course could take 10 minutes to display the form before the fix, vs. 30 seconds after) and as it cherry-picks cleanly, I've submitted branches for all currently supported versions. Of course, if integrators decide it's only suitable for master branch, feel free to make that decision.

            Show
            Sam Marshall added a comment - I think this backup issue is ready for review. As it's a serious performance problem (a large course could take 10 minutes to display the form before the fix, vs. 30 seconds after) and as it cherry-picks cleanly, I've submitted branches for all currently supported versions. Of course, if integrators decide it's only suitable for master branch, feel free to make that decision.
            Hide
            Sam Marshall added a comment -

            Information from my comments on the other issue, directly about this change:

            The problem is that we are calling setDefaults (which applies to the entire form) 3,000 times in my profile run. This comes from the Moodle function base_moodleform::add_setting, which is also being called 3,000 times. It is possible to re-engineer the code so that it instead uses base_moodleform::add_settings which allows for adding multiple settings at once and only calls setDefaults once.

            I've changed it to add the settings in a batch.

            Execution time before: 692 seconds.
            Execution time after: 33 seconds.

            Memory usage before/after: 91,966 KB -> 92,170 KB (either within random variation or at least within 'nobody cares').

            Show
            Sam Marshall added a comment - Information from my comments on the other issue, directly about this change: The problem is that we are calling setDefaults (which applies to the entire form) 3,000 times in my profile run. This comes from the Moodle function base_moodleform::add_setting, which is also being called 3,000 times. It is possible to re-engineer the code so that it instead uses base_moodleform::add_settings which allows for adding multiple settings at once and only calls setDefaults once. I've changed it to add the settings in a batch. Execution time before: 692 seconds. Execution time after: 33 seconds. Memory usage before/after: 91,966 KB -> 92,170 KB (either within random variation or at least within 'nobody cares').
            Hide
            Sam Marshall added a comment -

            Rebased to latest master.

            Show
            Sam Marshall added a comment - Rebased to latest master.
            Hide
            Mark Nelson added a comment -

            Hi Sam,

            Thanks yet again for your contribution to Moodle.

            I have adjusted the fix version for this issue. If this does get pushed into master, then when 2.6 is officially released this won't have ever been an issue from that point. So for historic reasons we write what current stable versions it is affecting, if it is something that is only affecting master, then we use that version.

            Three points:

            1. In your commit you could remove the $content variable as it is not used.
            2. Some of the foreach loops in the function contain "$task->get_settings()", which means this is getting called every loop. I think this should be assigned to a variable which you then use in the foreach, this should be done in all places this occurs in the function.
            3. You loop through the tasks at the end and call get_settings, however you have done this earlier and added these to the $allsettings array. Rather than unsetting this variable, couldn't you loop through it and add the dependencies? That would save calling get_settings again.

            Regards,

            Mark

            Show
            Mark Nelson added a comment - Hi Sam, Thanks yet again for your contribution to Moodle. I have adjusted the fix version for this issue. If this does get pushed into master, then when 2.6 is officially released this won't have ever been an issue from that point. So for historic reasons we write what current stable versions it is affecting, if it is something that is only affecting master, then we use that version. Three points: In your commit you could remove the $content variable as it is not used. Some of the foreach loops in the function contain "$task->get_settings()", which means this is getting called every loop. I think this should be assigned to a variable which you then use in the foreach, this should be done in all places this occurs in the function. You loop through the tasks at the end and call get_settings, however you have done this earlier and added these to the $allsettings array. Rather than unsetting this variable, couldn't you loop through it and add the dependencies? That would save calling get_settings again. Regards, Mark
            Hide
            Sam Marshall added a comment -

            Thanks for review!

            Point 1 wasn't related to my change but you're absolutely right, it is a good opportunity to remove this unused variable; I've done so.

            Point 2 - this is existing code that I didn't change. There are two calls to get_settings, but they are in blocks of code of which only one can run for each task, so there isn't a performance problem here (and because it's such a simple call, I don't think it would improve readability to put it in a variable either). The code does run every time around the loop but it runs with a different $task value so obviously you can't store that as it will be different each time... So I haven't changed this, hope that's okay.

            Point 3 is a very good point and makes the code several lines shorter! I've changed it.

            I've repeated the test script to check it still works, rebased, and pushed all the new branches. Now submitting for integration review.

            Show
            Sam Marshall added a comment - Thanks for review! Point 1 wasn't related to my change but you're absolutely right, it is a good opportunity to remove this unused variable; I've done so. Point 2 - this is existing code that I didn't change. There are two calls to get_settings, but they are in blocks of code of which only one can run for each task, so there isn't a performance problem here (and because it's such a simple call, I don't think it would improve readability to put it in a variable either). The code does run every time around the loop but it runs with a different $task value so obviously you can't store that as it will be different each time... So I haven't changed this, hope that's okay. Point 3 is a very good point and makes the code several lines shorter! I've changed it. I've repeated the test script to check it still works, rebased, and pushed all the new branches. Now submitting for integration review.
            Hide
            Dan Poltawski added a comment -

            Thanks Sam, i've integrated that to master, 25 and 24.

            If was being picky, it would've been nice to see a bit more explanation in the commit message for this issue (as its not immediately clear the impact), but its all recorded here on the bug at least anyway

            cheers
            dan

            Show
            Dan Poltawski added a comment - Thanks Sam, i've integrated that to master, 25 and 24. If was being picky, it would've been nice to see a bit more explanation in the commit message for this issue (as its not immediately clear the impact), but its all recorded here on the bug at least anyway cheers dan
            Hide
            Rossiani Wijaya added a comment -

            This is working as expected.

            Tested for 2.4, 2.5 and master.

            Test passed.

            Show
            Rossiani Wijaya added a comment - This is working as expected. Tested for 2.4, 2.5 and master. Test passed.
            Hide
            Damyon Wiese added a comment -

            This issue along with 77 of it's friends has been sent upstream and released to the world.

            Thankyou for your contribution.

            Show
            Damyon Wiese added a comment - This issue along with 77 of it's friends has been sent upstream and released to the world. Thankyou for your contribution.

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved: