Moodle
  1. Moodle
  2. MDL-41045

course_add_cm_to_section should not test for sections unless necessary

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.6
    • Fix Version/s: 2.4.6, 2.5.2
    • Component/s: Course
    • Labels:
    • Rank:
      51964

      Description

      The course_add_cm_to_section has a comment about not using modinfo because it might not be valid. (It clears modinfo, but does not use it directly.) However, it calls course_create_sections_if_missing which does use modinfo.

      This causes performance problems if you try to add a cm to a section repeatedly, which I am doing at present; each time it will create modinfo, then clear it. Additionally, presumably there was some reason for not using modinfo in the first place.

      I propose that we change course_create_sections_if_missing so that it optionally doesn't use modinfo.

        Issue Links

          Activity

          Hide
          Sam Marshall added a comment -

          Code and unit test now ready for review.

          Performance before and after this change when creating 200 Page activities:

          Before this change: 106 seconds.
          After this change: 5 seconds.

          (Before, it also gets exponentially worse as page count increases.)

          Running entire system unit tests on my local PC (Postgres) showed no performance degradation. I did two 'after' runs and one 'before' (plus an initial 'before' to fill the cache, which was slower and isn't shown):

          After this change (MDL-41045-master): 08:59
          Before this change (master): 08:55
          After this change (MDL-41045-master): 08:48

          To review, justification for this change:

          • This makes it a lot faster to repeatedly create activities (a fairly specialised task).
          • The existing comment in that function claims it is supposed to work without using modinfo, so it would be good to make the code agree with that!
          Show
          Sam Marshall added a comment - Code and unit test now ready for review. Performance before and after this change when creating 200 Page activities: Before this change: 106 seconds. After this change: 5 seconds. (Before, it also gets exponentially worse as page count increases.) Running entire system unit tests on my local PC (Postgres) showed no performance degradation. I did two 'after' runs and one 'before' (plus an initial 'before' to fill the cache, which was slower and isn't shown): After this change ( MDL-41045 -master): 08:59 Before this change (master): 08:55 After this change ( MDL-41045 -master): 08:48 To review, justification for this change: This makes it a lot faster to repeatedly create activities (a fairly specialised task). The existing comment in that function claims it is supposed to work without using modinfo, so it would be good to make the code agree with that!
          Hide
          Marina Glancy added a comment -
          Show
          Marina Glancy added a comment - This was introduced in MDL-35339 so I guess I'm responsible https://github.com/moodle/moodle/commit/4ede27b253ca58f08d1b6f0c8823c76ad38d433a#L3L2700
          Hide
          Marina Glancy added a comment -

          Hi Sam, this is a very good observation. But I would suggest another approach. In course_add_cm_to_section() we query the section entry from DB anyway, maybe we call create_section_if_missing() only if we can't find a record? In this case no argument change is required and this can be backported to 2.5 and 2.4 as well.

          I'm pretty sure this is what Petr Škoda was trying to do in MDL-36547 and just accidentally left call to create_section_if_missing() :
          https://github.com/moodle/moodle/commit/46453565f34fb6a0df7197c675d7c90be04d1be6#L0R2781

          Show
          Marina Glancy added a comment - Hi Sam, this is a very good observation. But I would suggest another approach. In course_add_cm_to_section() we query the section entry from DB anyway, maybe we call create_section_if_missing() only if we can't find a record? In this case no argument change is required and this can be backported to 2.5 and 2.4 as well. I'm pretty sure this is what Petr Škoda was trying to do in MDL-36547 and just accidentally left call to create_section_if_missing() : https://github.com/moodle/moodle/commit/46453565f34fb6a0df7197c675d7c90be04d1be6#L0R2781
          Hide
          Marina Glancy added a comment -

          just curious, can it be connected to the bug reported in MDL-36789

          Show
          Marina Glancy added a comment - just curious, can it be connected to the bug reported in MDL-36789
          Hide
          Sam Marshall added a comment -

          Thanks Marina. Good idea - I will do that much simpler change today and resubmit.

          Technically it will still be violating that comment in the code but I guess we can just add a comment that if you want it to work when there is no valid modinfo, you need to ensure that the section already exists.

          Show
          Sam Marshall added a comment - Thanks Marina. Good idea - I will do that much simpler change today and resubmit. Technically it will still be violating that comment in the code but I guess we can just add a comment that if you want it to work when there is no valid modinfo, you need to ensure that the section already exists.
          Hide
          Sam Marshall added a comment - - edited

          I have applied Marina's suggestion, which is a much simpler fix.

          • I added lines to the function comment to indicate that it requires modinfo if the section does not already exist.
          • There didn't seem to be a unit test for this function. I made a full unit test for it which tests all options including that it doesn't use modinfo (when the section already exists).
          • I have added branches for 24 and 25. (Direct cherry-picks, no conflicts.)

          Submitting for re-review of this new change, which I now think is a valuable improvement since it includes the new unit test (as well as this performance improvement which only affects specialised usage). Also, thanks to Marina's suggestion, this now does not decrease performance at all (my change made it faster in some cases but might have added one query in others).

          Show
          Sam Marshall added a comment - - edited I have applied Marina's suggestion, which is a much simpler fix. I added lines to the function comment to indicate that it requires modinfo if the section does not already exist. There didn't seem to be a unit test for this function. I made a full unit test for it which tests all options including that it doesn't use modinfo (when the section already exists). I have added branches for 24 and 25. (Direct cherry-picks, no conflicts.) Submitting for re-review of this new change, which I now think is a valuable improvement since it includes the new unit test (as well as this performance improvement which only affects specialised usage). Also, thanks to Marina's suggestion, this now does not decrease performance at all (my change made it faster in some cases but might have added one query in others).
          Hide
          Sam Marshall added a comment -

          By the way - I don't think it is very likely to be related to MDL-36789 although there might be some rare place in Moodle where this will reduce the number of modinfo rebuilds - if the modinfo problem is caused by a race condition due to simultaneous rebuilds, then having (potentially) 2 instead of 1 might reduce the chances of it. This is total speculation though. Overall I think probably no connection.

          Show
          Sam Marshall added a comment - By the way - I don't think it is very likely to be related to MDL-36789 although there might be some rare place in Moodle where this will reduce the number of modinfo rebuilds - if the modinfo problem is caused by a race condition due to simultaneous rebuilds, then having (potentially) 2 instead of 1 might reduce the chances of it. This is total speculation though. Overall I think probably no connection.
          Hide
          Marina Glancy added a comment -

          yes agree Sam. Thanks

          Show
          Marina Glancy added a comment - yes agree Sam. Thanks
          Hide
          Marina Glancy added a comment -

          looks good to me Sam, submitting for integration

          Show
          Marina Glancy added a comment - looks good to me Sam, submitting for integration
          Hide
          Sam Hemelryk added a comment -

          Thanks Sam - this has been integrated now.

          Show
          Sam Hemelryk added a comment - Thanks Sam - this has been integrated now.
          Hide
          Michael de Raadt added a comment -

          Test results: Success!

          Show
          Michael de Raadt added a comment - Test results: Success!
          Hide
          Dan Poltawski added a comment -

          Cảm ơn!

          Your changes have now been merged upstream in git and will be available on the Moodle download sites shortly!

          Một hai ba, yo

          Show
          Dan Poltawski added a comment - Cảm ơn! Your changes have now been merged upstream in git and will be available on the Moodle download sites shortly! Một hai ba, yo

            People

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

              Dates

              • Created:
                Updated:
                Resolved: