Moodle
  1. Moodle
  2. MDL-33448

Enrolment page very slow (freezes)

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor 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
    • Rank:
      41342

      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.

        Issue Links

          Activity

          Hide
          Petr Škoda added a comment -

          I am sorry, I can not reproduce this.

          Show
          Petr Škoda added a comment - I am sorry, I can not reproduce this.
          Hide
          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
          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 Agarwal added a comment -

          I can reproduce this as well.

          Show
          Ankit Agarwal added a comment - I can reproduce this as well.
          Hide
          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
          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
          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
          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
          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
          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
          Sam Hemelryk added a comment -
          Show
          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
          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
          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
          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
          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
          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
          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
          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
          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 added a comment -

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

          Show
          CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
          Hide
          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
          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
          Aparup Banerjee added a comment -

          release imminent. stopping for now.

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

          Removing integrator as this one got missed this week.

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

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

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

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

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

          Works as expected on 22, 23, 24 and master

          Show
          Dan Poltawski added a comment - Works as expected on 22, 23, 24 and master
          Hide
          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
          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 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 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: