Moodle
  1. Moodle
  2. MDL-40903

Split the persistent cache option into logical parts.

    Details

    • Rank:
      51806

      Description

      Presently setting the persistent cache option has two effects:

      1. The cache instance is held onto and future requests for it get given the original instance.
      2. Information passing through the cache is held onto in a static var for performance serving of subsequent requests.

      It's become apparent that really we need two options here.
      For example Fred's changes on MDL-13114.
      It makes no sense to use the static var for request caches - and doing so adds both processing and memory overhead.

      This change needs to be made and immediately applied to MDL-13114

        Issue Links

          Activity

          Hide
          Russell Smith added a comment -

          I've wondered why we can't just put a request cache on top of the other cache levels to produce what we want. You then can limit the size of the static cache in memory. Which could be a win in places like the modinfo cache
          Thoughts?

          Show
          Russell Smith added a comment - I've wondered why we can't just put a request cache on top of the other cache levels to produce what we want. You then can limit the size of the static cache in memory. Which could be a win in places like the modinfo cache Thoughts?
          Hide
          Sam Hemelryk added a comment -

          Sorry Russell - just saw your comment and I'm heading off for the night - I'll have a good think first thing tomorrow and let you know what I come up with.

          Show
          Sam Hemelryk added a comment - Sorry Russell - just saw your comment and I'm heading off for the night - I'll have a good think first thing tomorrow and let you know what I come up with.
          Hide
          Marina Glancy added a comment -

          Sam, I did some testing and reviewing, some questions I have.

          1. Why would anybody want loader to be not persistent? You explain in comment why we would want it to be persistent but it does not look like there is any downsides
          2. Can I still set persistentmaxsize for request cache? I don't want to run out of memory.
          3. I made tests on session cache and there is absolutely no difference in both get() and set() time whether the cache has persistentdata or not (using session cache)
          4. If I try to set/get many big objects in session cache during one request it consumes the RAM and very soon I get out of memory exception

          This is my test:

              $time_start = microtime(true);
              for ($i = 0; $i<$count; $i++) {
                  $cache->set($i, $record);
                  $cache->get($i);
              }
              $time_end = microtime(true);
              echo "$cachename: Average cache set time $count requests: ".(($time_end - $time_start)/$count*1000)." ms<br>";    
          

          Cache definitions:

              'cache1' => array(
                  'mode' => cache_store::MODE_SESSION,
                  'persistentloader' => true,
                  'simplekeys' => true,
                  'persistentmaxsize' => 10,
              ),
              'cache2' => array(
                  'mode' => cache_store::MODE_SESSION,
                  'persistentloader' => true,
                  'persistentdata' => true,
                  'simplekeys' => true,
                  'persistentmaxsize' => 10,
              ),
              'cache3' => array(
                  'mode' => cache_store::MODE_REQUEST,
                  'persistentloader' => true,
                  'persistentdata' => true,
                  'simplekeys' => true,
                  'persistentmaxsize' => 10,
              ),
          

          And results:

          cache1: Average cache set time 50 requests: 12.697396278381 ms, RAM: 40.6MB RAM peak: 50.6MB
          cache1: Average cache set time 80 requests: 19.553700089455 ms, RAM: 49.1MB RAM peak: 65.2MB
          
          cache2: Average cache set time 50 requests: 12.550940513611 ms, RAM: 36.1MB RAM peak: 46.1MB
          cache2: Average cache set time 80 requests: 20.269224047661 ms, RAM: 53.2MB RAM peak: 69.3MB
          
          cache3: Average cache set time 50 requests: 0.77355861663818 ms, RAM: 49.1MB RAM peak: 49.5MB
          cache3: Average cache set time 80 requests: 0.88483989238739 ms, RAM: 53.2MB RAM peak: 53.6MB
          cache3: Average cache set time 200 requests: 0.79622983932495 ms, RAM: 69.7MB RAM peak: 70.1MB
          
          Show
          Marina Glancy added a comment - Sam, I did some testing and reviewing, some questions I have. 1. Why would anybody want loader to be not persistent? You explain in comment why we would want it to be persistent but it does not look like there is any downsides 2. Can I still set persistentmaxsize for request cache? I don't want to run out of memory. 3. I made tests on session cache and there is absolutely no difference in both get() and set() time whether the cache has persistentdata or not (using session cache) 4. If I try to set/get many big objects in session cache during one request it consumes the RAM and very soon I get out of memory exception This is my test: $time_start = microtime( true ); for ($i = 0; $i<$count; $i++) { $cache->set($i, $record); $cache->get($i); } $time_end = microtime( true ); echo "$cachename: Average cache set time $count requests: " .(($time_end - $time_start)/$count*1000). " ms<br>" ; Cache definitions: 'cache1' => array( 'mode' => cache_store::MODE_SESSION, 'persistentloader' => true , 'simplekeys' => true , 'persistentmaxsize' => 10, ), 'cache2' => array( 'mode' => cache_store::MODE_SESSION, 'persistentloader' => true , 'persistentdata' => true , 'simplekeys' => true , 'persistentmaxsize' => 10, ), 'cache3' => array( 'mode' => cache_store::MODE_REQUEST, 'persistentloader' => true , 'persistentdata' => true , 'simplekeys' => true , 'persistentmaxsize' => 10, ), And results: cache1: Average cache set time 50 requests: 12.697396278381 ms, RAM: 40.6MB RAM peak: 50.6MB cache1: Average cache set time 80 requests: 19.553700089455 ms, RAM: 49.1MB RAM peak: 65.2MB cache2: Average cache set time 50 requests: 12.550940513611 ms, RAM: 36.1MB RAM peak: 46.1MB cache2: Average cache set time 80 requests: 20.269224047661 ms, RAM: 53.2MB RAM peak: 69.3MB cache3: Average cache set time 50 requests: 0.77355861663818 ms, RAM: 49.1MB RAM peak: 49.5MB cache3: Average cache set time 80 requests: 0.88483989238739 ms, RAM: 53.2MB RAM peak: 53.6MB cache3: Average cache set time 200 requests: 0.79622983932495 ms, RAM: 69.7MB RAM peak: 70.1MB
          Hide
          Marina Glancy added a comment -

          The same test on current integration with cache definition

              'cache4' => array(
                  'mode' => cache_store::MODE_REQUEST,
                  'persistent' => true,
                  'simplekeys' => true,
                  'persistentmaxsize' => 10,
              ),
          

          result:

          cache4: Average cache set time 50 requests: 0.73930263519287 ms, RAM: 49.2MB RAM peak: 49.6MB
          cache4: Average cache set time 80 requests: 0.7015734910965 ms, RAM: 53.3MB RAM peak: 53.7MB
          cache4: Average cache set time 200 requests: 0.74580550193787 ms, RAM: 69.8MB RAM peak: 70.2MB
          

          so there are no changes and the same memory leak

          Show
          Marina Glancy added a comment - The same test on current integration with cache definition 'cache4' => array( 'mode' => cache_store::MODE_REQUEST, 'persistent' => true , 'simplekeys' => true , 'persistentmaxsize' => 10, ), result: cache4: Average cache set time 50 requests: 0.73930263519287 ms, RAM: 49.2MB RAM peak: 49.6MB cache4: Average cache set time 80 requests: 0.7015734910965 ms, RAM: 53.3MB RAM peak: 53.7MB cache4: Average cache set time 200 requests: 0.74580550193787 ms, RAM: 69.8MB RAM peak: 70.2MB so there are no changes and the same memory leak
          Hide
          Marina Glancy added a comment -

          sorry, forgot to mention: The time is for the script execution only. The RAM is taken from page information so it contains some moodlepage overhead (logged in as student)

          Show
          Marina Glancy added a comment - sorry, forgot to mention: The time is for the script execution only. The RAM is taken from page information so it contains some moodlepage overhead (logged in as student)
          Hide
          Damyon Wiese added a comment -

          Hi Sam,

          Taking a look at this too:

          The name for the option is a bit confusing - persistentdata - is not really about persistence at all - it is about a non-persistent, in memory cache sitting on top of the store.

          I also agree with Marina - the memory cost of the loader instance is small and don't see any downsides to always caching this and not having an option for it.

          The function should_be_persistent has not been deprecated properly - although annoying I think it should print a debugging message (if it stays the same as in this patch).

          I'll do a bit of testing on this and post some results in a bit.

          Show
          Damyon Wiese added a comment - Hi Sam, Taking a look at this too: The name for the option is a bit confusing - persistentdata - is not really about persistence at all - it is about a non-persistent, in memory cache sitting on top of the store. I also agree with Marina - the memory cost of the loader instance is small and don't see any downsides to always caching this and not having an option for it. The function should_be_persistent has not been deprecated properly - although annoying I think it should print a debugging message (if it stays the same as in this patch). I'll do a bit of testing on this and post some results in a bit.
          Hide
          Sam Hemelryk added a comment -

          Hiya,

          So - answering questions first up:

          Marina -

          1. Why would anyone not want the loader to be persistent. Now that we have two separate options perhaps it is time to make the loader persistent by default. But I would certainly do that as a second issue, this issue doesn't change the default functionality of the cache API it just make it more flexible. As for why, having an option for it I think is beneficial as its a matter of who will take ownership of the cache API. The option being on basically puts ownership of the loader in the hands of the cache API. If you don't use the persistent option then you are able to take full control of the loader yourself.
          2. persistentmaxsize => no. This change sees the persistcache disabled for request caches as the persistentmaxsize option affects the persistcache only it will have no effect. If you want to limit the size you'd need to use the maxsize option.
          3. The session loader operates differently to the application cache in how it handles the data. Before this change the session loader forces the persistent option, after this change it needs to force the persistentloader to true, and the persistentdata to false. I have to amend my patch to reflect this.
          4. It will always use memory - at the moment because of the above I'd bet it is using more than it should be due to object unreferencing. After I amend the patch hopefully we'll see that drop.

          Russel -
          I'm not too sure how this would sit on top of the other caches sorry - do you mean like have an option to use a request cache infront of an application cache for instance? If so that is pretty much what the persistent data option is about, but without the overhead of using MUC.

          Show
          Sam Hemelryk added a comment - Hiya, So - answering questions first up: Marina - Why would anyone not want the loader to be persistent. Now that we have two separate options perhaps it is time to make the loader persistent by default. But I would certainly do that as a second issue, this issue doesn't change the default functionality of the cache API it just make it more flexible. As for why, having an option for it I think is beneficial as its a matter of who will take ownership of the cache API. The option being on basically puts ownership of the loader in the hands of the cache API. If you don't use the persistent option then you are able to take full control of the loader yourself. persistentmaxsize => no. This change sees the persistcache disabled for request caches as the persistentmaxsize option affects the persistcache only it will have no effect. If you want to limit the size you'd need to use the maxsize option. The session loader operates differently to the application cache in how it handles the data. Before this change the session loader forces the persistent option, after this change it needs to force the persistentloader to true, and the persistentdata to false. I have to amend my patch to reflect this. It will always use memory - at the moment because of the above I'd bet it is using more than it should be due to object unreferencing. After I amend the patch hopefully we'll see that drop. Russel - I'm not too sure how this would sit on top of the other caches sorry - do you mean like have an option to use a request cache infront of an application cache for instance? If so that is pretty much what the persistent data option is about, but without the overhead of using MUC.
          Hide
          Marina Glancy added a comment -

          Sam,
          2. I did not know about maxsize. What happens if I try to push into request cache more than maxsize? Will it just loose the data? Another thing is that request cache is very slow and I use static variable instead (as you suggested). All I need from MUC is to store something that is above my limit for static variable, it can be slower but I want it to be stored somewhere (outside of RAM) so all calculated data is not lost in case somebody wants to use it again.

          The use case is any block/report that shows information about multiple courses but max cache size for get_fast_modinfo() is only 10 courses.

          3. I was not talking about "before patch", I was just comparing cache1 and cache2 after your patch (see definitions above). My point is that as a cache user I don't see any difference between those two caches (in terms of time/RAM usage). What I expect from "persistent" option is that cache with persistentdata is faster but uses more RAM, so developer has to choose what is more important for him.

          4. I will have to re-test for application cache. It just logically seems to me that using MUC must be faster than inserting in DB. And why inserting 1000 entries in DB does not result in RAM increase but inserting them in non-persistent MUC gives me out-of-memory?

          Show
          Marina Glancy added a comment - Sam, 2. I did not know about maxsize. What happens if I try to push into request cache more than maxsize? Will it just loose the data? Another thing is that request cache is very slow and I use static variable instead (as you suggested). All I need from MUC is to store something that is above my limit for static variable, it can be slower but I want it to be stored somewhere (outside of RAM) so all calculated data is not lost in case somebody wants to use it again. The use case is any block/report that shows information about multiple courses but max cache size for get_fast_modinfo() is only 10 courses. 3. I was not talking about "before patch", I was just comparing cache1 and cache2 after your patch (see definitions above). My point is that as a cache user I don't see any difference between those two caches (in terms of time/RAM usage). What I expect from "persistent" option is that cache with persistentdata is faster but uses more RAM, so developer has to choose what is more important for him. 4. I will have to re-test for application cache. It just logically seems to me that using MUC must be faster than inserting in DB. And why inserting 1000 entries in DB does not result in RAM increase but inserting them in non-persistent MUC gives me out-of-memory?
          Hide
          Damyon Wiese added a comment -

          Hi Sam:

          Re 1 - I would rather see this done at the same time so are not adding a new option and then immediately taking it away.

          Also looking at the persistcache - it looks like you are not always keeping persistcount in sync - e.g. putting something in the persist cache that already exists, or purging the cache.

          This test case below fails because of this:

          public function test_persistentmaxsize() {                                                                                      
                  $instance = cache_config_phpunittest::instance(true);                                                                       
                  $instance->phpunit_add_definition('phpunit/persistentmaxsize', array(                                                       
                      'mode' => cache_store::MODE_APPLICATION,                                                                                
                      'component' => 'phpunit',                                                                                               
                      'area' => 'persistentmaxsize',                                                                                          
                      'persistentloader' => true,                                                                                             
                      'persistentdata' => true,                                                                                               
                      'persistentmaxsize' => 5                                                                                                
                  ));                                                                                                                         
                                                                                                                                              
                  $cache = cache::make('phpunit', 'persistentmaxsize');                                                                       
                  $cacheentry = 'abcdef';                                                                                                     
                                                                                                                                              
                  foreach (range(0, 5) as $i) {                                                                                               
                      $cache->set('key', $cacheentry);                                                                                        
                  }                                                                                                                           
                  $this->assertEquals($cacheentry, $cache->phpunit_get_directly_from_persistcache('key'));                                    
              }
          

          Finally - I wonder if the code that does this: "Move the item to the end of the array so that it is last to be removed." makes things faster or slower? (Last retrieved != most used and the cost in reordering the array may add up over thousands of hits).

          Show
          Damyon Wiese added a comment - Hi Sam: Re 1 - I would rather see this done at the same time so are not adding a new option and then immediately taking it away. Also looking at the persistcache - it looks like you are not always keeping persistcount in sync - e.g. putting something in the persist cache that already exists, or purging the cache. This test case below fails because of this: public function test_persistentmaxsize() { $instance = cache_config_phpunittest::instance( true ); $instance->phpunit_add_definition('phpunit/persistentmaxsize', array( 'mode' => cache_store::MODE_APPLICATION, 'component' => 'phpunit', 'area' => 'persistentmaxsize', 'persistentloader' => true , 'persistentdata' => true , 'persistentmaxsize' => 5 )); $cache = cache::make('phpunit', 'persistentmaxsize'); $cacheentry = 'abcdef'; foreach (range(0, 5) as $i) { $cache->set('key', $cacheentry); } $ this ->assertEquals($cacheentry, $cache->phpunit_get_directly_from_persistcache('key')); } Finally - I wonder if the code that does this: "Move the item to the end of the array so that it is last to be removed." makes things faster or slower? (Last retrieved != most used and the cost in reordering the array may add up over thousands of hits).
          Hide
          Sam Hemelryk added a comment -

          2. Maxsize is an interesting setting really - it is up to the store as to how it deals with maxsize and not all stores implement it. The session and static stores both implement it and its a basically a fifo queue.
          Why don't all stores implement it, the cache is meant to be volatile, fs space is cheap and systems like memcache auto-manage space used in which case ignoring maxsize in favour of using what is available and having more in the cache is of more benefit. PHP memory of course really needs this to be managed hence we definitely have it there.
          As for memory use - MUC is designed such that you don't know where it is being cached. Of course request and session cache default to memory, we don't interact with any external services (so pretty much anything but RAM) unless we absolutely need to.
          The idea of providing some sort of "size" indication about a cache has been tossed around before, potentially with such a system you could flag a cache as likely to be large and then implement some logic about how stores are chosen based upon that but presently it has only been discussed and not acted upon.
          As for the use cache I'm just not familiar enough with how modinfo is built sorry to suggest anything presently myself.

          3. What I was saying above is that there is a bug in my patch whereby persistentdata is being forced on when in fact it should be the opposite for the session cache. Even then however inserting into the session cache will consume memory - it is just interacting with the global SESSION which lives in memory until being written out when the request is being cleaned up.

          4. Using MUC should certainly be faster than inserting to the dabatase on a medium or larger site. The session cache of course does not interact with the database however, as above it is purely in memory - the session itself is completely separate.

          No doubt I am missing something in translation sorry Marina - once we're through integration this week I will make the amendment I've talked about and then put this through a stress test.

          Show
          Sam Hemelryk added a comment - 2. Maxsize is an interesting setting really - it is up to the store as to how it deals with maxsize and not all stores implement it. The session and static stores both implement it and its a basically a fifo queue. Why don't all stores implement it, the cache is meant to be volatile, fs space is cheap and systems like memcache auto-manage space used in which case ignoring maxsize in favour of using what is available and having more in the cache is of more benefit. PHP memory of course really needs this to be managed hence we definitely have it there. As for memory use - MUC is designed such that you don't know where it is being cached. Of course request and session cache default to memory, we don't interact with any external services (so pretty much anything but RAM) unless we absolutely need to. The idea of providing some sort of "size" indication about a cache has been tossed around before, potentially with such a system you could flag a cache as likely to be large and then implement some logic about how stores are chosen based upon that but presently it has only been discussed and not acted upon. As for the use cache I'm just not familiar enough with how modinfo is built sorry to suggest anything presently myself. 3. What I was saying above is that there is a bug in my patch whereby persistentdata is being forced on when in fact it should be the opposite for the session cache. Even then however inserting into the session cache will consume memory - it is just interacting with the global SESSION which lives in memory until being written out when the request is being cleaned up. 4. Using MUC should certainly be faster than inserting to the dabatase on a medium or larger site. The session cache of course does not interact with the database however, as above it is purely in memory - the session itself is completely separate. No doubt I am missing something in translation sorry Marina - once we're through integration this week I will make the amendment I've talked about and then put this through a stress test.
          Hide
          Sam Hemelryk added a comment -

          Hi Damyon,

          Thanks for looking it over.

          1. I had wondered about the names as well, did anything spring mind for you while reviewing it? I had bit of a mental block with regards to the name and never came up with anything better so just left it as that.

          2. Regarding the persistent loading the option would still stay but the default would change from being off to being on.
          I think there is still value in having an option for it especially in situations where either a custom loader or data source are being used because it would allow for finer grained control if code required it.
          Assuming we agree to keep the option and just change the default I'd then review each definition and change things accordingly. Areas that are setting it on are easy I'd remove the use of the option. For other definitions I'd have to review the use of the definition, if it is self managing (like the question caches do) then I'd set it explicitly off.
          How does that sound?

          3. Ah yes there is another issue about the persistent cache size not being maintained correctly when setting something that is already in there. I had thought it had been integrated already but I see it has not. I'll go hunt that out and make sure it is progressing.

          4. That was an interesting bit of code performance wise. When writing it originally I wrote it as a simple fifo queue, upon more discussion I stress tested both the fifo queue and the current re-ordering. The current approach worked out faster than the fifo queue slightly and thus we went with it, but from memory it was only slightly faster. No doubt the result will change depending upon the site setup as well.
          I have wondered in the past whether we should try test a prioritisation method instead however I've not found time to do so and I'm somewhat sceptical about how its perform so I've not made it a priority.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi Damyon, Thanks for looking it over. 1. I had wondered about the names as well, did anything spring mind for you while reviewing it? I had bit of a mental block with regards to the name and never came up with anything better so just left it as that. 2. Regarding the persistent loading the option would still stay but the default would change from being off to being on. I think there is still value in having an option for it especially in situations where either a custom loader or data source are being used because it would allow for finer grained control if code required it. Assuming we agree to keep the option and just change the default I'd then review each definition and change things accordingly. Areas that are setting it on are easy I'd remove the use of the option. For other definitions I'd have to review the use of the definition, if it is self managing (like the question caches do) then I'd set it explicitly off. How does that sound? 3. Ah yes there is another issue about the persistent cache size not being maintained correctly when setting something that is already in there. I had thought it had been integrated already but I see it has not. I'll go hunt that out and make sure it is progressing. 4. That was an interesting bit of code performance wise. When writing it originally I wrote it as a simple fifo queue, upon more discussion I stress tested both the fifo queue and the current re-ordering. The current approach worked out faster than the fifo queue slightly and thus we went with it, but from memory it was only slightly faster. No doubt the result will change depending upon the site setup as well. I have wondered in the past whether we should try test a prioritisation method instead however I've not found time to do so and I'm somewhat sceptical about how its perform so I've not made it a priority. Cheers Sam
          Hide
          Damyon Wiese added a comment -

          Hi Sam,

          1. Indeed - you have already taken all the obvious names and used them elsewhere Maybe fetchcache? Note - I only think we should change the name if we are changing it anyway do to refactoring - I'm not in favour of changing it just for the sake of it.

          2. Sorry Sam - I still don't see the point in having the option. Even if the question caches are managing their use of the definition - having the cache do it's own caching of the instance will not hurt anything because they all will have a reference to the same instance.

          3. Np - this bug would not hurt anything except the occasional cache miss.

          4. Good to know you've done some before/after testing on that bit of code.

          Cheers
          Damyon

          Show
          Damyon Wiese added a comment - Hi Sam, 1. Indeed - you have already taken all the obvious names and used them elsewhere Maybe fetchcache? Note - I only think we should change the name if we are changing it anyway do to refactoring - I'm not in favour of changing it just for the sake of it. 2. Sorry Sam - I still don't see the point in having the option. Even if the question caches are managing their use of the definition - having the cache do it's own caching of the instance will not hurt anything because they all will have a reference to the same instance. 3. Np - this bug would not hurt anything except the occasional cache miss. 4. Good to know you've done some before/after testing on that bit of code. Cheers Damyon
          Hide
          Marina Glancy added a comment -

          Sam, I created an issue MDL-41106 about slowness of session cache. It is unrelated to this issue.

          BTW writing to application cache is 10 times faster than writing to session cache.

          Show
          Marina Glancy added a comment - Sam, I created an issue MDL-41106 about slowness of session cache. It is unrelated to this issue. BTW writing to application cache is 10 times faster than writing to session cache.
          Hide
          Sam Hemelryk added a comment -

          Putting this up for integration review now.

          I've removed the persistentloader option so that only persistentdata remains.
          At the same time I've introduced a new method for the cache factory to reset its cache instances allowing for these to be clearer.

          Many thanks
          Sam

          Show
          Sam Hemelryk added a comment - Putting this up for integration review now. I've removed the persistentloader option so that only persistentdata remains. At the same time I've introduced a new method for the cache factory to reset its cache instances allowing for these to be clearer. Many thanks Sam
          Hide
          Sam Hemelryk added a comment -

          Added Ankit here as the new cache lead.

          Show
          Sam Hemelryk added a comment - Added Ankit here as the new cache lead.
          Hide
          Sam Hemelryk added a comment -

          Noting for integrators this was a straight cherry-pick to 2.5.
          There were two conflicts on 2.4 one in the readme and the other in lib/db/caches.php (because 2.5/master have more definitions). Both dead easy resolutions.

          Show
          Sam Hemelryk added a comment - Noting for integrators this was a straight cherry-pick to 2.5. There were two conflicts on 2.4 one in the readme and the other in lib/db/caches.php (because 2.5/master have more definitions). Both dead easy resolutions.
          Hide
          Marina Glancy added a comment -

          Thanks a lot Sam, it makes it much easier when all loaders are always persistent.
          Do I understand correctly that option 'persistentdata' only makes sense for application cache? For session and request cache data is always persistent anyway.

          Show
          Marina Glancy added a comment - Thanks a lot Sam, it makes it much easier when all loaders are always persistent. Do I understand correctly that option 'persistentdata' only makes sense for application cache? For session and request cache data is always persistent anyway.
          Hide
          Ankit Agarwal added a comment - - edited

          Added dev_docs_required label, as this change needs to be documented. We also need to document the fact that the loader is always cached now.
          ( Marina)

          Show
          Ankit Agarwal added a comment - - edited Added dev_docs_required label, as this change needs to be documented. We also need to document the fact that the loader is always cached now. ( Marina)
          Hide
          Marina Glancy added a comment -

          yay, we catch it

          Show
          Marina Glancy added a comment - yay, we catch it
          Hide
          Marina Glancy added a comment -

          Hi Sam, I'm really glad this is getting out.

          There are some mistakes in phpdocs, I suppose from shuffling code back and forth. For example:

          • in 2.4 and 2.5 branch phpdocs say "deprecated since 2.6". At the same time, should not there be a debugging message in 2.6 in this function?
          • phpdocs for function data_should_be_persistent() says: "Returns true if the cache loader for this definition should be persistent."

          Other things:

          • cache_session::is_using_persist_cache() returns false. It is not changed in this issue. Is it ok?
          • two functions are added to class cache_phpunit_application and they are never used. This all looks like it needs couple more unittests - to check that both 'persistentdata' and old 'persistent' options work.
          • in core/coursecat cache the option persistentdata is not needed because it is a session cache

          I want to try to add some more unittests now, will post results here

          Show
          Marina Glancy added a comment - Hi Sam, I'm really glad this is getting out. There are some mistakes in phpdocs, I suppose from shuffling code back and forth. For example: in 2.4 and 2.5 branch phpdocs say "deprecated since 2.6". At the same time, should not there be a debugging message in 2.6 in this function? phpdocs for function data_should_be_persistent() says: "Returns true if the cache loader for this definition should be persistent." Other things: cache_session::is_using_persist_cache() returns false. It is not changed in this issue. Is it ok? two functions are added to class cache_phpunit_application and they are never used. This all looks like it needs couple more unittests - to check that both 'persistentdata' and old 'persistent' options work. in core/coursecat cache the option persistentdata is not needed because it is a session cache I want to try to add some more unittests now, will post results here
          Hide
          Sam Hemelryk added a comment -

          Thanks for picking those up Marina. I've pushed up branches now which have the following fixes made:

          • Fixed deprecation version on 24 and 25 branch.
          • Fixed phpdoc comment for data_should_be_persistent.
          • cache_session::is_using_persist_cache returning false is correct.
          • I've removed the two superfluous functions from cache_phpunit_application now.
          • Removed persistentdata from core/coursecat.

          How'd you get along with those unit tests?

          Many thanks
          Sam

          Show
          Sam Hemelryk added a comment - Thanks for picking those up Marina. I've pushed up branches now which have the following fixes made: Fixed deprecation version on 24 and 25 branch. Fixed phpdoc comment for data_should_be_persistent. cache_session::is_using_persist_cache returning false is correct. I've removed the two superfluous functions from cache_phpunit_application now. Removed persistentdata from core/coursecat. How'd you get along with those unit tests? Many thanks Sam
          Hide
          Marina Glancy added a comment -

          Hi Sam, finally I wrote the test I had in mind. Unfortunately some of them are falling (I commented out the statements that I expect to pass but which fail)
          https://github.com/marinaglancy/moodle/commit/67a15c65e4dc4c832d6997ca683b914bdde84047

          I started fixing:
          https://github.com/marinaglancy/moodle/commit/32bdb342baf995e61d7619b51b40c20b5f59f2ab

          but still there is one thing remaining: get_many() does not move item into persistent cache

          Show
          Marina Glancy added a comment - Hi Sam, finally I wrote the test I had in mind. Unfortunately some of them are falling (I commented out the statements that I expect to pass but which fail) https://github.com/marinaglancy/moodle/commit/67a15c65e4dc4c832d6997ca683b914bdde84047 I started fixing: https://github.com/marinaglancy/moodle/commit/32bdb342baf995e61d7619b51b40c20b5f59f2ab but still there is one thing remaining: get_many() does not move item into persistent cache
          Hide
          Marina Glancy added a comment -

          I squashed my commits from above plus added Sam's fix to get_many() and little changes to unittest (shuffling the assert lines to access the data in the correct order)

          So final branches with all changes:

          https://github.com/marinaglancy/moodle/compare/x-int-m24...wip-MDL-40903-m24
          https://github.com/marinaglancy/moodle/compare/x-int-m25...wip-MDL-40903-m25
          https://github.com/marinaglancy/moodle/compare/x-int-master...wip-MDL-40903-master2

          git pull git://github.com/marinaglancy/moodle.git wip-MDL-40903-m24
          git pull git://github.com/marinaglancy/moodle.git wip-MDL-40903-m25
          git pull git://github.com/marinaglancy/moodle.git wip-MDL-40903-master2
          

          I've been looking at this issue too long, can somebody else please have a fresh look and integrate it?

          Show
          Marina Glancy added a comment - I squashed my commits from above plus added Sam's fix to get_many() and little changes to unittest (shuffling the assert lines to access the data in the correct order) So final branches with all changes: https://github.com/marinaglancy/moodle/compare/x-int-m24...wip-MDL-40903-m24 https://github.com/marinaglancy/moodle/compare/x-int-m25...wip-MDL-40903-m25 https://github.com/marinaglancy/moodle/compare/x-int-master...wip-MDL-40903-master2 git pull git: //github.com/marinaglancy/moodle.git wip-MDL-40903-m24 git pull git: //github.com/marinaglancy/moodle.git wip-MDL-40903-m25 git pull git: //github.com/marinaglancy/moodle.git wip-MDL-40903-master2 I've been looking at this issue too long, can somebody else please have a fresh look and integrate it?
          Hide
          Sam Hemelryk added a comment -

          Thanks Marina - I've cherry-picked in your commits now and this is once more ready for integration

          Show
          Sam Hemelryk added a comment - Thanks Marina - I've cherry-picked in your commits now and this is once more ready for integration
          Hide
          Eloy Lafuente (stronk7) added a comment - - edited

          Hey, nice stuff. After some chatting with SamH, there there are a bunch of details that we could consider before allowing this to land:

          1) The names. Calling it "persistent cache" or "persistent data" or "should_be_persistent()" is 100% misleading. In fact, in out conversations we end mixing continuously real MUC persistent caches and that static accelerator built within them. So I'd start naming it "static accelerator" and change the setting, the docs and the method to that name. And this is the perfect time to do so. No more "persistent" terminology for them will help everybody, I think.

          2) The branches. Do we really need this going back to 24_STABLE ?

          3) Typo: "Memeory" in README.md

          4) PHPDocs: "Please call data_should_be_persistent instead." should use an inline

          {@link}

          tag. And potentially some debug warn?

          5) With the new assumptions (defined in data_should_be_persistent()):

          • request caches don't have ever static accelerators (hope nobody is using a non-static request cache they will get penalized losing the ability to use static accelerators for it).
          • session caches have always static accelerators (not sure why but it's clear for Sam, so I trust him).
          • application caches decide if they have static accelerators.

          It seems that some logic when invalidating by event can be cleaned.

          6) Hope there is no place where the setting in the definition takes precedence over data_should_be_persistent() and every place is using always the later to decide. Setting manually the option should not happen ever. And I've seen various hand assignments here and there ($this->persistdata = xxxx). Cannot find a reason for them.

          7) Also, if the cache instances are now always persistent for the whole request… then, the "persistent" option in make_from_params() and create_cache_from_params() seem no sense. Not sure.

          Note this review has been done against the 24_STABLE branch, but surely applies to all them.

          Also note that, for some reason, the github diffs above do not show all the changes. Please fix that on next submission.

          Agreed with Sam, reopening this for one more round, thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - - edited Hey, nice stuff. After some chatting with SamH, there there are a bunch of details that we could consider before allowing this to land: 1) The names. Calling it "persistent cache" or "persistent data" or "should_be_persistent()" is 100% misleading. In fact, in out conversations we end mixing continuously real MUC persistent caches and that static accelerator built within them. So I'd start naming it "static accelerator" and change the setting, the docs and the method to that name. And this is the perfect time to do so. No more "persistent" terminology for them will help everybody, I think. 2) The branches. Do we really need this going back to 24_STABLE ? 3) Typo: "Memeory" in README.md 4) PHPDocs: "Please call data_should_be_persistent instead." should use an inline {@link} tag. And potentially some debug warn? 5) With the new assumptions (defined in data_should_be_persistent()): request caches don't have ever static accelerators (hope nobody is using a non-static request cache they will get penalized losing the ability to use static accelerators for it). session caches have always static accelerators (not sure why but it's clear for Sam, so I trust him). application caches decide if they have static accelerators. It seems that some logic when invalidating by event can be cleaned. 6) Hope there is no place where the setting in the definition takes precedence over data_should_be_persistent() and every place is using always the later to decide. Setting manually the option should not happen ever. And I've seen various hand assignments here and there ($this->persistdata = xxxx). Cannot find a reason for them. 7) Also, if the cache instances are now always persistent for the whole request… then, the "persistent" option in make_from_params() and create_cache_from_params() seem no sense. Not sure. Note this review has been done against the 24_STABLE branch, but surely applies to all them. Also note that, for some reason, the github diffs above do not show all the changes. Please fix that on next submission. Agreed with Sam, reopening this for one more round, thanks!
          Hide
          CiBoT added a comment -

          Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

          Show
          CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
          Hide
          Sam Hemelryk added a comment -

          Discussed this with Eloy this morning.

          I've pushed up modified branches now.
          All issues have been addressed, there are two new commits on 24 and 25 and three new commits on 26.
          The 26 branch renames functions that cannot be renamed on stable branches (hence the additional commit).

          Up for peer-review again.

          Show
          Sam Hemelryk added a comment - Discussed this with Eloy this morning. I've pushed up modified branches now. All issues have been addressed, there are two new commits on 24 and 25 and three new commits on 26. The 26 branch renames functions that cannot be renamed on stable branches (hence the additional commit). Up for peer-review again.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Note that this is likely to conflict with MDL-41437 (already integrated), I bet.

          Show
          Eloy Lafuente (stronk7) added a comment - Note that this is likely to conflict with MDL-41437 (already integrated), I bet.
          Hide
          Sam Hemelryk added a comment -

          No probs I've rebased 40903-26 on top of integration now and have resolved the conflicts.

          Show
          Sam Hemelryk added a comment - No probs I've rebased 40903-26 on top of integration now and have resolved the conflicts.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          This gets my +1. Just waiting Marina in case she has anything against...

          Show
          Eloy Lafuente (stronk7) added a comment - This gets my +1. Just waiting Marina in case she has anything against...
          Hide
          Marina Glancy added a comment -

          looks good to me. Thanks Sam

          Show
          Marina Glancy added a comment - looks good to me. Thanks Sam
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Integrated (24, 25 and master), thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - Integrated (24, 25 and master), thanks!
          Hide
          Mark Nelson added a comment -

          Works as expected, thanks Sam.

          Show
          Mark Nelson added a comment - Works as expected, thanks Sam.
          Hide
          Marina Glancy added a comment -

          And THANK YOU again for making Moodle better every day!

          Another weekly release has been released.

          Show
          Marina Glancy added a comment - And THANK YOU again for making Moodle better every day! Another weekly release has been released.

            People

            • Votes:
              0 Vote for this issue
              Watchers:
              9 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: