Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-41728

Backup: Unnecessary use of in_array in base_plan (performance)

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 2.4.6, 2.5.2, 2.6
    • Fix Version/s: 2.4.7, 2.5.3
    • Component/s: Backup
    • Labels:
    • Testing Instructions:
      Hide

      Note: This change is not supposed to result in any difference in behaviour.

      1. Go to the backup screen for a reasonably-sized test course such as the 'S' size test course from 'Make test course'.

      2. Proceed through backup using default options. When you get to the schema page, turn off a couple of activities (just to test that actually changing settings still works), then continue.

      EXPECTED: The review page should correctly indicate that you've turned off the activities.

      3. Continue through backup.

      EXPECTED: Backup completes successfully.

      4. Restore to a new course using default options.

      EXPECTED: Restore completes successfully.

      Show
      Note: This change is not supposed to result in any difference in behaviour. 1. Go to the backup screen for a reasonably-sized test course such as the 'S' size test course from 'Make test course'. 2. Proceed through backup using default options. When you get to the schema page, turn off a couple of activities (just to test that actually changing settings still works), then continue. EXPECTED: The review page should correctly indicate that you've turned off the activities. 3. Continue through backup. EXPECTED: Backup completes successfully. 4. Restore to a new course using default options. EXPECTED: Restore completes successfully.
    • Affected Branches:
      MOODLE_24_STABLE, MOODLE_25_STABLE, MOODLE_26_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE, MOODLE_25_STABLE
    • Pull Master Branch:
      MDL-41728-master

      Description

      The add_task function in base_plan contains logic in a loop that does in_array on the list of settings. For very large courses, there may be thousands of settings, so this is a low-performance call.

      The setting list is already indexed by name. We can refactor the code to use the name index instead. By adding comments as well, the code will become clearer as well as slightly faster.

      Performance improvement on backup 'initial settings' screen:

      XL test course (measured using profiler):
      129.0 -> 110.5 seconds.
      14% execution time, 23% CPU time.

      L test course (no profiler, measured using perf info block, after 1 request to prime caches, 2 runs):
      15.1 -> 14.7 seconds
      15.3 -> 14.7 seconds
      3% execution time (average)

      Conclusion: This change has a small impact (half a second, 3%) on large courses, but becomes significant with very large courses where time is critical.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            quen Sam Marshall added a comment -

            Linking to another issue; this will improve performance on the pages mentioned in that issue (which is about very large courses).

            Show
            quen Sam Marshall added a comment - Linking to another issue; this will improve performance on the pages mentioned in that issue (which is about very large courses).
            Hide
            quen Sam Marshall added a comment -

            As this is potentially a performance bug, is quite small, and cleanly cherry-picks, I have provided all three branches.

            Show
            quen Sam Marshall added a comment - As this is potentially a performance bug, is quite small, and cleanly cherry-picks, I have provided all three branches.
            Hide
            quen Sam Marshall added a comment -

            Requesting peer review from Tim because he's already helped me confirm my understanding of this code...

            Show
            quen Sam Marshall added a comment - Requesting peer review from Tim because he's already helped me confirm my understanding of this code...
            Hide
            timhunt Tim Hunt added a comment -

            Nice one. Submitting for integration.

            Show
            timhunt Tim Hunt added a comment - Nice one. Submitting for integration.
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Integrated (24, 25 & master), thanks!

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Integrated (24, 25 & master), thanks!
            Hide
            phalacee Jason Fowler added a comment -

            Tested as a teacher and an admin, all working, thanks Sam!

            Show
            phalacee Jason Fowler added a comment - Tested as a teacher and an admin, all working, thanks Sam!
            Hide
            samhemelryk Sam Hemelryk added a comment -

            Yarrr me 'arties, good job done. Yer code 'as landed and the weeklies ave been released with your contributions in tow.
            The brethren thank ya for yer 'ard work and if there'd been treasure to ave ya would ave got yer cut.

            Thanks for the effort everyone, another successful weekly release has been rolled.
            Please keep in mind code freeze is just around the corner now, get your new features and improvements in ASAP.

            Many thanks
            Sam

            Show
            samhemelryk Sam Hemelryk added a comment - Yarrr me 'arties, good job done. Yer code 'as landed and the weeklies ave been released with your contributions in tow. The brethren thank ya for yer 'ard work and if there'd been treasure to ave ya would ave got yer cut. Thanks for the effort everyone, another successful weekly release has been rolled. Please keep in mind code freeze is just around the corner now, get your new features and improvements in ASAP. Many thanks Sam

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  11/Nov/13