From 610c602b0644e95acc237e92104b54e8d687236f Mon Sep 17 00:00:00 2001 From: Bernhard Schussek Date: Mon, 16 Jul 2012 21:36:01 +0200 Subject: [PATCH] [OptionsResolver] Slightly tweaked the performance of the Options class --- .../Component/OptionsResolver/LazyOption.php | 22 +++-- .../Component/OptionsResolver/Options.php | 98 ++++++++----------- .../OptionsResolver/Tests/LazyOptionTest.php | 44 +++++++++ 3 files changed, 99 insertions(+), 65 deletions(-) create mode 100644 src/Symfony/Component/OptionsResolver/Tests/LazyOptionTest.php diff --git a/src/Symfony/Component/OptionsResolver/LazyOption.php b/src/Symfony/Component/OptionsResolver/LazyOption.php index dd52c5a70b..fbe700a6b0 100644 --- a/src/Symfony/Component/OptionsResolver/LazyOption.php +++ b/src/Symfony/Component/OptionsResolver/LazyOption.php @@ -33,22 +33,22 @@ class LazyOption /** * Creates a new lazy option. * - * @param Closure $closure The closure used for initializing the - * option value. + * @param \Closure $closure The closure used for initializing the + * option value. * @param mixed $previousValue The previous value of the option. This - * value is passed to the closure when it is - * evaluated. + * value is passed to the closure when it is + * evaluated. * * @see evaluate() */ - public function __construct(\Closure $closure, $previousValue) + public function __construct(\Closure $closure, $previousValue = null) { $this->closure = $closure; $this->previousValue = $previousValue; } /** - * Evaluates the underyling closure and returns its result. + * Evaluates the underlying closure and returns its result. * * The given Options instance is passed to the closure as first argument. * The previous default value set in the constructor is passed as second @@ -60,10 +60,14 @@ class LazyOption */ public function evaluate(Options $options) { - if ($this->previousValue instanceof self) { - $this->previousValue = $this->previousValue->evaluate($options); + $previousValue = $this->previousValue; + $closure = $this->closure; + + if ($previousValue instanceof self) { + $previousValue = $this->previousValue->evaluate($options); } - return $this->closure->__invoke($options, $this->previousValue); + // Performs a bit better than __invoke() and call_user_func() + return $closure($options, $previousValue); } } diff --git a/src/Symfony/Component/OptionsResolver/Options.php b/src/Symfony/Component/OptionsResolver/Options.php index 88fa375f72..575faa087b 100644 --- a/src/Symfony/Component/OptionsResolver/Options.php +++ b/src/Symfony/Component/OptionsResolver/Options.php @@ -139,23 +139,32 @@ class Options implements \ArrayAccess, \Iterator, \Countable throw new OptionDefinitionException('Options cannot be overloaded anymore once options have been read.'); } + // If an option is a closure that should be evaluated lazily, store it + // inside a LazyOption instance. + if ($value instanceof \Closure) { + $reflClosure = new \ReflectionFunction($value); + $params = $reflClosure->getParameters(); + + if (isset($params[0]) && null !== ($class = $params[0]->getClass()) && __CLASS__ === $class->name) { + $currentValue = isset($this->options[$option]) ? $this->options[$option] : null; + $value = new LazyOption($value, $currentValue); + + // Store locks for lazy options to detect cyclic dependencies + $this->lock[$option] = false; + + // Store which options are lazy for more efficient resolving + $this->lazy[$option] = true; + + $this->options[$option] = $value; + + return; + } + } + // Reset lazy flag and locks by default unset($this->lock[$option]); unset($this->lazy[$option]); - // If an option is a closure that should be evaluated lazily, store it - // inside a LazyOption instance. - if (self::isEvaluatedLazily($value)) { - $currentValue = isset($this->options[$option]) ? $this->options[$option] : null; - $value = new LazyOption($value, $currentValue); - - // Store locks for lazy options to detect cyclic dependencies - $this->lock[$option] = false; - - // Store which options are lazy for more efficient resolving - $this->lazy[$option] = true; - } - $this->options[$option] = $value; } @@ -251,8 +260,14 @@ class Options implements \ArrayAccess, \Iterator, \Countable // Create a copy because resolve() modifies the array $lazy = $this->lazy; + // Performance-wise this is slightly better than + // while (null !== $option = key($this->lazy)) foreach ($lazy as $option => $isLazy) { - $this->resolve($option); + // When resolve() is called, potentially multiple lazy options + // are evaluated, so check again if the option is still lazy. + if (isset($this->lazy[$option])) { + $this->resolve($option); + } } return $this->options; @@ -329,7 +344,7 @@ class Options implements \ArrayAccess, \Iterator, \Countable */ public function current() { - return $this->offsetGet($this->key()); + return $this->get($this->key()); } /** @@ -385,52 +400,23 @@ class Options implements \ArrayAccess, \Iterator, \Countable */ private function resolve($option) { - if ($this->options[$option] instanceof LazyOption) { - if ($this->lock[$option]) { - $conflicts = array_keys(array_filter($this->lock, function ($locked) { - return $locked; - })); + if ($this->lock[$option]) { + $conflicts = array(); - throw new OptionDefinitionException('The options "' . implode('", "', - $conflicts) . '" have a cyclic dependency.'); + foreach ($this->lock as $option => $locked) { + if ($locked) { + $conflicts[] = $option; + } } - $this->lock[$option] = true; - $this->options[$option] = $this->options[$option]->evaluate($this); - $this->lock[$option] = false; - - // The option now isn't lazy anymore - unset($this->lazy[$option]); - } - } - - /** - * Returns whether the option is a lazy option closure. - * - * Lazy option closure expect an {@link Options} instance - * in their first parameter. - * - * @param mixed $value The option value to test. - * - * @return Boolean Whether it is a lazy option closure. - */ - private static function isEvaluatedLazily($value) - { - if (!$value instanceof \Closure) { - return false; + throw new OptionDefinitionException('The options "' . implode('", "', $conflicts) . '" have a cyclic dependency.'); } - $reflClosure = new \ReflectionFunction($value); - $params = $reflClosure->getParameters(); + $this->lock[$option] = true; + $this->options[$option] = $this->options[$option]->evaluate($this); + $this->lock[$option] = false; - if (count($params) < 1) { - return false; - } - - if (null === $params[0]->getClass()) { - return false; - } - - return __CLASS__ === $params[0]->getClass()->name; + // The option now isn't lazy anymore + unset($this->lazy[$option]); } } diff --git a/src/Symfony/Component/OptionsResolver/Tests/LazyOptionTest.php b/src/Symfony/Component/OptionsResolver/Tests/LazyOptionTest.php new file mode 100644 index 0000000000..48b15cec80 --- /dev/null +++ b/src/Symfony/Component/OptionsResolver/Tests/LazyOptionTest.php @@ -0,0 +1,44 @@ + + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +namespace Symfony\Component\OptionsResolver\Tests; + +use Symfony\Component\OptionsResolver\LazyOption; +use Symfony\Component\OptionsResolver\Options; + +/** + * @author Bernhard Schussek + */ +class LazyOptionTest extends \PHPUnit_Framework_TestCase +{ + public function testDontCacheEvaluatedPreviousValue() + { + $previousValue = new LazyOption(function (Options $options) { + return $options['foo']; + }); + + $lazyOption = new LazyOption(function (Options $options, $previousValue) { + return $previousValue; + }, $previousValue); + + // If provided with two different option sets, two different results + // should be returned + $options1 = new Options(); + $options1['foo'] = 'bar'; + + $this->assertSame('bar', $lazyOption->evaluate($options1)); + + $options2 = new Options(); + $options2['foo'] = 'boo'; + + $this->assertSame('boo', $lazyOption->evaluate($options2)); + } +}