Sam Hemelryk added a comment - 11/Apr/13 12:45 AM Mega epic blocker - thanks for the report Damyon.

Sam Hemelryk added a comment - 17/Apr/13 3:52 AM Hi guys, I believe I've got a solution for this now. Re-designed the session loader pretty much but it all makes more sense now I think. Patch is rather large Eloy or Damyon, if pos could you take a look? Many thanks Sam

Sam Hemelryk added a comment - 18/Apr/13 12:14 AM I've added a 24 branch as well. Simple cherry-pick with two minor collisions, cache/tests/cache_test.php and cache/README.php. Both due to new features in 2.5.

Damyon Wiese added a comment - 18/Apr/13 4:03 AM - edited Hi Sam, Thanks for the patch (lots of reading). Your performance tests show that this new code is faster which is great. Minor things: 1. double ;; in cache/classes/definition.php 2. In session_cache::delete and session_cache::delete_many there is a slight difference to parent class (!empty($this->get_loader()) verses $this->get_loader() !== false (I prefer !empty) 3. (mentioned on chat) MUST_EXUST in cache_phpunit_tests (kiwi accent typo) 4. whitespace in lib/db/upgrade.php Improvement suggestions: would prefer the function call before the foreach here (cache/classes/factory.php): + // Initialise all of the stores used for that definition. + foreach (self::initialise_cachestore_instances($stores, $definition) as $store) { session_cache has a few functions (get and delete in particular) that are 95% same as the parent class - it would be nice to remove that duplication if possible (and reasonable). Other things: (Discussed on chat about adding sessionid to the keys for the session cache and modifying check_tracked_user to only purge the cache of keys matching the session id). Note: I tried modifying a unit test to detect this but couldn't because it requires separate sessions.

Sam Hemelryk added a comment - 18/Apr/13 8:00 AM Thanks for looking this over Damyon, I've made the changes noted and am pushing this up for integration now. For the integrator - performance comparisons can be found here: Comparison of master vs this patch applied with default settings: http://moodev.com/?w=300&h=150&before=master-2013-04-18.01&after=MDL-38565-2013-04-18.01&x=1&o=filesincluded Comparison of master vs this patch applied and a file store being used to store session data: http://moodev.com/?w=300&h=150&before=master-2013-04-18.01&after=MDL-38565-alt-2013-04-18.01&o=filesincluded Many thanks Sam

Dan Poltawski added a comment - 18/Apr/13 12:47 PM The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao

Dan Poltawski added a comment - 19/Apr/13 12:53 PM Integrated to master and 24, thanks Sam

Adrian Greeve added a comment - 22/Apr/13 3:49 AM Tested on 2.4 and master integration. I ran the unit tests multiple times, upgraded the sites, created new installs, enabled all of the front page settings, switched between different users, and ran performance tests. No problems found. Test passed.

Eloy Lafuente (stronk7) added a comment - 25/Apr/13 12:08 AM I feel myself really alone tonight! So was time to push your fixes upstream! "Lest we forget. We will remember them." Thanks and ciao!

Matteo Scaramuccia added a comment - 25/Apr/13 7:29 PM Hi Sam, great work and nice improvement! I got in touch with it thanks to the Integration round 2013-04-25 post: I'll take the time to learn it. BTW, there 8 typos (the same pattern, 8 times) in PHPDoc, do you think they should be "fixed"? I'm not native English i.e. I'm not sure if they'll "hurt" someone...: diff --git a/cache/classes/interfaces.php b/cache/classes/interfaces.php index 24996ed..b950260 100644 --- a/cache/classes/interfaces.php +++ b/cache/classes/interfaces.php @@ -108,7 +108,7 @@ interface cache_loader { public function set_many(array $keyvaluearray); /** - * Test is a cache has a key. + * Test if a cache has a key. * * The use of the has methods is strongly discouraged. In a high load environment the cache may well change between the * test and any subsequent action (get, set, delete etc). @@ -144,7 +144,7 @@ interface cache_loader { public function has_any(array $keys); /** - * Test is a cache has all of the given keys. + * Test if a cache has all of the given keys. * * It is strongly recommended to avoid the use of this function if not absolutely required. * In a high load environment the cache may well change between the test and any subsequent action (get, set, delete etc). @@ -287,7 +287,7 @@ interface cache_is_lockable { interface cache_is_key_aware { /** - * Test is a cache has a key. + * Test if a cache has a key. * * The use of the has methods is strongly discouraged. In a high load environment the cache may well change between the * test and any subsequent action (get, set, delete etc). @@ -323,7 +323,7 @@ interface cache_is_key_aware { public function has_any(array $keys); /** - * Test is a cache has all of the given keys. + * Test if a cache has all of the given keys. * * It is strongly recommended to avoid the use of this function if not absolutely required. * In a high load environment the cache may well change between the test and any subsequent action (get, set, delete etc). diff --git a/cache/classes/loaders.php b/cache/classes/loaders.php index 9f7dc79..5298ae3 100644 --- a/cache/classes/loaders.php +++ b/cache/classes/loaders.php @@ -622,7 +622,7 @@ class cache implements cache_loader { } /** - * Test is a cache has a key. + * Test if a cache has a key. * * The use of the has methods is strongly discouraged. In a high load environment the cache may well change between the * test and any subsequent action (get, set, delete etc). @@ -678,7 +678,7 @@ class cache implements cache_loader { } /** - * Test is a cache has all of the given keys. + * Test if a cache has all of the given keys. * * It is strongly recommended to avoid the use of this function if not absolutely required. * In a high load environment the cache may well change between the test and any subsequent action (get, set, delete etc). @@ -1877,7 +1877,7 @@ class cache_session extends cache { } /** - * Test is a cache has a key. + * Test if a cache has a key. * * The use of the has methods is strongly discouraged. In a high load environment the cache may well change between the * test and any subsequent action (get, set, delete etc). @@ -1927,7 +1927,7 @@ class cache_session extends cache { } /** - * Test is a cache has all of the given keys. + * Test if a cache has all of the given keys. * * It is strongly recommended to avoid the use of this function if not absolutely required. * In a high load environment the cache may well change between the test and any subsequent action (get, set, delete etc).