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

Enrolment page very slow (freezes)

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 2.2.6, 2.3, 2.3.3, 2.4
    • Fix Version/s: 2.2.7, 2.3.4, 2.4.1
    • Component/s: Enrolments
    • Labels:
      None
    • Testing Instructions:
      Hide
      1. Log in as an admin
      2. Browse to any course
      3. Head to Course settings > Users > Enrolled users
      4. Click "Enrol Users" button
      5. In the dialog click to expand "Enrolment options"
      6. Check that there are start date and period options.
      7. Test enrolling a user.
      Show
      Log in as an admin Browse to any course Head to Course settings > Users > Enrolled users Click "Enrol Users" button In the dialog click to expand "Enrolment options" Check that there are start date and period options. Test enrolling a user.
    • Affected Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE, MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE, MOODLE_24_STABLE
    • Pull Master Branch:
      wip-MDL-33448-m24

      Description

      1. Go to a course with a few enrolled users
      2. Navigate to Settings > Course administration > Users > Enrolled users

      The page itself appears quickly but it seems like the JavaScript causes the browser to freeze for a couple of seconds. Try to scroll up and down while the page is loading to notice the freeze.
      This is not happening on 2.1 and 2.2. I reproduced this on master and master integration, with Chrome, Opera and Firefox. With about 15 enrolled users. The total user count over the site was either 15ish or 250ish. Same results.

      I will attach screenshots of Firebug and Dev tools.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            skodak Petr Skoda added a comment -

            I am sorry, I can not reproduce this.

            Show
            skodak Petr Skoda added a comment - I am sorry, I can not reproduce this.
            Hide
            fred Frédéric Massart added a comment -

            I have dug a bit more and found out that you need to have debug mode set to developer for this to happen. And the reason is located at javascript-static.js:719 where a new YUI object is created in the get_string() method. This method is called 365 times from manual/yui/quickenrolment.js:198 (populateDuration()).

            I still don't know why there is such a difference between 2.2 and master, perhaps because of an update of the YUI libraries.

            Anyway, this is not really an 'issue'.

            Show
            fred Frédéric Massart added a comment - I have dug a bit more and found out that you need to have debug mode set to developer for this to happen. And the reason is located at javascript-static.js:719 where a new YUI object is created in the get_string() method. This method is called 365 times from manual/yui/quickenrolment.js:198 (populateDuration()). I still don't know why there is such a difference between 2.2 and master, perhaps because of an update of the YUI libraries. Anyway, this is not really an 'issue'.
            Hide
            ankit_frenz Ankit Agarwal added a comment -

            I can reproduce this as well.

            Show
            ankit_frenz Ankit Agarwal added a comment - I can reproduce this as well.
            Hide
            samhemelryk Sam Hemelryk added a comment -

            Putting a fix for this up for peer-review.
            The fix is just to cache the Y instance within get_string so that scripts calling get_string a lot don't kill performance if debugging is on.

            Show
            samhemelryk Sam Hemelryk added a comment - Putting a fix for this up for peer-review. The fix is just to cache the Y instance within get_string so that scripts calling get_string a lot don't kill performance if debugging is on.
            Hide
            dobedobedoh Andrew Nicols added a comment -

            Looks like a good solution to me.
            Doesn't explain why this has only just become an issue in 2.3, unless there are suddenly a lot more calls to M.util.get_string() than in 2.2.

            The YUI creation in this module hasn't changed in a couple of years.

            Show
            dobedobedoh Andrew Nicols added a comment - Looks like a good solution to me. Doesn't explain why this has only just become an issue in 2.3, unless there are suddenly a lot more calls to M.util.get_string() than in 2.2. The YUI creation in this module hasn't changed in a couple of years.
            Hide
            samhemelryk Sam Hemelryk added a comment -

            Hmm good point Andrew, perhaps an underlying issue still here.
            I've created MDL-34404 to get my fix here integrated and will continue to look for a bug + solution to the numerous get_string calls which I've confirmed I can reproduce.

            Show
            samhemelryk Sam Hemelryk added a comment - Hmm good point Andrew, perhaps an underlying issue still here. I've created MDL-34404 to get my fix here integrated and will continue to look for a bug + solution to the numerous get_string calls which I've confirmed I can reproduce.
            Hide
            samhemelryk Sam Hemelryk added a comment -
            Show
            samhemelryk Sam Hemelryk added a comment - The offending code is https://github.com/samhemelryk/moodle/blob/9fe40de660bcf764b48d3de256f9a294ea9a4e03/enrol/manual/yui/quickenrolment/quickenrolment.js#L205 A loop to generate a select box for the enrolment duration.
            Hide
            samhemelryk Sam Hemelryk added a comment -

            Ok.

            I've looked at this quickly, using get_string there is still a pretty good solution I feel, accurate any way.
            What I've done is re-organise the code so that the duration and start date options arn't generated until those controls are actually viewed by the user.
            This offsets the processing in most cases and means we only fetch those strings when the user is interesting in changing from the default settings.

            Cheers
            Sam

            Show
            samhemelryk Sam Hemelryk added a comment - Ok. I've looked at this quickly, using get_string there is still a pretty good solution I feel, accurate any way. What I've done is re-organise the code so that the duration and start date options arn't generated until those controls are actually viewed by the user. This offsets the processing in most cases and means we only fetch those strings when the user is interesting in changing from the default settings. Cheers Sam
            Hide
            samhemelryk Sam Hemelryk added a comment -

            Trying my luck and pushing this up for integration before a peer-review could happen.
            Integrators feel free to reject this until a peer-review has happened.

            Show
            samhemelryk Sam Hemelryk added a comment - Trying my luck and pushing this up for integration before a peer-review could happen. Integrators feel free to reject this until a peer-review has happened.
            Hide
            mytskine François Gannaz added a comment -

            I've been annoyed by this too (15 seconds freeze using the latest Opera). IMHO, the patch proposed wouldn't help: I don't understand how delaying the freeze to the moment the user clicks the "Add users" button is better than freezing when the page loads. It only makes the bug a little less frequent.

            Show
            mytskine François Gannaz added a comment - I've been annoyed by this too (15 seconds freeze using the latest Opera). IMHO, the patch proposed wouldn't help: I don't understand how delaying the freeze to the moment the user clicks the "Add users" button is better than freezing when the page loads. It only makes the bug a little less frequent.
            Hide
            nebgor Aparup Banerjee added a comment -

            Hi Sam!
            i've tried it out and it didn't seem to have any effect for me.
            Nevertheless i think the js(yui)<->php interaction here could be improved to be more server-side. i would think some sort of bulk string loader get_strings_bulk() that wrapped around get_string() or so would help reduce the iterations at the js (and xhr overhead). Perhaps this would also help towards any other development looking to improve bulk-ish string loads... and caching.

            i haven't thought about optimising the need for the strings being displayed but i like my above thoughts better so here's to safe flights and happy landings.

            see you soon.

            reopening :-D

            Show
            nebgor Aparup Banerjee added a comment - Hi Sam! i've tried it out and it didn't seem to have any effect for me. Nevertheless i think the js(yui)<->php interaction here could be improved to be more server-side. i would think some sort of bulk string loader get_strings_bulk() that wrapped around get_string() or so would help reduce the iterations at the js (and xhr overhead). Perhaps this would also help towards any other development looking to improve bulk-ish string loads... and caching. i haven't thought about optimising the need for the strings being displayed but i like my above thoughts better so here's to safe flights and happy landings. see you soon. reopening :-D
            Hide
            cibot CiBoT added a comment -

            Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

            Show
            cibot CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
            Hide
            samhemelryk Sam Hemelryk added a comment -

            Hi Apu,

            In this case those strings have already been loaded into the JS on the server using $PAGE->requires->strings_for_js. That handles the bulk loading for us on the server.
            The problem was arising because the front-end was repetitively calling the JS get_string method.

            The first commit I've made simply offset the collection+use of those strings by attaching their generation to the display of the settings, something that will only happen if the user wants to configure the enrollment duration and start time.

            I've made a second commit just now that reduces the JS get_string calls by 355.

            Also worth noting is that in a separate tracker issue I reworked the way in which JS get_string was working. Previously it generated a YUI instance for every call and caused death for the enrolment widget on IE because of the the number of get_string calls it makes.
            I'm pretty confident that change plus this one will nullify the issue.

            If you want to try to reproduce it you best bet would be to set master back a couple of months first to see the overall effect and then bring master up to date with these changes.
            Even then I found my dev machine was sizeable enough not to be troubled, I went back to an older laptop I had to replicate the issue.

            Many thanks
            Sam

            Show
            samhemelryk Sam Hemelryk added a comment - Hi Apu, In this case those strings have already been loaded into the JS on the server using $PAGE->requires->strings_for_js. That handles the bulk loading for us on the server. The problem was arising because the front-end was repetitively calling the JS get_string method. The first commit I've made simply offset the collection+use of those strings by attaching their generation to the display of the settings, something that will only happen if the user wants to configure the enrollment duration and start time. I've made a second commit just now that reduces the JS get_string calls by 355. Also worth noting is that in a separate tracker issue I reworked the way in which JS get_string was working. Previously it generated a YUI instance for every call and caused death for the enrolment widget on IE because of the the number of get_string calls it makes. I'm pretty confident that change plus this one will nullify the issue. If you want to try to reproduce it you best bet would be to set master back a couple of months first to see the overall effect and then bring master up to date with these changes. Even then I found my dev machine was sizeable enough not to be troubled, I went back to an older laptop I had to replicate the issue. Many thanks Sam
            Hide
            nebgor Aparup Banerjee added a comment -

            release imminent. stopping for now.

            Show
            nebgor Aparup Banerjee added a comment - release imminent. stopping for now.
            Hide
            poltawski Dan Poltawski added a comment -

            Removing integrator as this one got missed this week.

            Show
            poltawski Dan Poltawski added a comment - Removing integrator as this one got missed this week.
            Hide
            poltawski Dan Poltawski added a comment -

            Will test and integrate this one today to get it through whilst Apu is on plugins.

            Show
            poltawski Dan Poltawski added a comment - Will test and integrate this one today to get it through whilst Apu is on plugins.
            Hide
            poltawski Dan Poltawski added a comment -

            Thanks Sam, i've integrated and tested this now.

            Show
            poltawski Dan Poltawski added a comment - Thanks Sam, i've integrated and tested this now.
            Hide
            poltawski Dan Poltawski added a comment -

            Works as expected on 22, 23, 24 and master

            Show
            poltawski Dan Poltawski added a comment - Works as expected on 22, 23, 24 and master
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Many thanks for your effort, the whole Moodle Community will be enjoying your great solutions starting now!

            Closing, ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Many thanks for your effort, the whole Moodle Community will be enjoying your great solutions starting now! Closing, ciao
            Hide
            sumit.negi sumit negi added a comment - - edited

            Hi,
            I am still facing this problem, page is very slow(Moodle version 2.3.1). Can anyone help me.

            Show
            sumit.negi sumit negi added a comment - - edited Hi, I am still facing this problem, page is very slow(Moodle version 2.3.1). Can anyone help me.

              People

              • Votes:
                1 Vote for this issue
                Watchers:
                4 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  14/Jan/13