From c98621977cc93af1433ea796a8f8c3a13fdd1b2d Mon Sep 17 00:00:00 2001 From: Hamish Friedlander Date: Tue, 26 Feb 2013 16:39:40 +1300 Subject: [PATCH] Cache the merged version of any Config value in an in-mem LRU cache --- core/Config.php | 175 +++++++++++++++++++++++++++++++------- core/Object.php | 13 +-- tests/core/ConfigTest.php | 52 ++++++++++- 3 files changed, 198 insertions(+), 42 deletions(-) diff --git a/core/Config.php b/core/Config.php index c6b08daac..5a945fb5a 100644 --- a/core/Config.php +++ b/core/Config.php @@ -163,11 +163,14 @@ class Config { $_SINGLETONS['Config'] = $instance; } + protected $cache; + /** * Empty construction, otherwise calling singleton('Config') (not the right way to get the current active config * instance, but people might) gives an error */ public function __construct() { + $this->cache = new Config_LRU(); } /** @var [array] - Array of arrays. Each member is an nested array keyed as $class => $name => $value, @@ -189,6 +192,7 @@ class Config { */ public function pushConfigManifest(SS_ConfigManifest $manifest) { array_unshift($this->manifests, $manifest->yamlConfig); + $this->cache->clean(); // @todo: Do anything with these. They're for caching after config.php has executed $this->collectConfigPHPSettings = true; @@ -342,34 +346,17 @@ class Config { return $res; } - /** - * Get the config value associated for a given class and property - * - * This merges all current sources and overrides together to give final value - * todo: Currently this is done every time. This function is an inner loop function, so we really need to be - * caching heavily here. - * - * @param $class string - The name of the class to get the value for - * @param $name string - The property to get the value for - * @param int $sourceOptions Bitmask which can be set to some combintain of Config::UNINHERITED, - * Config::FIRST_SET, and Config::EXCLUDE_EXTENSIONS. - * - * Config::UNINHERITED does not include parent classes when merging configuration fragments - * Config::FIRST_SET stops inheriting once the first class that sets a value (even an empty value) is encoutered - * Config::EXCLUDE_EXTRA_SOURCES does not include any additional static sources (such as extensions) - * - * Config::INHERITED is a utility constant that can be used to mean "none of the above", equvilient to 0 - * Setting both Config::UNINHERITED and Config::FIRST_SET behaves the same as just Config::UNINHERITED - * - * should the parent classes value be merged in as the lowest priority source? - * @param $result array|scalar Reference to a variable to put the result in. Also returned, so this can be left - * as null safely. If you do pass a value, it will be treated as the highest priority - * value in the result chain - * @param $suppress array Internal use when called by child classes. Array of mask pairs to filter value by - * @return array|scalar The value of the config item, or null if no value set. Could be an associative array, - * sequential array or scalar depending on value (see class docblock) - */ - public function get($class, $name, $sourceOptions = 0, &$result = null, $suppress = null) { + protected $extraConfigSources = array(); + + public function extraConfigSourcesChanged($class) { + unset($this->extraConfigSources[$class]); + $this->cache->clean("__{$class}"); + } + + protected function getUncached($class, $name, $sourceOptions, &$result, $suppress, &$tags) { + $tags[] = "__{$class}"; + $tags[] = "__{$class}__{$name}"; + // If result is already not something to merge into, just return it if ($result !== null && !is_array($result)) return $result; @@ -401,9 +388,16 @@ class Config { $nothing = new stdClass(); $sources = array($class); + // Include extensions only if not flagged not to, and some have been set if (($sourceOptions & self::EXCLUDE_EXTRA_SOURCES) != self::EXCLUDE_EXTRA_SOURCES) { - $extraSources = Object::get_extra_config_sources($class); + // If we don't have a fresh list of extra sources, get it from the class itself + if (!array_key_exists($class, $this->extraConfigSources)) { + $this->extraConfigSources[$class] = Object::get_extra_config_sources($class); + } + + // Update $sources with any extra sources + $extraSources = $this->extraConfigSources[$class]; if ($extraSources) $sources = array_merge($sources, $extraSources); } @@ -421,11 +415,48 @@ class Config { if (($sourceOptions & self::UNINHERITED) != self::UNINHERITED && (($sourceOptions & self::FIRST_SET) != self::FIRST_SET || $result === null)) { $parent = get_parent_class($class); - if ($parent) $this->get($parent, $name, $sourceOptions, $result, $suppress); + if ($parent) $this->getUncached($parent, $name, $sourceOptions, $result, $suppress, $tags); } - if ($name == 'routes') { - print_r($result); die; + return $result; + } + + /** + * Get the config value associated for a given class and property + * + * This merges all current sources and overrides together to give final value + * todo: Currently this is done every time. This function is an inner loop function, so we really need to be + * caching heavily here. + * + * @param $class string - The name of the class to get the value for + * @param $name string - The property to get the value for + * @param int $sourceOptions Bitmask which can be set to some combintain of Config::UNINHERITED, + * Config::FIRST_SET, and Config::EXCLUDE_EXTENSIONS. + * + * Config::UNINHERITED does not include parent classes when merging configuration fragments + * Config::FIRST_SET stops inheriting once the first class that sets a value (even an empty value) is encoutered + * Config::EXCLUDE_EXTRA_SOURCES does not include any additional static sources (such as extensions) + * + * Config::INHERITED is a utility constant that can be used to mean "none of the above", equvilient to 0 + * Setting both Config::UNINHERITED and Config::FIRST_SET behaves the same as just Config::UNINHERITED + * + * should the parent classes value be merged in as the lowest priority source? + * @param $result array|scalar Reference to a variable to put the result in. Also returned, so this can be left + * as null safely. If you do pass a value, it will be treated as the highest priority + * value in the result chain + * @param $suppress array Internal use when called by child classes. Array of mask pairs to filter value by + * @return array|scalar The value of the config item, or null if no value set. Could be an associative array, + * sequential array or scalar depending on value (see class docblock) + */ + public function get($class, $name, $sourceOptions = 0, &$result = null, $suppress = null) { + // Have we got a cached value? Use it if so + $key = sha1($class.$name.$sourceOptions); + + if (($result = $this->cache->get($key)) === false) { + $tags = array(); + $result = null; + $this->getUncached($class, $name, $sourceOptions, $result, $suppress, $tags); + $this->cache->set($key, $result, $tags); } return $result; @@ -452,6 +483,8 @@ class Config { if (!isset($this->overrides[0][$class][$name])) $this->overrides[0][$class][$name] = $val; else self::merge_high_into_low($this->overrides[0][$class][$name], $val); + + $this->cache->clean("__{$class}__{$name}"); } /** @@ -512,6 +545,84 @@ class Config { } +class Config_LRU { + const SIZE = 1000; + + protected $cache; + protected $indexing; + + protected $i = 0; + protected $c = 0; + + public function __construct() { + $this->cache = new SplFixedArray(self::SIZE); + $this->indexing = array(); + } + + public function set($key, $val, $tags = array()) { + // Find an index to set at + $replacing = null; + + // Target count - not always the lowest, but guaranteed to exist (or hit an empty item) + $target = $this->c - self::SIZE + 1; + $i = $stop = $this->i; + + do { + if (!($i--)) $i = self::SIZE-1; + $item = $this->cache[$i]; + + if (!$item) { $replacing = null; break; } + else if ($item->c <= $target) { $replacing = $item; break; } + } + while ($i != $stop); + + if ($replacing) unset($this->indexing[$replacing->key]); + + $this->indexing[$key] = $this->i = $i; + $this->cache[$i] = $obj = new stdClass(); + + $obj->key = $key; + $obj->value = $val; + $obj->tags = $tags; + $obj->c = ++$this->c; + } + + private $hit = 0; + private $miss = 0; + + public function stats() { + return $this->miss ? ($this->hit / $this->miss) : 0; + } + + public function get($key) { + if (isset($this->indexing[$key])) { + $this->hit++; + + $res = $this->cache[$this->indexing[$key]]; + $res->c = ++$this->c; + return $res->value; + } + + $this->miss++; + return false; + } + + public function clean($tag = null) { + if ($tag) { + foreach ($this->cache as $i => $v) { + if ($v && in_array($tag, $v->tags)) { + unset($this->indexing[$v->key]); + $this->cache[$i] = null; + } + } + } + else { + $this->cache = new SplFixedArray(self::SIZE); + $this->indexing = array(); + } + } +} + class Config_ForClass { protected $class; diff --git a/core/Object.php b/core/Object.php index f313eb9c1..a97152288 100755 --- a/core/Object.php +++ b/core/Object.php @@ -483,10 +483,11 @@ abstract class Object { if($subclasses) foreach($subclasses as $subclass) { unset(self::$classes_constructed[$subclass]); unset(self::$extra_methods[$subclass]); - unset(self::$extension_sources[$subclass]); } Config::inst()->update($class, 'extensions', array($extension)); + Config::inst()->extraConfigSourcesChanged($class); + Injector::inst()->unregisterAllObjects(); // load statics now for DataObject classes @@ -523,6 +524,7 @@ abstract class Object { } Config::inst()->remove($class, 'extensions', Config::anything(), $extension); + Config::inst()->extraConfigSourcesChanged($class); // unset singletons to avoid side-effects Injector::inst()->unregisterAllObjects(); @@ -533,7 +535,6 @@ abstract class Object { if($subclasses) foreach($subclasses as $subclass) { unset(self::$classes_constructed[$subclass]); unset(self::$extra_methods[$subclass]); - unset(self::$extension_sources[$subclass]); } } @@ -560,9 +561,6 @@ abstract class Object { // -------------------------------------------------------------------------------------------------------------- - private static $extension_sources = array(); - - // Don't bother checking some classes that should never be extended private static $unextendable_classes = array('Object', 'ViewableData', 'RequestHandler'); static public function get_extra_config_sources($class = null) { @@ -571,9 +569,6 @@ abstract class Object { // If this class is unextendable, NOP if(in_array($class, self::$unextendable_classes)) return; - // If we have a pre-cached version, use that - if(array_key_exists($class, self::$extension_sources)) return self::$extension_sources[$class]; - // Variable to hold sources in $sources = null; @@ -604,7 +599,7 @@ abstract class Object { } } - return self::$extension_sources[$class] = $sources; + return $sources; } public function __construct() { diff --git a/tests/core/ConfigTest.php b/tests/core/ConfigTest.php index 2598bd404..7ed19ed9e 100644 --- a/tests/core/ConfigTest.php +++ b/tests/core/ConfigTest.php @@ -163,5 +163,55 @@ class ConfigTest extends SapphireTest { public function testFragmentOrder() { $this->markTestIncomplete(); } - + + public function testLRUDiscarding() { + $cache = new ConfigTest_Config_LRU(); + + for ($i = 0; $i < Config_LRU::SIZE*2; $i++) $cache->set($i, $i); + $this->assertEquals( + Config_LRU::SIZE, count($cache->indexing), + 'Homogenous usage gives exact discarding' + ); + + $cache = new ConfigTest_Config_LRU(); + + for ($i = 0; $i < Config_LRU::SIZE; $i++) $cache->set($i, $i); + for ($i = 0; $i < Config_LRU::SIZE; $i++) $cache->set(-1, -1); + $this->assertLessThan( + Config_LRU::SIZE, count($cache->indexing), + 'Heterogenous usage gives sufficient discarding' + ); + } + + public function testLRUCleaning() { + $cache = new ConfigTest_Config_LRU(); + + for ($i = 0; $i < Config_LRU::SIZE; $i++) $cache->set($i, $i); + $this->assertEquals(Config_LRU::SIZE, count($cache->indexing)); + + $cache->clean(); + $this->assertEquals(0, count($cache->indexing), 'Clean clears all items'); + $this->assertFalse($cache->get(1), 'Clean clears all items'); + + $cache->set(1, 1, array('Foo')); + $this->assertEquals(1, count($cache->indexing)); + + $cache->clean('Foo'); + $this->assertEquals(0, count($cache->indexing), 'Clean items with matching tag'); + $this->assertFalse($cache->get(1), 'Clean items with matching tag'); + + $cache->set(1, 1, array('Foo', 'Bar')); + $this->assertEquals(1, count($cache->indexing)); + + $cache->clean('Bar'); + $this->assertEquals(0, count($cache->indexing), 'Clean items with any single matching tag'); + $this->assertFalse($cache->get(1), 'Clean items with any single matching tag'); + } +} + +class ConfigTest_Config_LRU extends Config_LRU { + + public $cache; + public $indexing; + }