Details

      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.

        Gliffy Diagrams

        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: