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

          Attachments

            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