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

          Attachments

            Activity

            Hide
            dobedobedoh Andrew Nicols added a comment -

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

            Callgraphs attached for reference.

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

            Sorry, these callgraphs after from a standard frontpage load.

            Show
            dobedobedoh Andrew Nicols added a comment - Sorry, these callgraphs after from a standard frontpage load.
            Hide
            samhemelryk 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
            samhemelryk 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
            dobedobedoh 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
            dobedobedoh 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
            poltawski Dan Poltawski added a comment -

            Integrated to master, thanks Andrew.

            Show
            poltawski Dan Poltawski added a comment - Integrated to master, thanks Andrew.
            Hide
            ankit_frenz 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_frenz 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
            stronk7 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
            stronk7 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
            jpahullo 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
            jpahullo 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
            dobedobedoh 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
            dobedobedoh 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:
                  Fix Release Date:
                  14/May/13