Moodle
  1. Moodle
  2. MDL-40538

cachestore_static, cachestore_session: array_key_exists slow

    Details

    • Testing Instructions:
      Hide

      Before application (or revert if possible)

      1. site administration > settings > plugins > caching > test performance
      2. Run performance test with 10000 unique queries. Note the times for "session" and "static request" caches

      Apply Patch/ensure it's installed

      1. site administration > settings > plugins > caching > test performance
      2. Run performance test with 10000 unique queries. Note the times for "session" and "static request" caches

      3. Compare times

      4. Run phpunit tests.

      Please note that performance improvements will vary on the platform you test this on. For example time() performance varies depending on system architecture, eg VM or physical. array_key_exists will depending on CPU caches and may other things.

      On KVM on AMD Athlon(tm) 64 X2 Dual Core Processor 5200+ gave a 100x increase for hits on static cache.

      Show
      Before application (or revert if possible) 1. site administration > settings > plugins > caching > test performance 2. Run performance test with 10000 unique queries. Note the times for "session" and "static request" caches Apply Patch/ensure it's installed 1. site administration > settings > plugins > caching > test performance 2. Run performance test with 10000 unique queries. Note the times for "session" and "static request" caches 3. Compare times 4. Run phpunit tests. Please note that performance improvements will vary on the platform you test this on. For example time() performance varies depending on system architecture, eg VM or physical. array_key_exists will depending on CPU caches and may other things. On KVM on AMD Athlon(tm) 64 X2 Dual Core Processor 5200+ gave a 100x increase for hits on static cache.
    • Affected Branches:
      MOODLE_24_STABLE, MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE, MOODLE_25_STABLE
    • Pull 2.4 Branch:
    • Pull 2.5 Branch:
    • Pull Master Branch:
    • Rank:
      51361

      Description

      array_key_exists can be slow and can add substantial overhead to calls.

      time() can be an expensive call on some platforms.

      There calls should be out of the critical path in the static cache.

        Issue Links

          Activity

          Hide
          Russell Smith added a comment - - edited

          Hi Sam,

          I was doing some performance testing on backup/restore and using MUC MODE_REQUEST mode. the static cache was performing very poorly in my kvm hosted VM. See the attached graphs.

          The changes produce the graph with no strong contention on the static cache.

          Show
          Russell Smith added a comment - - edited Hi Sam, I was doing some performance testing on backup/restore and using MUC MODE_REQUEST mode. the static cache was performing very poorly in my kvm hosted VM. See the attached graphs. The changes produce the graph with no strong contention on the static cache.
          Hide
          Russell Smith added a comment -

          A sample test run of the test plan for static cache;

                                          Set     Get-Hit Get-Mis Delete
          Before: Static request cache 	0.0205 	67.3718 74.1980	0.0188
           After: Static request cache	0.0198 	 0.0160	74.2952	0.0190
          
          Show
          Russell Smith added a comment - A sample test run of the test plan for static cache; Set Get-Hit Get-Mis Delete Before: Static request cache 0.0205 67.3718 74.1980 0.0188 After: Static request cache 0.0198 0.0160 74.2952 0.0190
          Hide
          Russell Smith added a comment -

          Further results after altering session and file and others. I've not been able to test memcache or mongodb as I don't have them. They php lint fine. And unit tests pass.

          Before:
          File cache Tested 1.5577 0.3815 0.0789 0.2648
          File cache Tested 1.5976 0.5298 0.0810 0.3046
          Session cache Tested 0.0314 68.8976 64.5369 0.0192

          After:
          File cache Tested 1.7847 0.4116 0.0780 0.2582
          File cache Tested 1.5824 0.4368 0.0795 0.2628
          Session cache Tested 0.0321 0.0157 45.6193 0.0180

          Show
          Russell Smith added a comment - Further results after altering session and file and others. I've not been able to test memcache or mongodb as I don't have them. They php lint fine. And unit tests pass. Before: File cache Tested 1.5577 0.3815 0.0789 0.2648 File cache Tested 1.5976 0.5298 0.0810 0.3046 Session cache Tested 0.0314 68.8976 64.5369 0.0192 After: File cache Tested 1.7847 0.4116 0.0780 0.2582 File cache Tested 1.5824 0.4368 0.0795 0.2628 Session cache Tested 0.0321 0.0157 45.6193 0.0180
          Hide
          Russell Smith added a comment -

          A further note about the test environment. I read http://ilia.ws/archives/247-Performance-Analysis-of-isset-vs-array_key_exists.html and it indicates things like Xdebug cause issues here. My environment has XHprof enabled, but not Xdebug. This may be contributing to my large performance improvements. So the gains may not be the same without the profiling. I'm also running Ubuntu 12.04 64bit, PHP5.3.9 (default).

          XHprof is supposed to be able to run in a production environment with little overhead, so I believe these optimizations are worthwhile for those doing performance testing and to assist anybody running XHprof in production. It is very hard to test the benefits of a cache when the overhead of measuring it is so high. It's also affecting the caches that need to be the fastest, session and static.

          Show
          Russell Smith added a comment - A further note about the test environment. I read http://ilia.ws/archives/247-Performance-Analysis-of-isset-vs-array_key_exists.html and it indicates things like Xdebug cause issues here. My environment has XHprof enabled, but not Xdebug. This may be contributing to my large performance improvements. So the gains may not be the same without the profiling. I'm also running Ubuntu 12.04 64bit, PHP5.3.9 (default). XHprof is supposed to be able to run in a production environment with little overhead, so I believe these optimizations are worthwhile for those doing performance testing and to assist anybody running XHprof in production. It is very hard to test the benefits of a cache when the overhead of measuring it is so high. It's also affecting the caches that need to be the fastest, session and static.
          Hide
          Sam Hemelryk added a comment -

          Intrestingly enough I've not seen great performance differences here.
          I've run out of time tonight but this will be first on my list for tomorrow morning.

          Show
          Sam Hemelryk added a comment - Intrestingly enough I've not seen great performance differences here. I've run out of time tonight but this will be first on my list for tomorrow morning.
          Hide
          Russell Smith added a comment -

          Yes, I wouldn't expect you would get as much improvement due to the overhead I know I have in the VM setup. That's why I wanted a second test and to understand what environment you are testing it. These types of issues perform differently due to hardware and infrastructure changes. The context switching on CPU's for VM related tasks with profiling are probably costing quite a bit in my environment. Are you running real hardware, or virtualized? Do you have number you can post for me to see the difference?

          Show
          Russell Smith added a comment - Yes, I wouldn't expect you would get as much improvement due to the overhead I know I have in the VM setup. That's why I wanted a second test and to understand what environment you are testing it. These types of issues perform differently due to hardware and infrastructure changes. The context switching on CPU's for VM related tasks with profiling are probably costing quite a bit in my environment. Are you running real hardware, or virtualized? Do you have number you can post for me to see the difference?
          Hide
          Russell Smith added a comment -

          More evidence to support my theory;

          Both PC's have XHProf at version 0.9.3

          <?php
          
          xhprof_enable(XHPROF_FLAGS_CPU + XHPROF_FLAGS_MEMORY);
          $i = 0;
          $arr = array(); 
          $fp = fopen("/etc/services", "r"); 
          while ($i < 5000 && ($w = fgets($fp))) {
               $arr[trim($w)] = ++$i;
          }
          $s = microtime(1);
          for ($i = 0; $i < 100000; $i++) {
               isset($arr['abracadabra']);
          }
          
          $e = microtime(1);
          
          echo "Isset: ".($e - $s)."\n";
          
          $s = microtime(1);
          for ($i = 0; $i < 100000; $i++) {
               array_key_exists('abracadabra', $arr);
          }
          $e = microtime(1);
          echo "array_key_exists: ".($e - $s)."\n";
          

          Result:
          Isset: 0.016741991043091
          array_key_exists: 0.24929690361023

          Remove XHProf line:
          Isset: 0.022376775741577
          array_key_exists: 0.043159961700439

          Testing on my physical workstation and dual core Intel Pentium D

          Result:
          Isset: 0.014761209487915
          array_key_exists: 0.35524106025696

          Remove XHProf line:
          Isset: 0.015526056289673
          array_key_exists: 0.05945897102356

          So on my environment XHProf is adding significant overheads on isset vs array_key_exists. It's about 2x-3x faster under normal circumstances. In most parts of the code I wouldn't consider this worthwhile. But in what needs to be a really fast cache, the 2-3x win is significant for what could be 1 million calls. Given I expect most calls will result in a cache hit. It also provides the performance profiling benefit of not giving the false impression that the cache is the problem. I spent some time battling with that during the investigations into putting MUC into backup/restore.

          The lack of a function call in most circumstances of a speed critical code path is worth the 2-3x faster. As the stack overhead is much lower. My results up thread suggest it's slightly slow on misses. The question is are we optimizing the cache for the misses or hits case?

          From my perspective I'm in favour of integrating this on the basis that when doing performance analysis of other functions it's impossible to test with the overhead the array_key_exists introduces here. As MUC becomes more widely used and more people start paying attention to XHProf it will become something others run into when they shouldn't. If it can't be integrated I'll have to maintain this patch on my performance testing stack to ensure that I can get reasonable results. Without it, all the MUC Request and Session caches become unusable.

          I think that's all I'm going to be able to come up with for a case to integrate.

          Show
          Russell Smith added a comment - More evidence to support my theory; Both PC's have XHProf at version 0.9.3 <?php xhprof_enable(XHPROF_FLAGS_CPU + XHPROF_FLAGS_MEMORY); $i = 0; $arr = array(); $fp = fopen( "/etc/services" , "r" ); while ($i < 5000 && ($w = fgets($fp))) { $arr[trim($w)] = ++$i; } $s = microtime(1); for ($i = 0; $i < 100000; $i++) { isset($arr['abracadabra']); } $e = microtime(1); echo "Isset: " .($e - $s). "\n" ; $s = microtime(1); for ($i = 0; $i < 100000; $i++) { array_key_exists('abracadabra', $arr); } $e = microtime(1); echo "array_key_exists: " .($e - $s). "\n" ; Result: Isset: 0.016741991043091 array_key_exists: 0.24929690361023 Remove XHProf line: Isset: 0.022376775741577 array_key_exists: 0.043159961700439 Testing on my physical workstation and dual core Intel Pentium D Result: Isset: 0.014761209487915 array_key_exists: 0.35524106025696 Remove XHProf line: Isset: 0.015526056289673 array_key_exists: 0.05945897102356 So on my environment XHProf is adding significant overheads on isset vs array_key_exists. It's about 2x-3x faster under normal circumstances. In most parts of the code I wouldn't consider this worthwhile. But in what needs to be a really fast cache, the 2-3x win is significant for what could be 1 million calls. Given I expect most calls will result in a cache hit. It also provides the performance profiling benefit of not giving the false impression that the cache is the problem. I spent some time battling with that during the investigations into putting MUC into backup/restore. The lack of a function call in most circumstances of a speed critical code path is worth the 2-3x faster. As the stack overhead is much lower. My results up thread suggest it's slightly slow on misses. The question is are we optimizing the cache for the misses or hits case? From my perspective I'm in favour of integrating this on the basis that when doing performance analysis of other functions it's impossible to test with the overhead the array_key_exists introduces here. As MUC becomes more widely used and more people start paying attention to XHProf it will become something others run into when they shouldn't. If it can't be integrated I'll have to maintain this patch on my performance testing stack to ensure that I can get reasonable results. Without it, all the MUC Request and Session caches become unusable. I think that's all I'm going to be able to come up with for a case to integrate.
          Hide
          Russell Smith added a comment -

          https://bugs.php.net/bug.php?id=63736

          We don't use array references in $this->store do we? As that bug appears to explain the performance issue.

          I've tried to disable XHProf and couldn't make it faster. So I'm a little confused.

          Show
          Russell Smith added a comment - https://bugs.php.net/bug.php?id=63736 We don't use array references in $this->store do we? As that bug appears to explain the performance issue. I've tried to disable XHProf and couldn't make it faster. So I'm a little confused.
          Hide
          Tim Hunt added a comment -

          I really don't like (isset($this->keys[$key]) || array_key_exists($key, $this->keys)) everywhere. That is hard to understand.

          Oh. I see, you can't do (isset($this->keys[$key]) || $this->keys[$key] === null) because that will generate a notice.

          As we found, array_key_exists is only significantly slower than isset when you are profiling function calls, because array_key_exists is a function (and so has to be counted) and isset is not.

          The fact that isset is slower than array_key_exists (when using a PHP optimiser that should be able to inline the function call) is a PHP bug that we should expect to be fixed one day, leaving any messy work-around we add as a liability.

          Can $this->keys[$key] ever by null? If so, the I suggest you change the implemenation so that is not true, and then you can just use isset($this->keys[$key]), which is simiple code and fast.

          At the very least, if you insist on submtiting this for integration, you need to add comments explainging what on earth is going on in this code.

          Show
          Tim Hunt added a comment - I really don't like (isset($this->keys [$key] ) || array_key_exists($key, $this->keys)) everywhere. That is hard to understand. Oh. I see, you can't do (isset($this->keys [$key] ) || $this->keys [$key] === null) because that will generate a notice. As we found, array_key_exists is only significantly slower than isset when you are profiling function calls, because array_key_exists is a function (and so has to be counted) and isset is not. The fact that isset is slower than array_key_exists (when using a PHP optimiser that should be able to inline the function call) is a PHP bug that we should expect to be fixed one day, leaving any messy work-around we add as a liability. Can $this->keys [$key] ever by null? If so, the I suggest you change the implemenation so that is not true, and then you can just use isset($this->keys [$key] ), which is simiple code and fast. At the very least, if you insist on submtiting this for integration, you need to add comments explainging what on earth is going on in this code.
          Hide
          Sam Hemelryk added a comment -

          Hi guys,

          My results can be found here: https://docs.google.com/file/d/0B1oqmBH3MKqCYkczckZDYks1Z1U/edit?usp=sharing

          I've tested on just PHP 5.3.26 however I have tested with both Zend Opcache and xhprof combinations.
          In general this is resulting in a minor degradation in all situations except where we are hitting the improved code for the session and static caches.
          In that one situation the is a huge improvement in performance of the code.

          As Tim noted this area is well recognised as an issue when profiling. I was interested to see however that with all performance extension disabled I was still seeing a notable increase in code performance with this change for those two noted situations.
          However Tim is right as this is such a widely recognised issue likely we'll see improvements for it somewhere along the line.
          In regards to it ever being a null value while there is no code that I am aware of that stores null's in the cache it is possible that one day code will, and as I am concerned it should be able to (although I cannot imagine a use case for this I'm sure one exists).
          The defined "empty" value for a cache is false, and that is the one value that cannot be stored.
          There has been discussion in the past about changing this to null, however it's always ended up leaning back towards false so I would expect that to change any time soon.
          I think Tim is right, the best solution would be to work out some way of ensuring null is never stored as a value and then simplifying code checks to isset.
          Perhaps we could use an "cache_null" object in place of a null value when storing? It would add overhead should someone store null but we would be able to easily improve performance in all other situations by making changes like this. What do you guys think?

          As it stands perhaps we land this change for the session/static classes (leaving the rest alone), backport it (tested as safe) and then open a new issue for the above.
          We could of course add comments to explain this code as well, although truthfully I found it quite clear - but perhaps that is because I had my head in that mindframe and the array_key_exists issue has been targeted in several changes lately.

          Many thanks
          Sam

          Show
          Sam Hemelryk added a comment - Hi guys, My results can be found here: https://docs.google.com/file/d/0B1oqmBH3MKqCYkczckZDYks1Z1U/edit?usp=sharing I've tested on just PHP 5.3.26 however I have tested with both Zend Opcache and xhprof combinations. In general this is resulting in a minor degradation in all situations except where we are hitting the improved code for the session and static caches. In that one situation the is a huge improvement in performance of the code. As Tim noted this area is well recognised as an issue when profiling. I was interested to see however that with all performance extension disabled I was still seeing a notable increase in code performance with this change for those two noted situations. However Tim is right as this is such a widely recognised issue likely we'll see improvements for it somewhere along the line. In regards to it ever being a null value while there is no code that I am aware of that stores null's in the cache it is possible that one day code will, and as I am concerned it should be able to (although I cannot imagine a use case for this I'm sure one exists). The defined "empty" value for a cache is false, and that is the one value that cannot be stored. There has been discussion in the past about changing this to null, however it's always ended up leaning back towards false so I would expect that to change any time soon. I think Tim is right, the best solution would be to work out some way of ensuring null is never stored as a value and then simplifying code checks to isset. Perhaps we could use an "cache_null" object in place of a null value when storing? It would add overhead should someone store null but we would be able to easily improve performance in all other situations by making changes like this. What do you guys think? As it stands perhaps we land this change for the session/static classes (leaving the rest alone), backport it (tested as safe) and then open a new issue for the above. We could of course add comments to explain this code as well, although truthfully I found it quite clear - but perhaps that is because I had my head in that mindframe and the array_key_exists issue has been targeted in several changes lately. Many thanks Sam
          Hide
          Russell Smith added a comment -

          I agree with the moving to null if possible. And a separate issue is required as it will be master only and require significantly more discussion and/or agreement due to the behaviour change.

          However I think as Sam says it would be good to put the change in all branches. The numbers Sam posted are like mine and anybody using those caches really need that performance back.

          So I'm clear on what should be in and out, can you clarify that Sam?

          My understanding so far is;

          1. Remove all changes except for static and session stores.
          2. Leave the code for static and session as submitted.
          3. Add comments to the code explaining.

          On (3), what is the level of comment, I agree most of the isset() || is clear. the !isset() && !array_key_exists could be explained. And is a comment at the top of the file sufficient? Or should one be put in every location I changed.

          Not much comment has been made about the time() change I made, so direction there would be helpful. Overall I'm in favour of it. Tim says micro optimizations like this can cause problems. However of all the places in the code, this is the one I feed that kind of optimization is worthwhile.

          Show
          Russell Smith added a comment - I agree with the moving to null if possible. And a separate issue is required as it will be master only and require significantly more discussion and/or agreement due to the behaviour change. However I think as Sam says it would be good to put the change in all branches. The numbers Sam posted are like mine and anybody using those caches really need that performance back. So I'm clear on what should be in and out, can you clarify that Sam? My understanding so far is; 1. Remove all changes except for static and session stores. 2. Leave the code for static and session as submitted. 3. Add comments to the code explaining. On (3), what is the level of comment, I agree most of the isset() || is clear. the !isset() && !array_key_exists could be explained. And is a comment at the top of the file sufficient? Or should one be put in every location I changed. Not much comment has been made about the time() change I made, so direction there would be helpful. Overall I'm in favour of it. Tim says micro optimizations like this can cause problems. However of all the places in the code, this is the one I feed that kind of optimization is worthwhile.
          Hide
          Russell Smith added a comment -

          Further on the null discussion;

              /**
               * Deletes an item from the cache store.
               *
               * @param string $key The key to delete.
               * @return bool Returns true if the operation was a success, false otherwise.
               */
              public function delete($key) {
                  $result = isset($this->store[$key]);
                  unset($this->store[$key]);
                  return $result;
              }
          

          The delete code will return false under null conditions. Is that also a bug? As it will report that the value was never stored if it was null. Maybe it doesn't matter?

          Show
          Russell Smith added a comment - Further on the null discussion; /** * Deletes an item from the cache store. * * @param string $key The key to delete. * @ return bool Returns true if the operation was a success, false otherwise. */ public function delete($key) { $result = isset($ this ->store[$key]); unset($ this ->store[$key]); return $result; } The delete code will return false under null conditions. Is that also a bug? As it will report that the value was never stored if it was null. Maybe it doesn't matter?
          Hide
          Russell Smith added a comment -

          I have published the new revision that includes changes to only static and session. A comment was added with this issue as a reference. Please provide feedback.

          Show
          Russell Smith added a comment - I have published the new revision that includes changes to only static and session. A comment was added with this issue as a reference. Please provide feedback.
          Hide
          Russell Smith added a comment -

          I had another possible solution come to me this morning;

          
              public function set($key, $data) {
                  if ($this->ttl == 0) {
                      $this->store[$key] = $data;
                  } else {
                      $this->store[$key] = array($data, cache::now());
                  }
                  return true;
              }
          

          At the moment, if there is a TTL, then isset will work in all cases, including null. As the null is stored in $this->store[$key][0]. Is it reasonable to use a single element array to store the value when there is no TTL? I think that's acceptable. You can the remove all array_key_exists from the critical path. As the code for isset will either find nothing or an array all the time

          Thoughts? If people agree, I'll change the patch to that.

          Show
          Russell Smith added a comment - I had another possible solution come to me this morning; public function set($key, $data) { if ($ this ->ttl == 0) { $ this ->store[$key] = $data; } else { $ this ->store[$key] = array($data, cache::now()); } return true ; } At the moment, if there is a TTL, then isset will work in all cases, including null. As the null is stored in $this->store [$key] [0] . Is it reasonable to use a single element array to store the value when there is no TTL? I think that's acceptable. You can the remove all array_key_exists from the critical path. As the code for isset will either find nothing or an array all the time Thoughts? If people agree, I'll change the patch to that.
          Hide
          Sam Hemelryk added a comment -

          I really like that, thats a nice approach that could be easily replaced should we come up with a better solution in the future.
          I don't imagine there is much cost involved in instantiating an array.

          Show
          Sam Hemelryk added a comment - I really like that, thats a nice approach that could be easily replaced should we come up with a better solution in the future. I don't imagine there is much cost involved in instantiating an array.
          Hide
          Russell Smith added a comment -
          Master code (10000):
          Type 	Plugin 		Result 	Set 	Get - Hit 	Get - Miss 	Delete
          App	File cache 	Tested 	1.5653 	0.3938 		0.0778 		0.2799
          Session	File cache 	Tested 	1.9445 	0.3779 		0.0760 		0.2752
          Session	Session cache 	Tested 	0.0283 	40.5774 	42.7253 	0.0182
          Request	Static request 	Tested 	0.0202 	44.6557 	41.0206 	0.0181
          
          New isset method installed (10000):
          Type 	Plugin 		Result 	Set 	Get - Hit 	Get - Miss 	Delete
          App	File cache 	Tested 	1.8585 	0.4359 		0.0786 		0.2967
          Session	File cache 	Tested 	2.0814 	0.3807 		0.0750 		0.2976
          Session	Session cache 	Tested 	0.0291 	0.0478 		0.0093 		0.0259
          Request	Static request 	Tested 	0.0413 	0.0420 		0.0107 		0.0273
          

          This is definitely an improvement for both get and set. Nice to see the memory cache performing better than the filesystem . In my view, we are ready for final peer review and integration. I've backpatched this version as it's a clear win and should be used on all stable branches.

          I don't have any of the other cachestores like mongodb or a memcache variety. If you are able to do a test with those Sam, that would be appreciated.

          Show
          Russell Smith added a comment - Master code (10000): Type Plugin Result Set Get - Hit Get - Miss Delete App File cache Tested 1.5653 0.3938 0.0778 0.2799 Session File cache Tested 1.9445 0.3779 0.0760 0.2752 Session Session cache Tested 0.0283 40.5774 42.7253 0.0182 Request Static request Tested 0.0202 44.6557 41.0206 0.0181 New isset method installed (10000): Type Plugin Result Set Get - Hit Get - Miss Delete App File cache Tested 1.8585 0.4359 0.0786 0.2967 Session File cache Tested 2.0814 0.3807 0.0750 0.2976 Session Session cache Tested 0.0291 0.0478 0.0093 0.0259 Request Static request Tested 0.0413 0.0420 0.0107 0.0273 This is definitely an improvement for both get and set. Nice to see the memory cache performing better than the filesystem . In my view, we are ready for final peer review and integration. I've backpatched this version as it's a clear win and should be used on all stable branches. I don't have any of the other cachestores like mongodb or a memcache variety. If you are able to do a test with those Sam, that would be appreciated.
          Hide
          Sam Hemelryk added a comment -

          Hi Russel,

          This definitely get a +1 from me now.

          The following are the results of my testing:

          Current master - unpatched - 10000 requests

          nothing enabled Session cache 0.0123 60.6398 60.9693 0.0113
          nothing enabled Static request cache 0.0132 8.3958 8.4739 0.0130
          opcache enabled Session cache 0.0140 7.8627 7.8650 0.0109
          opcache enabled Static request cache 0.0123 7.9277 7.9886 0.0121
          xhprof enabled Session cache 0.0135 8.1110 7.8490 0.0111
          xhprof enabled Static request cache 0.0115 7.9826 8.0489 0.0122

          Patched master - 10000 requests

          nothing enabled Session cache 0.0192 0.0198 0.0062 0.0126
          nothing enabled Static request cache 0.0160 0.0164 0.0072 0.0125
          opcache enabled Session cache 0.0186 0.0192 0.0061 0.0124
          opcache enabled Static request cache 0.0161 0.0146 0.0076 0.0125
          xhprof enabled Session cache 0.0186 0.0200 0.0063 0.0118
          xhprof enabled Static request cache 0.0153 0.0148 0.0066 0.0130

          As none of the application caches have been altered now I've not included there test results. Just to confirm however as expected their performance has not been affected by this change.
          All branches tested and unit tests run - appears to work perfectly.

          I'm going to submit this straight to integration

          Many thanks
          Sam

          Show
          Sam Hemelryk added a comment - Hi Russel, This definitely get a +1 from me now. The following are the results of my testing: Current master - unpatched - 10000 requests nothing enabled Session cache 0.0123 60.6398 60.9693 0.0113 nothing enabled Static request cache 0.0132 8.3958 8.4739 0.0130 opcache enabled Session cache 0.0140 7.8627 7.8650 0.0109 opcache enabled Static request cache 0.0123 7.9277 7.9886 0.0121 xhprof enabled Session cache 0.0135 8.1110 7.8490 0.0111 xhprof enabled Static request cache 0.0115 7.9826 8.0489 0.0122 Patched master - 10000 requests nothing enabled Session cache 0.0192 0.0198 0.0062 0.0126 nothing enabled Static request cache 0.0160 0.0164 0.0072 0.0125 opcache enabled Session cache 0.0186 0.0192 0.0061 0.0124 opcache enabled Static request cache 0.0161 0.0146 0.0076 0.0125 xhprof enabled Session cache 0.0186 0.0200 0.0063 0.0118 xhprof enabled Static request cache 0.0153 0.0148 0.0066 0.0130 As none of the application caches have been altered now I've not included there test results. Just to confirm however as expected their performance has not been affected by this change. All branches tested and unit tests run - appears to work perfectly. I'm going to submit this straight to integration Many thanks Sam
          Hide
          Sam Hemelryk added a comment -

          Up for integration now.

          Show
          Sam Hemelryk added a comment - Up for integration now.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          I like the trick of always storing an array ($value, [$timecreated]). That way we workaround and continue supporting stored nulls perfectly (that was one of the main problems in my mind to get this accepted). Good idea!

          Just an observation, personally, for readability, I prefer the:

          $maxtime = cache::now() - $this->ttl;
          

          line to be calculated unconditionally always, instead of doing it conditionally sometimes and "inline" other times. I'm sure it does not cause any appreciable slowdown and makes all methods more "similar".

          But that's only a personal taste. So +1 too.

          Show
          Eloy Lafuente (stronk7) added a comment - I like the trick of always storing an array ($value, [$timecreated] ). That way we workaround and continue supporting stored nulls perfectly (that was one of the main problems in my mind to get this accepted). Good idea! Just an observation, personally, for readability, I prefer the: $maxtime = cache::now() - $ this ->ttl; line to be calculated unconditionally always, instead of doing it conditionally sometimes and "inline" other times. I'm sure it does not cause any appreciable slowdown and makes all methods more "similar". But that's only a personal taste. So +1 too.
          Hide
          Russell Smith added a comment -

          Normally I would not make the time based changes I did in other places in the code. I agree it's not pretty. I see the caching layer as a critically performing layer that needs every bit of optimization possible. It needs to be fast as we scale to bigger and bigger installations. Items like cache::now() start to matter in the critical path at large numbers of requests. It was very easy to find this when I was using it for 10,000,000 get calls

          The performance gain here shows we have not spent significant time yet optimizing all of the cachestore's performance. This issue skipped through as we only had 1 static cache in the core (mod_assign) code at the time I discovered this.

          So even going forward, the cache code is one place I would vote for ugly fast code every time. Other places I'm much more likely to vote for easier similar and pretty.

          Show
          Russell Smith added a comment - Normally I would not make the time based changes I did in other places in the code. I agree it's not pretty. I see the caching layer as a critically performing layer that needs every bit of optimization possible. It needs to be fast as we scale to bigger and bigger installations. Items like cache::now() start to matter in the critical path at large numbers of requests. It was very easy to find this when I was using it for 10,000,000 get calls The performance gain here shows we have not spent significant time yet optimizing all of the cachestore's performance. This issue skipped through as we only had 1 static cache in the core (mod_assign) code at the time I discovered this. So even going forward, the cache code is one place I would vote for ugly fast code every time. Other places I'm much more likely to vote for easier similar and pretty.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          don't agree, I'm all about real optimizations - like moving to isset() while keeping NULLs working, but not about zuper-micro-optimizations only causing code to be hard to understand and not producing any benefit.

          In any case, as said, I'm not against current patch, just wanted to share my personal POV about some of the changes there. Aka, +1.

          Thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - don't agree, I'm all about real optimizations - like moving to isset() while keeping NULLs working, but not about zuper-micro-optimizations only causing code to be hard to understand and not producing any benefit. In any case, as said, I'm not against current patch, just wanted to share my personal POV about some of the changes there. Aka, +1. Thanks!
          Hide
          Russell Smith added a comment -

          I'm looking at implementing maxsize for this cachestore. My review of cache::now() and this store being MODE_REQUEST only indicates that ttl can't be implemented. As the first request for time() will fix the time for the entire request. All future requests will work with the same cache::now() time. So it raises the question of whether this store should support ttl at all? Or am I missing something in my understanding of this.

          Show
          Russell Smith added a comment - I'm looking at implementing maxsize for this cachestore. My review of cache::now() and this store being MODE_REQUEST only indicates that ttl can't be implemented. As the first request for time() will fix the time for the entire request. All future requests will work with the same cache::now() time. So it raises the question of whether this store should support ttl at all? Or am I missing something in my understanding of this.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          aha, that's a VERY good point. Which is the rationale for a static-cache ttl if "now" is statically defined. Uhm... Sam?

          Show
          Eloy Lafuente (stronk7) added a comment - aha, that's a VERY good point. Which is the rationale for a static-cache ttl if "now" is statically defined. Uhm... Sam?
          Hide
          Sam Hemelryk added a comment -

          In general we advise against using TTL with any cache definition in preference to an event driven invalidation system instead.
          To me it makes no sense to use a ttl with a request cache, ttl introduces overhead and the only potential reason I can see to make use of ttl there would be memory management for which I think maxsize is a much better system than ttl for a request cache.

          TTL is hard coded into the cache_request loader by means on inheritance.
          I'm all for removing of nullifying ttl directives given to request caches in favour of a maxsize option.

          Show
          Sam Hemelryk added a comment - In general we advise against using TTL with any cache definition in preference to an event driven invalidation system instead. To me it makes no sense to use a ttl with a request cache, ttl introduces overhead and the only potential reason I can see to make use of ttl there would be memory management for which I think maxsize is a much better system than ttl for a request cache. TTL is hard coded into the cache_request loader by means on inheritance. I'm all for removing of nullifying ttl directives given to request caches in favour of a maxsize option.
          Hide
          Damyon Wiese added a comment -

          Thanks Russell,

          It seems 100% safe and passes all unit tests on all branches so I have integrated this to master, 25 and 24.

          Show
          Damyon Wiese added a comment - Thanks Russell, It seems 100% safe and passes all unit tests on all branches so I have integrated this to master, 25 and 24.
          Hide
          Michael de Raadt added a comment -

          Test result: Success!

          No significant change in Set/Delete times, but Get times were significantly improved. Well done!

          Unit tests passed in master, 25 and 24.

          Master before upgrade

          Plugin Result Set Get - Hit Get - Miss Delete
          Session cache Tested 0.0092 7.8151 7.8102 0.0088
          Static request cache Tested 0.0111 7.8215 7.8379 0.0075

          Master after upgrade

          Plugin Result Set Get - Hit Get - Miss Delete
          Session cache Tested 0.0112 0.0109 0.0046 0.0120
          Static request cache Tested 0.0130 0.0079 0.0057 0.0081

          25 before upgrade

          Plugin Result Set Get - Hit Get - Miss Delete
          Session cache Tested 0.0082 7.6543 7.7716 0.0078
          Static request cache Tested 0.0109 7.7667 7.6305 0.0074

          25 after upgrade

          Plugin Result Set Get - Hit Get - Miss Delete
          Session cache Tested 0.0134 0.0118 0.0050 0.0085
          Static request cache Tested 0.0145 0.0128 0.0053 0.0087

          24 before upgrade

          Plugin Result Set Get - Hit Get - Miss Delete
          Session cache Tested 0.0227 8.6240 8.5268 0.0065
          Static request cache Tested 0.0091 8.4468 8.5295 0.0067

          24 after upgrade

          Plugin Result Set Get - Hit Get - Miss Delete
          Session cache Tested 0.0125 0.0116 0.0053 0.0082
          Static request cache Tested 0.0113 0.0080 0.0047 0.0076
          Show
          Michael de Raadt added a comment - Test result: Success! No significant change in Set/Delete times, but Get times were significantly improved. Well done! Unit tests passed in master, 25 and 24. Master before upgrade Plugin Result Set Get - Hit Get - Miss Delete Session cache Tested 0.0092 7.8151 7.8102 0.0088 Static request cache Tested 0.0111 7.8215 7.8379 0.0075 Master after upgrade Plugin Result Set Get - Hit Get - Miss Delete Session cache Tested 0.0112 0.0109 0.0046 0.0120 Static request cache Tested 0.0130 0.0079 0.0057 0.0081 25 before upgrade Plugin Result Set Get - Hit Get - Miss Delete Session cache Tested 0.0082 7.6543 7.7716 0.0078 Static request cache Tested 0.0109 7.7667 7.6305 0.0074 25 after upgrade Plugin Result Set Get - Hit Get - Miss Delete Session cache Tested 0.0134 0.0118 0.0050 0.0085 Static request cache Tested 0.0145 0.0128 0.0053 0.0087 24 before upgrade Plugin Result Set Get - Hit Get - Miss Delete Session cache Tested 0.0227 8.6240 8.5268 0.0065 Static request cache Tested 0.0091 8.4468 8.5295 0.0067 24 after upgrade Plugin Result Set Get - Hit Get - Miss Delete Session cache Tested 0.0125 0.0116 0.0053 0.0082 Static request cache Tested 0.0113 0.0080 0.0047 0.0076
          Hide
          Damyon Wiese added a comment -

          Moodle has many old functions,
          And although they cause no malfunction,
          There comes a day,
          When they get deprecated away,
          And get and put on the list for expulsion.

          Thanks for all the reports/testing/fixes this week. This issue has been sent upstream.

          Show
          Damyon Wiese added a comment - Moodle has many old functions, And although they cause no malfunction, There comes a day, When they get deprecated away, And get and put on the list for expulsion. Thanks for all the reports/testing/fixes this week. This issue has been sent upstream.

            People

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

              Dates

              • Created:
                Updated:
                Resolved: