Details

    • Rank:
      50164

      Description

      Inspired by Tim and Sam's work, I've just been having a play with xh_prof.

      I noticed that in admin_category we use $this->category_cache to cache records.

      This is a protected variable, which is set to an array in the constructor, and also in a purge function. The only operations on it are:

      • $this->category_cache = array();
      • isset($this->category_cache[$key])
      • $this->category_cache[$key] = $value;
      • return $this->category_cache[$key];
      • while($this->category_cache)
      • array_pop($this->category_cache);

      In every function which accesses it, we still call is_array($this->category_cache) even though it is only every treated as an array.

      On a default install of Moodle with no additional plugins, the locate() function is called around about 8,600 times with up to two is_array() calls.

      By removing this, I'm seeing savings of around about 100ms on my laptop.

      1. 39496-after.png
        161 kB
      2. 39496-before.png
        192 kB

        Activity

        Hide
        Andrew Nicols added a comment -

        After disabling debugging mode, the difference reduced to 5ms but still a small improvement.

        Callgraphs attached for reference.

        Show
        Andrew Nicols added a comment - After disabling debugging mode, the difference reduced to 5ms but still a small improvement. Callgraphs attached for reference.
        Hide
        Andrew Nicols added a comment -

        Sorry, these callgraphs after from a standard frontpage load.

        Show
        Andrew Nicols added a comment - Sorry, these callgraphs after from a standard frontpage load.
        Hide
        Sam Hemelryk added a comment -

        Big +1 from me Andrew.

        Code looks good and is a nice safe change.
        Worth noting is that these changes would still see things work perfectly even if category_cache were not an array.

        Many thanks
        Sam

        Show
        Sam Hemelryk added a comment - Big +1 from me Andrew. Code looks good and is a nice safe change. Worth noting is that these changes would still see things work perfectly even if category_cache were not an array. Many thanks Sam
        Hide
        Andrew Nicols added a comment -

        Thanks Sam,

        I'll leave it up to the iTeam to decide whether this should hit 2.5 or not.

        Show
        Andrew Nicols added a comment - Thanks Sam, I'll leave it up to the iTeam to decide whether this should hit 2.5 or not.
        Hide
        Dan Poltawski added a comment -

        Integrated to master, thanks Andrew.

        Show
        Dan Poltawski added a comment - Integrated to master, thanks Andrew.
        Hide
        Ankit Agarwal added a comment -

        Skipping unit tests as they are run as a part of integration.
        Admin links are working fine, didn't see any errors.
        Passing
        Thanks

        Show
        Ankit Agarwal added a comment - Skipping unit tests as they are run as a part of integration. Admin links are working fine, didn't see any errors. Passing Thanks
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Did you think this day was not going to arrive ever?

        Your patience has been rewarded, yay, sent upstream, thanks!

        Closing...ciao

        Show
        Eloy Lafuente (stronk7) added a comment - Did you think this day was not going to arrive ever? Your patience has been rewarded, yay, sent upstream, thanks! Closing...ciao
        Hide
        Jordi Pujol-Ahulló added a comment -

        Just a note:

        Seen from php.net: http://www.php.net/manual/en/function.is-array.php#98156

        It is a lot better (in performance) checking if something is an array as follows:

        (array)$sth === $sth

        Good job!!!

        Show
        Jordi Pujol-Ahulló added a comment - Just a note: Seen from php.net: http://www.php.net/manual/en/function.is-array.php#98156 It is a lot better (in performance) checking if something is an array as follows: (array)$sth === $sth Good job!!!
        Hide
        Andrew Nicols added a comment -

        Jordi Pujol-Ahulló It's difficult to ascertain that really. It depends on the size of the array. I've also heard of a heisenbug in this respect - if you have xhprof enable, it adds a higher overhead to functions than to array casting, even if it is loaded but disabled. I tried this myself recently with object casting and is_object() and the results were very mixed.

        Show
        Andrew Nicols added a comment - Jordi Pujol-Ahulló It's difficult to ascertain that really. It depends on the size of the array. I've also heard of a heisenbug in this respect - if you have xhprof enable, it adds a higher overhead to functions than to array casting, even if it is loaded but disabled. I tried this myself recently with object casting and is_object() and the results were very mixed.

          People

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

            Dates

            • Created:
              Updated:
              Resolved: