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

course_add_cm_to_section should not test for sections unless necessary

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 2.6
    • Fix Version/s: 2.4.6, 2.5.2
    • Component/s: Course
    • Labels:
    • Testing Instructions:
      Hide
      1. Create course
      2. Add activity
      3. Move activity to another section
      4. Refresh page, make sure everything is ok
      5. Disable AJAX (or Javascript) and move activity to another section again
      6. Make sure everything looks ok
      Show
      Create course Add activity Move activity to another section Refresh page, make sure everything is ok Disable AJAX (or Javascript) and move activity to another section again Make sure everything looks ok
    • Affected Branches:
      MOODLE_26_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE, MOODLE_25_STABLE
    • Pull Master Branch:
      MDL-41045-master

      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.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            quen 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
            quen 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 Marina Glancy added a comment -
            Show
            marina 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 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 Skoda 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 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 Skoda 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 Marina Glancy added a comment -

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

            Show
            marina Marina Glancy added a comment - just curious, can it be connected to the bug reported in MDL-36789
            Hide
            quen 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
            quen 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
            quen 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
            quen 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
            quen 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
            quen 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 Marina Glancy added a comment -

            yes agree Sam. Thanks

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

            looks good to me Sam, submitting for integration

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

            Thanks Sam - this has been integrated now.

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

            Test results: Success!

            Show
            salvetore Michael de Raadt added a comment - Test results: Success!
            Hide
            poltawski 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
            poltawski 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:
                  Fix Release Date:
                  9/Sep/13