Skip to content

[OptionsResolver] refactoring/optimization #5073

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 4 commits into from
Closed

[OptionsResolver] refactoring/optimization #5073

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
72269e0
[OptionsResolver] removed unneeded private variable in Options
Tobion Jul 27, 2012
cc4f15a
[OptionsResolver] removed the unneeded private variable 'lazy' in Opt…
Tobion Jul 27, 2012
a8393e6
[OptionsResolver] small refactoring
Tobion Jul 27, 2012
aa208a9
[OptionsResolver] added performance test
Tobion Jul 28, 2012
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
99 changes: 39 additions & 60 deletions src/Symfony/Component/OptionsResolver/Options.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,10 @@
/**
* Container for resolving inter-dependent options.
*
* Once read, the options cannot be changed anymore.
*
* @author Bernhard Schussek <bschussek@gmail.com>
* @author Tobias Schultze <http://tobion.de>
*/
class Options implements \ArrayAccess, \Iterator, \Countable
{
Expand All @@ -32,18 +35,6 @@ class Options implements \ArrayAccess, \Iterator, \Countable
*/
private $normalizers = array();

/**
* A list storing the names of all options that need to be normalized.
* @var array
*/
private $normalization = array();

/**
* A list storing the names of all LazyOption instances as keys.
* @var array
*/
private $lazy = array();

/**
* A list of Boolean locks for each LazyOption.
* @var array
Expand Down Expand Up @@ -125,10 +116,8 @@ public function setNormalizer($option, \Closure $normalizer)
throw new OptionDefinitionException('Normalizers cannot be added anymore once options have been read.');
}

$this->normalizers[$option] = $normalizer;

// Each option for which a normalizer exists needs to be normalized
$this->normalization[$option] = true;
$this->normalizers[$option] = $normalizer;
}

/**
Expand Down Expand Up @@ -192,19 +181,9 @@ public function overload($option, $value)
if (isset($params[0]) && null !== ($class = $params[0]->getClass()) && __CLASS__ === $class->name) {
$currentValue = isset($params[1]) && isset($this->options[$option]) ? $this->options[$option] : null;
$value = new LazyOption($value, $currentValue);

// 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->lazy[$option]);

$this->options[$option] = $value;
}

Expand All @@ -229,11 +208,11 @@ public function get($option)
throw new \OutOfBoundsException('The option "' . $option . '" does not exist.');
}

if (isset($this->lazy[$option])) {
if ($this->options[$option] instanceof LazyOption) {
$this->resolve($option);
}

if (isset($this->normalization[$option])) {
if (isset($this->normalizers[$option])) {
$this->normalize($option);
}

Expand Down Expand Up @@ -268,7 +247,6 @@ public function remove($option)
}

unset($this->options[$option]);
unset($this->lazy[$option]);
}

/**
Expand All @@ -285,7 +263,6 @@ public function clear()
}

$this->options = array();
$this->lazy = array();
}

/**
Expand All @@ -299,16 +276,19 @@ public function all()
{
$this->reading = true;

// Performance-wise this is slightly better than
// while (null !== $option = key($this->lazy))
foreach ($this->lazy as $option => $isLazy) {
if (isset($this->lazy[$option])) {
foreach ($this->options as $option => $value) {
// We cannot use $value here because a lazy option might have
// invoked another lazy option and thus it might already be resolved
// during this loop.
if ($this->options[$option] instanceof LazyOption) {
$this->resolve($option);
}
}

foreach ($this->normalization as $option => $normalize) {
if (isset($this->normalization[$option])) {
foreach ($this->normalizers as $option => $normalizer) {
// This test is necessary for the same reason as above (applied to
// normalizers).
if (isset($this->normalizers[$option])) {
$this->normalize($option);
}
}
Expand Down Expand Up @@ -443,28 +423,15 @@ public function count()
*/
private function resolve($option)
{
if (isset($this->lock[$option])) {
$conflicts = array();

foreach ($this->lock as $option => $locked) {
if ($locked) {
$conflicts[] = $option;
}
}

throw new OptionDefinitionException('The options "' . implode('", "', $conflicts) . '" have a cyclic dependency.');
}
$this->checkCyclicDependency($option);

$this->lock[$option] = true;
$this->options[$option] = $this->options[$option]->evaluate($this);
unset($this->lock[$option]);

// The option now isn't lazy anymore
unset($this->lazy[$option]);
}

/**
* Normalizes the given option.
* Normalizes the given option.
*
* The evaluated value is written into the options array.
*
Expand All @@ -474,6 +441,28 @@ private function resolve($option)
* on another option.
*/
private function normalize($option)
{
$this->checkCyclicDependency($option);

/** @var \Closure $normalizer */
$normalizer = $this->normalizers[$option];

$this->lock[$option] = true;
$this->options[$option] = $normalizer($this, $this->options[$option]);
unset($this->lock[$option]);

// The option is now normalized
unset($this->normalizers[$option]);
}

/**
* Checks if there is cyclic dependency for the option by examining the locks.
*
* @param string $option The option
*
* @throws OptionDefinitionException If there is a cyclic dependency for the option.
*/
private function checkCyclicDependency($option)
{
if (isset($this->lock[$option])) {
$conflicts = array();
Expand All @@ -486,15 +475,5 @@ private function normalize($option)

throw new OptionDefinitionException('The options "' . implode('", "', $conflicts) . '" have a cyclic dependency.');
}

/** @var \Closure $normalizer */
$normalizer = $this->normalizers[$option];

$this->lock[$option] = true;
$this->options[$option] = $normalizer($this, $this->options[$option]);
unset($this->lock[$option]);

// The option is now normalized
unset($this->normalization[$option]);
}
}
54 changes: 52 additions & 2 deletions src/Symfony/Component/OptionsResolver/Tests/OptionsTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ public function testSetLazyOption()
{
$test = $this;

$this->options->set('foo', function (Options $options) use ($test) {
$this->options->set('foo', function (Options $options) {
return 'dynamic';
});

Expand Down Expand Up @@ -166,7 +166,7 @@ public function testPreviousValueIsNotEvaluatedIfNoSecondArgument()
});

// defined by subclass, no $previousValue argument defined!
$this->options->overload('foo', function (Options $options) use ($test) {
$this->options->overload('foo', function (Options $options) {
return 'dynamic';
});

Expand Down Expand Up @@ -458,8 +458,58 @@ public function testOptionsIteration()
{
$this->options->set('foo', 'bar');
$this->options->set('foo1', 'bar1');

$expectedResult = array('foo' => 'bar', 'foo1' => 'bar1');

$this->assertEquals($expectedResult, iterator_to_array($this->options, true));
}

/**
* Calls the methods of the Options class many times.
*
* @group benchmark
*/
public function testPerformance()
{
$s = microtime(true);

for ($n = 0; $n < 1000; $n++) {
for ($i = 0; $i < 50; $i++) {
$this->options->set('temp'.$i, 'foo');
}
$this->options->clear();
}

for ($i = 0; $i < 5000; $i++) {
$this->options->overload('static', function (Options $options) {
return 'test';
});
$this->options->set('static', 'bar');
$this->options->setNormalizer('static', function (Options $options) {
return 'BAR';
});

$this->options->set('foo', 'bar');
$this->options->remove('foo');

$opt = 'opt'.($i % 300);
$this->options->overload($opt, 'bar');
$this->options->overload($opt, function (Options $options, $previousValue) {
return $previousValue . 'test';

});
$this->options->setNormalizer($opt, function (Options $options, $previousValue) {
return $previousValue . $options->get('static');
});
}

for ($i = 0; $i < 3000; $i++) {
$this->options->get('opt0');
$this->options->all();
$this->options->get('opt'.($i % 300));
}

$time = microtime(true) - $s;
$this->assertTrue($time < 1);
}
}