Moodle
  1. Moodle
  2. MDL-31802

Navigation cache includes section summary data needlessly

    Details

    • Testing Instructions:
      Hide

      1. Create a summary with a huge chunk of text (large enough to notice the size difference when viewing session size).
      2. View the course with that summary and note the size of your session (either on disk or in mdl_sessions if dbsessions is enabled).
      3. Apply the fix, logout to get a new session and relogin and view the same page. Note the new size of the session is smaller.
      4. Check navigation block still works as expected on the course node.

      If you feel like inspecting the sessions themselves, base64_decode them and note that prior to fix, summary data is included and after, it is not.

      Show
      1. Create a summary with a huge chunk of text (large enough to notice the size difference when viewing session size). 2. View the course with that summary and note the size of your session (either on disk or in mdl_sessions if dbsessions is enabled). 3. Apply the fix, logout to get a new session and relogin and view the same page. Note the new size of the session is smaller. 4. Check navigation block still works as expected on the course node. If you feel like inspecting the sessions themselves, base64_decode them and note that prior to fix, summary data is included and after, it is not.
    • Affected Branches:
      MOODLE_20_STABLE, MOODLE_21_STABLE, MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE, MOODLE_23_STABLE
    • Pull from Repository:
    • Pull Master Branch:
    • Rank:
      38420

      Description

      When building the navigation cache, navigationlib stores section data in the cache. It does this by making a call to get_all_sections and storing the entire section data in the cache. This includes the 'summary' field. The summary of a section can be quite large, huge chunks of text and html markup. This can blow up the session data size quite a bit (especially if you have embedded images for example).

      The fix for this is simple, unset the summary field for each section before entering it into the cache, no part of the navigation cache needs the summary data.

        Issue Links

          Activity

          Hide
          Ashley Holman added a comment -

          Since section.summary is a text field, it can be of any size. In one case, the session was 3MB due to inline images in the section summaries. The patch reduces the session to ~70K. The smaller we can get sessions, the better! +1

          Show
          Ashley Holman added a comment - Since section.summary is a text field, it can be of any size. In one case, the session was 3MB due to inline images in the section summaries. The patch reduces the session to ~70K. The smaller we can get sessions, the better! +1
          Hide
          Adam Olley added a comment -

          Just noticed the summary from the navigation cache prevents the summary being shown on first page load of a course. Clearly the page is making use of the navigation cache somewhere it shouldn't...

          Show
          Adam Olley added a comment - Just noticed the summary from the navigation cache prevents the summary being shown on first page load of a course. Clearly the page is making use of the navigation cache somewhere it shouldn't...
          Hide
          Adam Olley added a comment -

          Needed to clone the section object before altering it for the navigation cache. Patch updated on github.

          Show
          Adam Olley added a comment - Needed to clone the section object before altering it for the navigation cache. Patch updated on github.
          Hide
          Sam Hemelryk added a comment -

          Hi guys,

          Thanks for reporting this and coming up with an initial patch.
          I've just linked to another issue MDL-31631 that is reporting a caching issue the rises when a course is being deleted.
          The reason I've linked that issue is because presently it looks like we will be removing the cache code from the navigation for activities and relying on the mod_info object which is doing it's own caching.

          I'll be visiting that issue shortly as there are several related issues now.
          I'll let you know how it all goes and whether we can make use of the changes you have made Adam or whether the removal of the cache is completed on all branches in which case the issue will have been solved.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi guys, Thanks for reporting this and coming up with an initial patch. I've just linked to another issue MDL-31631 that is reporting a caching issue the rises when a course is being deleted. The reason I've linked that issue is because presently it looks like we will be removing the cache code from the navigation for activities and relying on the mod_info object which is doing it's own caching. I'll be visiting that issue shortly as there are several related issues now. I'll let you know how it all goes and whether we can make use of the changes you have made Adam or whether the removal of the cache is completed on all branches in which case the issue will have been solved. Cheers Sam
          Hide
          Sam Hemelryk added a comment -

          Thanks Adam, I've made you the assignee for this issue, have peer-reviewed it, and am now putting it up for integration.
          Changes look good and make sense.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Thanks Adam, I've made you the assignee for this issue, have peer-reviewed it, and am now putting it up for integration. Changes look good and make sense. Cheers Sam
          Hide
          Dan Poltawski added a comment -

          Thanks guys i've integrated this now.

          I added another commit on top just to add a comment to explain those lines as I thought it'd be the sort of thing which would confuse me in the future

          Show
          Dan Poltawski added a comment - Thanks guys i've integrated this now. I added another commit on top just to add a comment to explain those lines as I thought it'd be the sort of thing which would confuse me in the future
          Hide
          Rajesh Taneja added a comment - - edited

          Works Great, Thanks for fixing this Adam.

          FYI:
          Tested on 21, 22, 23 and master

          Show
          Rajesh Taneja added a comment - - edited Works Great, Thanks for fixing this Adam. FYI: Tested on 21, 22, 23 and master
          Hide
          Sam Hemelryk added a comment -

          Congratulations your code is upstream - gold star for you!

          This issue + 79 others made it in in time for the minor releases.
          Thank you everyone involved for your exuberant efforts.

          Show
          Sam Hemelryk added a comment - Congratulations your code is upstream - gold star for you! This issue + 79 others made it in in time for the minor releases. Thank you everyone involved for your exuberant efforts.

            People

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

              Dates

              • Created:
                Updated:
                Resolved: