Moodle
  1. Moodle
  2. MDL-23754 Performance improvements META
  3. MDL-25249

Calendar Pollutes Sessions, Causing Saved Sessions To Waste 99% Of Disk Space

    Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: 1.9, 2.0
    • Fix Version/s: 2.1
    • Component/s: Calendar
    • Labels:
    • Testing Instructions:
      Hide

      Basic (functionality) testing instructions:

      Site with admin, teacher, student access (teacher and student enrolled in multiple courses)

      • as admin create some site events and also some course event. Test the calendar block, the upcoming events block and the calendar page shows all the information properly.
      • as teacher enrolled in various courses create some events of all the types allowed. Test the calendar block, the upcoming events block and the calendar page shows all the information (own and admin one) properly.
      • repeat as student (the information from previous steps should be visible. But personal events, of course.

      Surely there are more but this should cover the thingy more or less to detect any immediate regression.

      Show
      Basic (functionality) testing instructions: Site with admin, teacher, student access (teacher and student enrolled in multiple courses) as admin create some site events and also some course event. Test the calendar block, the upcoming events block and the calendar page shows all the information properly. as teacher enrolled in various courses create some events of all the types allowed. Test the calendar block, the upcoming events block and the calendar page shows all the information (own and admin one) properly. repeat as student (the information from previous steps should be visible. But personal events, of course. Surely there are more but this should cover the thingy more or less to detect any immediate regression.
    • Difficulty:
      Difficult
    • Affected Branches:
      MOODLE_19_STABLE, MOODLE_20_STABLE
    • Fixed Branches:
      MOODLE_21_STABLE
    • Pull Master Branch:
      wip-MDL-25249-master-r1
    • Rank:
      6218

      Description

      A lot of our sessions were curiously large, sometimes approaching 2 megabytes per session. Because sessions are read and rewritten on every request that uses the session, this generates a fair bit of traffic. After decoding the stored session information, I walked the session looking for the culprit. cal_courses_shown appeared to be the largest offender, so I wrote a test script which evaluated the top 10 sessions at the time. The number in the parentheses is the "before" size. The number after "Now:" is the "after" size. The only change that I made to the session was to run the following line of code:

      unset($_SESSION['SESSION']->cal_courses_shown);

      My results follow:

      _Start_

      Session #1 (1282077)
      Now: 5251 (99.59% savings)

      Session #2 (1113736)
      Now: 5449 (99.51% savings)

      Session #3 (870381)
      Now: 5489 (99.37% savings)

      Session #4 (487920)
      Now: 5658 (98.84% savings)

      Session #5 (451477)
      Now: 5143 (98.86% savings)

      Session #6 (440864)
      Now: 5300 (98.80% savings)

      Session #7 (255664)
      Now: 5915 (97.69% savings)

      Session #8 (255311)
      Now: 5562 (97.82% savings)

      Session #9 (255211)
      Now: 5462 (97.86% savings)

      Session #10 (255160)
      Now: 5411 (97.88% savings)

      _End_

      1. MDL25249.patch
        8 kB
        John Kelsh
      2. moodle_trim_calendar_data_from_session.patch
        0.6 kB
        Jonathan Champ

        Issue Links

          Activity

          Hide
          Petr Škoda added a comment -

          I have studied the code a bit and I have to agree the code is horrible! We should definitely fix this in 2.0.3.

          Show
          Petr Škoda added a comment - I have studied the code a bit and I have to agree the code is horrible! We should definitely fix this in 2.0.3.
          Hide
          Martin Dougiamas added a comment -

          Thanks Petr! (Taking this out of the sprint)

          Show
          Martin Dougiamas added a comment - Thanks Petr! (Taking this out of the sprint)
          Hide
          John Kelsh added a comment -

          Here's a quick patch to stop this from happening, hope it helps until a more permanent fix is made.

          Show
          John Kelsh added a comment - Here's a quick patch to stop this from happening, hope it helps until a more permanent fix is made.
          Hide
          Jonathan Champ added a comment -

          Petr, I noticed that you put this back in the DEV backlog. This seems like a downgrade in priority. Is there a specific reason? Thanks!

          Show
          Jonathan Champ added a comment - Petr, I noticed that you put this back in the DEV backlog. This seems like a downgrade in priority. Is there a specific reason? Thanks!
          Hide
          Petr Škoda added a comment -

          The reason is that stable branch is not a good place for development, I am working on a perf improvement plan for 2.1dev right now.

          Show
          Petr Škoda added a comment - The reason is that stable branch is not a good place for development, I am working on a perf improvement plan for 2.1dev right now.
          Hide
          Jonathan Champ added a comment -

          Here's the quick patch that we're using to cut the amount of data being written to the file system by Moodle in half.

          Show
          Jonathan Champ added a comment - Here's the quick patch that we're using to cut the amount of data being written to the file system by Moodle in half.
          Hide
          Rossiani Wijaya added a comment -

          Hi Sam,

          Tested the Calendar event code by populated it with different event types and roles. So far everything works great. I will do more testing to make sure it works as it should be.

          Show
          Rossiani Wijaya added a comment - Hi Sam, Tested the Calendar event code by populated it with different event types and roles. So far everything works great. I will do more testing to make sure it works as it should be.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Note this makes reference to this proposal: https://github.com/samhemelryk/moodle/compare/master...wip-MDL-25249-master 

          Ho Sam,

          while I agree that stop using SESSION information can be great for SESSION size... I can imagine that the call to: calendar_get_default_courses() in sites having thousands of courses can be really, really slow, more if it is executed twice on the site page, once for the months block and another for the upcoming events one.

          So I'd introduce some comparison for these situations (before and after the patch, session size, queries and time), front page for:

          • Site with 1000 courses. Logged in as admin.
          • Site with 1000 courses. Logged as teacher and student enrolled in, say, 100 courses.

          Perhaps we should try to make a bigger join of enrolled courses + course events together or so or, at least cache for the request the calendar_get_default_courses() results. And also, reduce the columns feched by enrol_get_my_courses(). id seems to be enough! Perhaps that's the cause for those BIG session contents originally.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Note this makes reference to this proposal: https://github.com/samhemelryk/moodle/compare/master...wip-MDL-25249-master   Ho Sam, while I agree that stop using SESSION information can be great for SESSION size... I can imagine that the call to: calendar_get_default_courses() in sites having thousands of courses can be really, really slow, more if it is executed twice on the site page, once for the months block and another for the upcoming events one. So I'd introduce some comparison for these situations (before and after the patch, session size, queries and time), front page for: Site with 1000 courses. Logged in as admin. Site with 1000 courses. Logged as teacher and student enrolled in, say, 100 courses. Perhaps we should try to make a bigger join of enrolled courses + course events together or so or, at least cache for the request the calendar_get_default_courses() results. And also, reduce the columns feched by enrol_get_my_courses(). id seems to be enough! Perhaps that's the cause for those BIG session contents originally. Ciao
          Hide
          Sam Hemelryk added a comment -

          Hi Eloy,

          I've generated a site today with 512 courses from which the users being used for performance testing are each enrolled in 30 courses.

          The stats are as following:
          http://moodev.com/?w=300&h=150&before=master.1308032995916&after=wip-MDL-25249-master.1308034944328#statsarray

          Compared to the run with 20 courses and each user having 5 enrolments which can be seen here:
          http://moodev.com/?w=300&h=150&before=master.1307686050649&after=wip-MDL-25249-master.1307689232914#statsarray

          I havn't yet tested as admin/teacher - all tests have been done as students.
          I will run tests as admin and then teacher shortly.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi Eloy, I've generated a site today with 512 courses from which the users being used for performance testing are each enrolled in 30 courses. The stats are as following: http://moodev.com/?w=300&h=150&before=master.1308032995916&after=wip-MDL-25249-master.1308034944328#statsarray Compared to the run with 20 courses and each user having 5 enrolments which can be seen here: http://moodev.com/?w=300&h=150&before=master.1307686050649&after=wip-MDL-25249-master.1307689232914#statsarray I havn't yet tested as admin/teacher - all tests have been done as students. I will run tests as admin and then teacher shortly. Cheers Sam
          Show
          Sam Hemelryk added a comment - Comparison if all users are also admins: http://moodev.com/?w=300&h=150&before=master.1308040286925&after=wip-MDL-25249-master.1308043105687#statsarray
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Yay, found the perfect excuse to not integrate this: WHITESPACE !!!!

          LOL (I needed it!)

          Show
          Eloy Lafuente (stronk7) added a comment - Yay, found the perfect excuse to not integrate this: WHITESPACE !!!! LOL (I needed it!)
          Hide
          Eloy Lafuente (stronk7) added a comment -

          For the records, got a problem trying to enable / disable some event types:

          Notice: Undefined variable: return in /calendar/set.php on line 47

          Show
          Eloy Lafuente (stronk7) added a comment - For the records, got a problem trying to enable / disable some event types: Notice: Undefined variable: return in /calendar/set.php on line 47
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Offtopic: Really hate the assumption of Sunday being the fist day of the week, grr

          Show
          Eloy Lafuente (stronk7) added a comment - Offtopic: Really hate the assumption of Sunday being the fist day of the week, grr
          Hide
          Eloy Lafuente (stronk7) added a comment - - edited

          Oki, have reviewed code and tested it a bit.

          I think the change has sense and code looks perfect, although I'm a bit worried with release happening in 2 days (lack of testing specially) and also with repeated calls to calendar_get_default_courses() in the same request and its speed in BIG sites.

          So we have 2 alternatives:

          • Be brave, fix the whitespace and the set.php problem above and make this change land in 24h.
          • Be calm and have some more time to test looking for regressions, probably build / improve some QA test related to calendar...

          Right now I feel myself brave enough, but surely it's one effect of the big glass of milk I'm drinking while typing.

          So asking for opinion... ciao

          Show
          Eloy Lafuente (stronk7) added a comment - - edited Oki, have reviewed code and tested it a bit. I think the change has sense and code looks perfect, although I'm a bit worried with release happening in 2 days (lack of testing specially) and also with repeated calls to calendar_get_default_courses() in the same request and its speed in BIG sites. So we have 2 alternatives: Be brave, fix the whitespace and the set.php problem above and make this change land in 24h. Be calm and have some more time to test looking for regressions, probably build / improve some QA test related to calendar... Right now I feel myself brave enough, but surely it's one effect of the big glass of milk I'm drinking while typing. So asking for opinion... ciao
          Hide
          Michael de Raadt added a comment -

          Hi, Eloy.

          I vote for option 1.

          Until we really get this code onto real sites we are not going to see if it has benefits or problems.

          Risky... hmmm.

          Michael;

          Show
          Michael de Raadt added a comment - Hi, Eloy. I vote for option 1. Until we really get this code onto real sites we are not going to see if it has benefits or problems. Risky... hmmm. Michael;
          Hide
          Sam Hemelryk added a comment -

          Hi guys,

          I made a couple more minor changes to this patch - particularly around the use of calendar_get_default_courses.
          I've also cleaned up the white space issues and fixed the bug in set.php.

          repo: git://github.com/samhemelryk/moodle.git
          brnh: wip-MDL-25249-master-r1
          diff: https://github.com/samhemelryk/moodle/compare/master...wip-MDL-25249-master-r1

          I've also run performance tests before and after this patch:
          http://moodev.com/jmeter/1.0/?w=300&h=150&before=master.1309250011827&after=wip-MDL-25249-master-temp.1309242670726#statsarray

          In regards to your concern about calendar_get_default_courses Eloy there are a couple of things to note:

          1. If an admin has enabled $CFG->calendar_adminseesall (off by default) and a user with moodle/calendar:manageentries in the system context (teacher, editingteacher, manager) then calendar events for all courses are meant to be shown.
          2. Otherwise for most people the call to enrol_get_my_courses is made to return courses.

          Since you last reviewed I made a couple of changes:

          • When fetching all courses there is now a hard limit of 20 courses and only courses with events are included.
          • We preload contexts when fetching these courses as we almost always run capability checks before displaying events.

          The only general places to call calendar_get_default_courses are the two blocks and a couple of calendar specific pages (3x calendar/*.php) so under normal use likely 3 calls to enrol_get_my_courses of which two could potentially be avoided.
          I am hoping that future performance improvements which are planned to cache groups, and enrolment information will help this.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi guys, I made a couple more minor changes to this patch - particularly around the use of calendar_get_default_courses. I've also cleaned up the white space issues and fixed the bug in set.php. repo: git://github.com/samhemelryk/moodle.git brnh: wip- MDL-25249 -master-r1 diff: https://github.com/samhemelryk/moodle/compare/master...wip-MDL-25249-master-r1 I've also run performance tests before and after this patch: http://moodev.com/jmeter/1.0/?w=300&h=150&before=master.1309250011827&after=wip-MDL-25249-master-temp.1309242670726#statsarray In regards to your concern about calendar_get_default_courses Eloy there are a couple of things to note: If an admin has enabled $CFG->calendar_adminseesall (off by default) and a user with moodle/calendar:manageentries in the system context (teacher, editingteacher, manager) then calendar events for all courses are meant to be shown. Otherwise for most people the call to enrol_get_my_courses is made to return courses. Since you last reviewed I made a couple of changes: When fetching all courses there is now a hard limit of 20 courses and only courses with events are included. We preload contexts when fetching these courses as we almost always run capability checks before displaying events. The only general places to call calendar_get_default_courses are the two blocks and a couple of calendar specific pages (3x calendar/*.php) so under normal use likely 3 calls to enrol_get_my_courses of which two could potentially be avoided. I am hoping that future performance improvements which are planned to cache groups, and enrolment information will help this. Cheers Sam
          Hide
          Eloy Lafuente (stronk7) added a comment -

          here we go... re-reviewing...

          Show
          Eloy Lafuente (stronk7) added a comment - here we go... re-reviewing...
          Hide
          Eloy Lafuente (stronk7) added a comment -

          What can I say, you convinced me, lol. Integrated, nice work Sam!

          Show
          Eloy Lafuente (stronk7) added a comment - What can I say, you convinced me, lol. Integrated, nice work Sam!
          Hide
          Glenn Ansley added a comment -

          Everything looks good for all roles, blocks, and calendars mentioned in testing. Couldn't find any errors either.

          Show
          Glenn Ansley added a comment - Everything looks good for all roles, blocks, and calendars mentioned in testing. Couldn't find any errors either.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Sweet one, upstream-ized! Thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - Sweet one, upstream-ized! Thanks!

            People

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

              Dates

              • Created:
                Updated:
                Resolved: