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

fix_course_sortorder makes editing course slow when large number of courses

    Details

    • Testing Instructions:
      Hide

      Testing for regressions:

      1. Update a course using /course/edit.php page without changing the category, make sure there are no errors and course is updated. This action should be faster than before patch (especially on big site with lots of categories and courses).
      2. Update a course using /course/edit.php with changing the category, make sure course is placed in the end of the course list in the new category.

      PS function update_course() is unfortunately not used on /course/manage.php page. Hopefully it will be after MDL-31830

      Show
      Testing for regressions: Update a course using /course/edit.php page without changing the category, make sure there are no errors and course is updated. This action should be faster than before patch (especially on big site with lots of categories and courses). Update a course using /course/edit.php with changing the category, make sure course is placed in the end of the course list in the new category. PS function update_course() is unfortunately not used on /course/manage.php page. Hopefully it will be after MDL-31830
    • Affected Branches:
      MOODLE_24_STABLE, MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE, MOODLE_25_STABLE
    • Pull Master Branch:
      wip-MDL-41517-master
    • Sprint:
      BACKEND Sprint 4
    • Sprint:
      BACKEND Sprint 4

      Description

      We have a client with over 30K courses and when a course is edited there is a ~15s delay, almost all of which is spent in fix_course_sortorder. This is needless if the course category has not been changed on the edit form.

      I have a patch against 2.5.1 which allows course/edit.php to signal when the sortorder has not changed, but leaves the default action alone otherwise.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            tworthington Thomas Worthington added a comment -

            Patch against 2.5.1 (Build: 20130823)

            Show
            tworthington Thomas Worthington added a comment - Patch against 2.5.1 (Build: 20130823)
            Hide
            salvetore Michael de Raadt added a comment -

            Thanks for reporting the issue and providing a fix.

            Show
            salvetore Michael de Raadt added a comment - Thanks for reporting the issue and providing a fix.
            Hide
            marina Marina Glancy added a comment -

            Thanks Thomas, I'm looking at your patch and wonder why would we ever need to call fix_course_sortorder() if course properties category and sortorder has not changed (btw! important that you forgot the 'sortorder' property). Maybe we should not introduce the argument at all and just do a check inside the function only. In this case it can be backported to stable versions.

            Show
            marina Marina Glancy added a comment - Thanks Thomas, I'm looking at your patch and wonder why would we ever need to call fix_course_sortorder() if course properties category and sortorder has not changed (btw! important that you forgot the 'sortorder' property). Maybe we should not introduce the argument at all and just do a check inside the function only. In this case it can be backported to stable versions.
            Hide
            marina Marina Glancy added a comment -

            Submitting for peer review

            Show
            marina Marina Glancy added a comment - Submitting for peer review
            Hide
            rajeshtaneja Rajesh Taneja added a comment -

            Thanks for fixing this Marina,

            Patch looks good..

            Just wondering, if purge_by_cache event is expensive and can be avoided, as event is trigged in fix_course_sortorder() and update_course().

            Feel free to push it for integration, when ready.

            Show
            rajeshtaneja Rajesh Taneja added a comment - Thanks for fixing this Marina, Patch looks good.. Just wondering, if purge_by_cache event is expensive and can be avoided, as event is trigged in fix_course_sortorder() and update_course(). Feel free to push it for integration, when ready.
            Hide
            marina Marina Glancy added a comment -

            Hi Raj, thanks for review, cache_helper::purge_by_event() is not expensive and it is not always called in fix_course_sortorder()

            Show
            marina Marina Glancy added a comment - Hi Raj, thanks for review, cache_helper::purge_by_event() is not expensive and it is not always called in fix_course_sortorder()
            Hide
            tworthington Thomas Worthington added a comment -

            Marina, I certainly agree that's a better way to do it. I did think my patch was a bit of a kludge but I needed something quickly and thought it might be of interest. At least it's sparked a decent fix, so it's all good.

            Show
            tworthington Thomas Worthington added a comment - Marina, I certainly agree that's a better way to do it. I did think my patch was a bit of a kludge but I needed something quickly and thought it might be of interest. At least it's sparked a decent fix, so it's all good.
            Hide
            marina Marina Glancy added a comment -

            Submitting for integration

            Show
            marina Marina Glancy added a comment - Submitting for integration
            Hide
            damyon Damyon Wiese added a comment -

            Thanks Thomas/Marina,

            This looks good to me - integrated to 24, 25 and master.

            Show
            damyon Damyon Wiese added a comment - Thanks Thomas/Marina, This looks good to me - integrated to 24, 25 and master.
            Hide
            salvetore Michael de Raadt added a comment -

            Test result: Success!

            Tested in 2.4, 2.5 and master. No errors presented themselves.

            Show
            salvetore Michael de Raadt added a comment - Test result: Success! Tested in 2.4, 2.5 and master. No errors presented themselves.
            Hide
            damyon 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 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:
                5 Start watching this issue

                Dates

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

                  Agile