Details

    • Difficulty:
      Moderate
    • Affected Branches:
      MOODLE_20_STABLE

      Description

      Most users can not see anything from admin tree, but still we are loading it nearly on each page, this caused very many extra php includes.

      We could store a flag in session which would indicate that user can not see anything from admin tree and then we would just skip loading of the admin tree completely.

      This could help with memory and CPU load.

      potential benefit?
      -50 file includes for each student on each page
      -4MB in 64 bit OS on each page for students

        Gliffy Diagrams

          Activity

          Hide
          Eloy Lafuente (stronk7) added a comment -

          Sending to stable backlog and assigning to Sam for his consideration (my main concern is if by avoiding some includes we are going to break other things relying on them).

          But in the other side if other things break, for sure it's because they are buggy and missing to include stuff themselves.

          So, as you decide, my +1 for being fixed. Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Sending to stable backlog and assigning to Sam for his consideration (my main concern is if by avoiding some includes we are going to break other things relying on them). But in the other side if other things break, for sure it's because they are buggy and missing to include stuff themselves. So, as you decide, my +1 for being fixed. Ciao
          Hide
          Sam Hemelryk added a comment -

          Hi guys,

          I've just posted a patch for this to https://github.com/samhemelryk/moodle/compare/master...wip-MDL-25291
          Andrew can you please peer-review this?

          It makes quite a noticeable difference, the following are the before and after stats I collected while performance testing this.

            Admin Student Guest None
          Before the patch
          Files 547 204 187 186
          Memory (MB) 69.3 61 57.1 57
          get_string calls 1371 272 238 236
          After the patch
          Files 547 151 133 132
          Memory (MB) 69.3 54.9 50.7 50.6
          get_string calls 1371 213 179 177
          Benefits
          Less files 0 53 54 54
          Less Memory (MB) 0 10% (6.1) 11% (6.4) 11% (6.4)
          Less get_string calls 0 59 59 59

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi guys, I've just posted a patch for this to https://github.com/samhemelryk/moodle/compare/master...wip-MDL-25291 Andrew can you please peer-review this? It makes quite a noticeable difference, the following are the before and after stats I collected while performance testing this.   Admin Student Guest None Before the patch Files 547 204 187 186 Memory (MB) 69.3 61 57.1 57 get_string calls 1371 272 238 236 After the patch Files 547 151 133 132 Memory (MB) 69.3 54.9 50.7 50.6 get_string calls 1371 213 179 177 Benefits Less files 0 53 54 54 Less Memory (MB) 0 10% (6.1) 11% (6.4) 11% (6.4) Less get_string calls 0 59 59 59 Cheers Sam
          Hide
          Andrew Davis added a comment -

          Looks good. I have two queries/suggestions.

          1) At line 2583. This line...
          $settings = $this->load_user_settings($this->page->course->id);
          Is it possible to move that into the else clause at line 2589? The $settings variable doesn't seem to be used if the user is an admin.

          2) Im not sure if the logic around $SESSION->load_navigation_admin is entirely correct. If I log in as an admin, log out, then log in as a student the session variable remains set so the admin tree is loaded. Guessing that wont break anything but is sub-optimal.

          Show
          Andrew Davis added a comment - Looks good. I have two queries/suggestions. 1) At line 2583. This line... $settings = $this->load_user_settings($this->page->course->id); Is it possible to move that into the else clause at line 2589? The $settings variable doesn't seem to be used if the user is an admin. 2) Im not sure if the logic around $SESSION->load_navigation_admin is entirely correct. If I log in as an admin, log out, then log in as a student the session variable remains set so the admin tree is loaded. Guessing that wont break anything but is sub-optimal.
          Hide
          Sam Hemelryk added a comment -

          Hi Andrew,

          Thanks for looking at that, regarding the two points you raised:

          1. The line ''$settings = $this->load_user_settings($this->page->course->id);'' generates the users settings, that needs to occur on all pages regardless of the admin generation. The if/else later on is just to decide which settings branch (admin, or user) to open.
          2. I've just been double checking things there, the logic is correct, you log in as admin and browse around and we load the admin settings on all pages. If you then log out and log in as a student the admin settings get loaded once, we find there is nothing in it so it isn't loaded again. We still need to load it once to find out if there is anything in it.

          I've created PULL-204 to see this integrated.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi Andrew, Thanks for looking at that, regarding the two points you raised: The line ''$settings = $this->load_user_settings($this->page->course->id);'' generates the users settings, that needs to occur on all pages regardless of the admin generation. The if/else later on is just to decide which settings branch (admin, or user) to open. I've just been double checking things there, the logic is correct, you log in as admin and browse around and we load the admin settings on all pages. If you then log out and log in as a student the admin settings get loaded once, we find there is nothing in it so it isn't loaded again. We still need to load it once to find out if there is anything in it. I've created PULL-204 to see this integrated. Cheers Sam
          Hide
          Dongsheng Cai added a comment -


          Tested. Thanks!

          Show
          Dongsheng Cai added a comment - Tested. Thanks!

            People

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

              Dates

              • Created:
                Updated:
                Resolved: