Details

    • Difficulty:
      Moderate
    • Affected Branches:
      MOODLE_20_STABLE
    • Rank:
      6363

      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

        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: