Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.4.3
    • Fix Version/s: 2.4.5, 2.5.1
    • Component/s: Roles / Access
    • Labels:
    • Rank:
      50114

      Description

      In a typical page request, is_siteadmin gets called more than 200 times. In my profiling this takes about 5ms.

      The function is not very efficient. I am investigating two approaches to improve performance with the aim of saving a couple of milliseconds.

        Activity

        Hide
        Sam Marshall added a comment -

        The slow part is explode and in_array on the config variable.

        Three approaches:

        1) Do list search using strpos and string concats instead of explode/in_array

        return strpos(',' . $CFG->siteadmins . ',', ',' . $userid . ',') !== false;
        

        2) Do list search using preg_match instead of explode/in_array

        return preg_match('~(^|,)' . $userid . '(,|$)~', $CFG->siteadmins);
        

        3) Static-cache the results for last userid

        In my core Moodle test environment (which makes fewer calls than my 'real-life' OU Moodle test page, there's only about 150), here are the times (using CPU rather than wall time and rounding to .1 ms):

        0) 5.0ms
        1) 2.0ms
        2) 1.0ms
        3) 0.0ms

        I was a bit doubtful about this so I did a second run of all four approaches in quick succession to see if results are consistent, this time including both the 'CPU time' and 'wall time' (second number):

        0) 4.0ms (2.6ms)
        1) 2.0ms (1.6ms)
        2) 2.0ms (1.9ms)
        3) 1.0ms (0.8ms)

        Conclusion is that static cache is fastest and the performance improvement is probably about 2ms on course view page.

        Show
        Sam Marshall added a comment - The slow part is explode and in_array on the config variable. Three approaches: 1) Do list search using strpos and string concats instead of explode/in_array return strpos(',' . $CFG->siteadmins . ',', ',' . $userid . ',') !== false ; 2) Do list search using preg_match instead of explode/in_array return preg_match('~(^|,)' . $userid . '(,|$)~', $CFG->siteadmins); 3) Static-cache the results for last userid In my core Moodle test environment (which makes fewer calls than my 'real-life' OU Moodle test page, there's only about 150), here are the times (using CPU rather than wall time and rounding to .1 ms): 0) 5.0ms 1) 2.0ms 2) 1.0ms 3) 0.0ms I was a bit doubtful about this so I did a second run of all four approaches in quick succession to see if results are consistent, this time including both the 'CPU time' and 'wall time' (second number): 0) 4.0ms (2.6ms) 1) 2.0ms (1.6ms) 2) 2.0ms (1.9ms) 3) 1.0ms (0.8ms) Conclusion is that static cache is fastest and the performance improvement is probably about 2ms on course view page.
        Hide
        Sam Marshall added a comment -

        Submitting for peer review with the 'static cache' approach which was fastest. Not quite so sure about this one, but it's still fairly simple code to save a couple milliseconds. Every little helps, right?

        Note: I reran the accesslib unit tests, they still pass.

        Show
        Sam Marshall added a comment - Submitting for peer review with the 'static cache' approach which was fastest. Not quite so sure about this one, but it's still fairly simple code to save a couple milliseconds. Every little helps, right? Note: I reran the accesslib unit tests, they still pass.
        Hide
        Petr Škoda added a comment -

        no more static caches! (it breaks unit tests and we are moving to MUC anyway)

        Show
        Petr Škoda added a comment - no more static caches! (it breaks unit tests and we are moving to MUC anyway)
        Hide
        Petr Škoda added a comment -

        -1

        Show
        Petr Škoda added a comment - -1
        Hide
        Petr Škoda added a comment -

        if you really want static cache you must also store the cfg->siteadmins in static var and validate it did not change....

        Show
        Petr Škoda added a comment - if you really want static cache you must also store the cfg->siteadmins in static var and validate it did not change....
        Hide
        Sam Marshall added a comment -

        MUC is itself really slow, that's part of the performance problems... OK, I will modify this to validate cfg->siteadmins and see if that still provides a performance gain.

        Show
        Sam Marshall added a comment - MUC is itself really slow, that's part of the performance problems... OK, I will modify this to validate cfg->siteadmins and see if that still provides a performance gain.
        Hide
        Sam Marshall added a comment - - edited

        Change updated as required so that static cache checks $CFG->siteadmins.

        I'm not really sure it is correct to prioritise simplicity of unit testing above performance (the previous change did provide a way to clear the cache, so it just made unit testing a bit more complex) but as this is a rather minor performance gain anyway, perhaps it's a moot point.

        I've added extra unit testing as part of this commit - the unit test before did not actually test the case with multiple site admins.

        Revised performance measurements from course page of my example site. I did two pairs of runs, master then this change, master then this change, in quick succession. The profiles are all with a student account who isn't admin, using basically stock moodle (master).

        (time in microseconds)

        2,408 -> 830
        2,485 -> 605

        i.e. saving ~1.7ms.

        This test is with a course that has 11 course_modules entries. I repeated the test on another course with 63 course_modules entries (still rather small, the median for OU courses is about 100, but that's the largest on my test server):

        3,600 -> 1,585
        3,335 -> 1,200

        So saving ~2.1ms in this case. Extrapolating to larger courses, such as with 1,000 activities (there are 6 on the OU system with more than this) would suggest a saving of about 8ms on those page loads. Woohoo.

        Note: is_siteadmin is almost exclusively used by has_capability, which always calls it with an integer id, so I also investigated whether it would be worth making a specific function is_siteadmin_id that removes all of the 'check if it's an object' stuff from the beginning of the function. My conclusion is that it isn't; there might be time savings but they're not really measurable within the error of my profiling.

        Show
        Sam Marshall added a comment - - edited Change updated as required so that static cache checks $CFG->siteadmins. I'm not really sure it is correct to prioritise simplicity of unit testing above performance (the previous change did provide a way to clear the cache, so it just made unit testing a bit more complex) but as this is a rather minor performance gain anyway, perhaps it's a moot point. I've added extra unit testing as part of this commit - the unit test before did not actually test the case with multiple site admins. Revised performance measurements from course page of my example site. I did two pairs of runs, master then this change, master then this change, in quick succession. The profiles are all with a student account who isn't admin, using basically stock moodle (master). (time in microseconds) 2,408 -> 830 2,485 -> 605 i.e. saving ~1.7ms. This test is with a course that has 11 course_modules entries. I repeated the test on another course with 63 course_modules entries (still rather small, the median for OU courses is about 100, but that's the largest on my test server): 3,600 -> 1,585 3,335 -> 1,200 So saving ~2.1ms in this case. Extrapolating to larger courses, such as with 1,000 activities (there are 6 on the OU system with more than this) would suggest a saving of about 8ms on those page loads. Woohoo. Note: is_siteadmin is almost exclusively used by has_capability, which always calls it with an integer id, so I also investigated whether it would be worth making a specific function is_siteadmin_id that removes all of the 'check if it's an object' stuff from the beginning of the function. My conclusion is that it isn't; there might be time savings but they're not really measurable within the error of my profiling.
        Hide
        Petr Škoda added a comment -

        looksok now, thanks, +1

        Show
        Petr Škoda added a comment - looksok now, thanks, +1
        Hide
        Andrew Nicols added a comment -

        As a further optimisation, we shoudl be able to hcange this to use isset($siteadmins[$userid]) instead of in_array - see the attached comparison of the two.

        Show
        Andrew Nicols added a comment - As a further optimisation, we shoudl be able to hcange this to use isset($siteadmins [$userid] ) instead of in_array - see the attached comparison of the two.
        Hide
        Andrew Nicols added a comment -

        For the record, I'm seeing the following on 1,000 checks of isset vs in_array:

        Generating test data... done
        Starting in_array test: Took 17.490947 seconds
        Starting isset test: Took 0.188568 seconds
        

        Sample code borrowed from http://blog.straylightrun.net/2008/12/03/tip-of-the-day-codeissetcode-vs-codein_arraycode/ and adjusted to give microtime comparisons.

        Show
        Andrew Nicols added a comment - For the record, I'm seeing the following on 1,000 checks of isset vs in_array: Generating test data... done Starting in_array test: Took 17.490947 seconds Starting isset test: Took 0.188568 seconds Sample code borrowed from http://blog.straylightrun.net/2008/12/03/tip-of-the-day-codeissetcode-vs-codein_arraycode/ and adjusted to give microtime comparisons.
        Hide
        Andrew Nicols added a comment -

        Of course, the explode would have to be changed too:

        $siteadmins = array_flip(explode(',', $CFG->siteadmins));
        $knownresult = isset($siteadmins[$userid]);
        return $knownresult;
        

        It may not be necessary to have a static cache if changed to isset.

        Show
        Andrew Nicols added a comment - Of course, the explode would have to be changed too: $siteadmins = array_flip(explode(',', $CFG->siteadmins)); $knownresult = isset($siteadmins[$userid]); return $knownresult; It may not be necessary to have a static cache if changed to isset.
        Hide
        Andrew Nicols added a comment -

        Bah - but because we can't cache the exploded siteadmins, the array_flip makes things slower when included within the loop.

        Show
        Andrew Nicols added a comment - Bah - but because we can't cache the exploded siteadmins, the array_flip makes things slower when included within the loop.
        Hide
        Sam Marshall added a comment -

        Andrew - I'll run the numbers for you (using that without the cache - with the static cache, there's definitely no point optimising the other bit as it only runs once). Hold on...

        Show
        Sam Marshall added a comment - Andrew - I'll run the numbers for you (using that without the cache - with the static cache, there's definitely no point optimising the other bit as it only runs once). Hold on...
        Hide
        Sam Marshall added a comment -

        actually if you've already decided it's slower, I'll skip it

        Show
        Sam Marshall added a comment - actually if you've already decided it's slower, I'll skip it
        Hide
        Andrew Nicols added a comment -

        Yup - Sam's way will be faster. Oh well, interesting to see how rubbish in_array is anyway.

        Show
        Andrew Nicols added a comment - Yup - Sam's way will be faster. Oh well, interesting to see how rubbish in_array is anyway.
        Hide
        Dan Poltawski added a comment -

        The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

        TIA and ciao

        Show
        Dan Poltawski added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
        Hide
        Dan Poltawski added a comment -

        This is really, really ugly.

        Show
        Dan Poltawski added a comment - This is really, really ugly.
        Hide
        Dan Poltawski added a comment -

        Hi,

        We're not so convinced of these micro optimisations in the integration team, which are making not so obviously good changes on their own, just tiny optimisations. Its really my thought that the gains are to be made at the higher levels and its particularly difficult to determine how measurable good they re for uglyness. For example, who knows how the actual measurement/lack of opcode cache might play into this.

        But really, this is bad time for us to have this debate, and so i'm adding integration_held and we will consider this after 2.5 release. Thanks for your understanding.

        Show
        Dan Poltawski added a comment - Hi, We're not so convinced of these micro optimisations in the integration team, which are making not so obviously good changes on their own, just tiny optimisations. Its really my thought that the gains are to be made at the higher levels and its particularly difficult to determine how measurable good they re for uglyness. For example, who knows how the actual measurement/lack of opcode cache might play into this. But really, this is bad time for us to have this debate, and so i'm adding integration_held and we will consider this after 2.5 release. Thanks for your understanding.
        Hide
        Sam Marshall added a comment -

        Hold after 2.5 is fine - just as long as the issue doesn't get lost.

        We definitely need to have a proper debate about this. For example, I don't accept that these micro-optimisations provide 'tiny' gains; they provide small gains proportionate to small effort and small risk. I do accept that larger-scale optimisations potentially have even better gains but they also have much larger required effort...

        Show
        Sam Marshall added a comment - Hold after 2.5 is fine - just as long as the issue doesn't get lost. We definitely need to have a proper debate about this. For example, I don't accept that these micro-optimisations provide 'tiny' gains; they provide small gains proportionate to small effort and small risk. I do accept that larger-scale optimisations potentially have even better gains but they also have much larger required effort...
        Hide
        Dan Poltawski added a comment -

        (We'll unhold all the integration_held issues after freeze, it won't get lost while waiting for integration )

        Show
        Dan Poltawski added a comment - (We'll unhold all the integration_held issues after freeze, it won't get lost while waiting for integration )
        Hide
        Sam Hemelryk added a comment -

        Thanks Sam, this has been integrated now.

        Could you please add testing instructions ASAP.

        Show
        Sam Hemelryk added a comment - Thanks Sam, this has been integrated now. Could you please add testing instructions ASAP.
        Hide
        Sam Marshall added a comment -

        Test instructions added. There is also a unit test.

        Regarding Dan's comment above, since this change was done we did learn more about profiler inaccuracy and variation between systems... I am no longer certain that it is going to provide a noticeable performance improvement (obviously it wouldn't hurt, though).

        Show
        Sam Marshall added a comment - Test instructions added. There is also a unit test. Regarding Dan's comment above, since this change was done we did learn more about profiler inaccuracy and variation between systems... I am no longer certain that it is going to provide a noticeable performance improvement (obviously it wouldn't hurt, though).
        Hide
        Jason Fowler added a comment -

        Works as described, thanks Sam!

        Show
        Jason Fowler added a comment - Works as described, thanks Sam!
        Hide
        Marina Glancy added a comment -

        Thanks for your awesome work! This has now become a part of Moodle.

        Closing as fixed!

        Show
        Marina Glancy added a comment - Thanks for your awesome work! This has now become a part of Moodle. Closing as fixed!

          People

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

            Dates

            • Created:
              Updated:
              Resolved: