Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Service manager performance optimized #4139

Merged
merged 6 commits into from

3 participants

Marco Pivetta Mike Willbanks Matthew Weier O'Phinney
Marco Pivetta
Collaborator

This PR inlines some of the methods internal to the service manager. Following changes may be potential BC breaks:

  • canCreate has been deprecated (as it seems, it is an internal method anyway. Users should keep using has)
  • isSubclassOf has been deprecated (was used internally)
  • The canonical names map is re-used directly, instead of calling canonicalizeName each time

NOTE: if this one is ok, remove the performance test case before merging (or tell me to do so)

Marco Pivetta
Collaborator

No idea whatsoever why the build fails. Ok, @weierophinney found out it's an un-related problem with RandomLib going out of memory.

For comparison, here's the results over 100000 iterations:

  • Zend\ServiceManager\ServiceManager::get (with defined service):

    • before: 0.65002608299255
    • after: 0.2622389793396
  • Zend\ServiceManager\ServiceManager::has (with defined service):

    • before: 0.65546894073486
    • after: 0.33996605873108
  • Zend\ServiceManager\ServiceManager::has (without defined service):

    • before: 0.84106206893921
    • after: 0.517333984375
  • Zend\ServiceManager\ServiceManager::create (without defined invokable):

    • before: 0.79828000068665
    • after: 0.70576095581055
Mike Willbanks
Collaborator

I see that also var_dump'd in the test... you may consider removing it :)

   [concat]                 ZendTest/ServiceManager
   [concat] 
   [concat]             PHPUnit 3.7.19 by Sebastian Bergmann.
   [concat] 
   [concat] Configuration read from /home/travis/build/zendframework/zf2/tests/phpunit.xml.dist
   [concat] 
   [concat] ........SS.string(73) "ZendTest\ServiceManager\ServiceManagerPerformanceTest::testGetPerformance"
   [concat] array(2) {
   [concat]   ["time"]=>
   [concat]   float(0.83468294143677)
   [concat]   ["iterations"]=>
   [concat]   int(100000)
   [concat] }
   [concat] .string(73) "ZendTest\ServiceManager\ServiceManagerPerformanceTest::testHasPerformance"
   [concat] array(2) {
   [concat]   ["time"]=>
   [concat]   float(1.0163159370422)
   [concat]   ["iterations"]=>
   [concat]   int(100000)
   [concat] }
   [concat] .string(86) "ZendTest\ServiceManager\ServiceManagerPerformanceTest::testHasPerformanceWithNoService"
   [concat] array(2) {
   [concat]   ["time"]=>
   [concat]   float(1.4539151191711)
   [concat]   ["iterations"]=>
   [concat]   int(100000)
   [concat] }
   [concat] .string(76) "ZendTest\ServiceManager\ServiceManagerPerformanceTest::testCreatePerformance"
   [concat] array(2) {
   [concat]   ["time"]=>
   [concat]   float(1.9051969051361)
   [concat]   ["iterations"]=>
   [concat]   int(100000)
   [concat] }
   [concat] ................................................... 65 / 74 ( 87%)
   [concat] .........
   [concat] 
   [concat] Time: 5 seconds, Memory: 11.25Mb
   [concat] 
   [concat] OK, but incomplete or skipped tests!
   [concat] Tests: 74, Assertions: 107, Skipped: 2.
Marco Pivetta
Collaborator

@mwillbanks yes, these performance tests are here for the only purpose of showing that it works. I noted that in the PR description

Matthew Weier O'Phinney weierophinney merged commit 89b6d01 into from
Deleted user Unknown referenced this pull request from a commit
Matthew Weier O'Phinney weierophinney Merge branch 'hotfix/4139' into develop
Close #4139
70b6348
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
Showing with 70 additions and 57 deletions.
  1. +70 −57 library/Zend/ServiceManager/ServiceManager.php
127 library/Zend/ServiceManager/ServiceManager.php
View
@@ -233,7 +233,7 @@ public function setInvokableClass($name, $invokableClass, $shared = null)
}
if ($shared === null) {
- $shared = $this->shareByDefault();
+ $shared = $this->shareByDefault;
}
$this->invokableClasses[$cName] = $invokableClass;
@@ -256,7 +256,7 @@ public function setFactory($name, $factory, $shared = null)
{
$cName = $this->canonicalizeName($name);
- if (!is_string($factory) && !$factory instanceof FactoryInterface && !is_callable($factory)) {
+ if (!($factory instanceof FactoryInterface || is_string($factory) || is_callable($factory))) {
throw new Exception\InvalidArgumentException(
'Provided abstract factory must be the class name of an abstract factory or an instance of an AbstractFactoryInterface.'
);
@@ -273,7 +273,7 @@ public function setFactory($name, $factory, $shared = null)
}
if ($shared === null) {
- $shared = $this->shareByDefault();
+ $shared = $this->shareByDefault;
}
$this->factories[$cName] = $factory;
@@ -292,24 +292,16 @@ public function setFactory($name, $factory, $shared = null)
*/
public function addAbstractFactory($factory, $topOfStack = true)
{
- if (!is_string($factory) && !$factory instanceof AbstractFactoryInterface) {
+ if (!$factory instanceof AbstractFactoryInterface && is_string($factory)) {
+ $factory = new $factory();
+ }
+
+ if (!$factory instanceof AbstractFactoryInterface) {
throw new Exception\InvalidArgumentException(
- 'Provided abstract factory must be the class name of an abstract factory or an instance of an AbstractFactoryInterface.'
+ 'Provided abstract factory must be the class name of an abstract'
+ . ' factory or an instance of an AbstractFactoryInterface.'
);
}
- if (is_string($factory)) {
- if (!class_exists($factory, true)) {
- throw new Exception\InvalidArgumentException(
- 'Provided abstract factory must be the class name of an abstract factory or an instance of an AbstractFactoryInterface.'
- );
- }
- $refl = new ReflectionClass($factory);
- if (!$refl->implementsInterface(__NAMESPACE__ . '\\AbstractFactoryInterface')) {
- throw new Exception\InvalidArgumentException(
- 'Provided abstract factory must be the class name of an abstract factory or an instance of an AbstractFactoryInterface.'
- );
- }
- }
if ($topOfStack) {
array_unshift($this->abstractFactories, $factory);
@@ -329,13 +321,14 @@ public function addAbstractFactory($factory, $topOfStack = true)
*/
public function addInitializer($initializer, $topOfStack = true)
{
- if (!is_callable($initializer) && !$initializer instanceof InitializerInterface) {
- if (!is_string($initializer)
- || !$this->isSubclassOf($initializer, __NAMESPACE__ . '\InitializerInterface')
- ) {
+ if (!($initializer instanceof InitializerInterface || is_callable($initializer))) {
+ if (is_string($initializer)) {
+ $initializer = new $initializer;
+ }
+
+ if (!($initializer instanceof InitializerInterface || is_callable($initializer))) {
throw new Exception\InvalidArgumentException('$initializer should be callable.');
}
- $initializer = new $initializer;
}
if ($topOfStack) {
@@ -410,10 +403,16 @@ public function setShared($name, $isShared)
*/
public function get($name, $usePeeringServiceManagers = true)
{
- $cName = $this->canonicalizeName($name);
+ // inlined code from ServiceManager::canonicalizeName for performance
+ if (isset($this->canonicalNames[$name])) {
+ $cName = $this->canonicalNames[$name];
+ } else {
+ $cName = $this->canonicalizeName($name);
+ }
+
$isAlias = false;
- if ($this->hasAlias($cName)) {
+ if (isset($this->aliases[$cName])) {
$isAlias = true;
do {
@@ -421,10 +420,9 @@ public function get($name, $usePeeringServiceManagers = true)
} while ($this->hasAlias($cName));
}
- $instance = null;
- $retrieveFromPeeringManagerFirst = $this->retrieveFromPeeringManagerFirst();
+ $instance = null;
- if ($usePeeringServiceManagers && $retrieveFromPeeringManagerFirst) {
+ if ($usePeeringServiceManagers && $this->retrieveFromPeeringManagerFirst) {
$instance = $this->retrieveFromPeeringManager($name);
if (null !== $instance) {
@@ -437,15 +435,21 @@ public function get($name, $usePeeringServiceManagers = true)
}
if (!$instance) {
- if ($this->canCreate(array($cName, $name))) {
+ if (
+ isset($this->invokableClasses[$cName])
+ || isset($this->factories[$cName])
+ || isset($this->aliases[$cName])
+ || isset($this->instances[$cName])
+ || $this->canCreateFromAbstractFactory($cName, $name)
+ ) {
$instance = $this->create(array($cName, $name));
- } elseif ($usePeeringServiceManagers && !$retrieveFromPeeringManagerFirst) {
+ } elseif ($usePeeringServiceManagers && !$this->retrieveFromPeeringManagerFirst) {
$instance = $this->retrieveFromPeeringManager($name);
}
}
// Still no instance? raise an exception
- if ($instance === null && !is_array($instance)) {
+ if ($instance === null) {
if ($isAlias) {
throw new Exception\ServiceNotFoundException(sprintf(
'An alias "%s" was requested but no service could be found.',
@@ -461,7 +465,7 @@ public function get($name, $usePeeringServiceManagers = true)
}
if (
- ($this->shareByDefault() && !isset($this->shared[$cName]))
+ ($this->shareByDefault && !isset($this->shared[$cName]))
|| (isset($this->shared[$cName]) && $this->shared[$cName] === true)
) {
$this->instances[$cName] = $instance;
@@ -486,7 +490,13 @@ public function create($name)
list($cName, $rName) = $name;
} else {
$rName = $name;
- $cName = $this->canonicalizeName($rName);
+
+ // inlined code from ServiceManager::canonicalizeName for performance
+ if (isset($this->canonicalNames[$rName])) {
+ $cName = $this->canonicalNames[$name];
+ } else {
+ $cName = $this->canonicalizeName($name);
+ }
}
@@ -502,7 +512,7 @@ public function create($name)
$instance = $this->createFromAbstractFactory($cName, $rName);
}
- if ($this->throwExceptionInCreate == true && $instance === false) {
+ if ($instance === false && $this->throwExceptionInCreate) {
throw new Exception\ServiceNotFoundException(sprintf(
'No valid instance was found for %s%s',
$cName,
@@ -513,8 +523,6 @@ public function create($name)
foreach ($this->initializers as $initializer) {
if ($initializer instanceof InitializerInterface) {
$initializer->initialize($instance, $this);
- } elseif (is_object($initializer) && is_callable($initializer)) {
- $initializer($instance, $this);
} else {
call_user_func($initializer, $instance, $this);
}
@@ -529,6 +537,8 @@ public function create($name)
* @param string|array $name
* @param bool $checkAbstractFactories
* @return bool
+ *
+ * @deprecated this method is being deprecated as of zendframework 2.2, and may be removed in future major versions
*/
public function canCreate($name, $checkAbstractFactories = true)
{
@@ -539,20 +549,13 @@ public function canCreate($name, $checkAbstractFactories = true)
$cName = $this->canonicalizeName($rName);
}
- if (
+ return (
isset($this->invokableClasses[$cName])
|| isset($this->factories[$cName])
|| isset($this->aliases[$cName])
|| isset($this->instances[$cName])
- ) {
- return true;
- }
-
- if ($checkAbstractFactories && $this->canCreateFromAbstractFactory($cName, $rName)) {
- return true;
- }
-
- return false;
+ || ($checkAbstractFactories && $this->canCreateFromAbstractFactory($cName, $rName))
+ );
}
/**
@@ -567,10 +570,22 @@ public function has($name, $checkAbstractFactories = true, $usePeeringServiceMan
list($cName, $rName) = $name;
} else {
$rName = $name;
- $cName = $this->canonicalizeName($rName);
+
+ // inlined code from ServiceManager::canonicalizeName for performance
+ if (isset($this->canonicalNames[$rName])) {
+ $cName = $this->canonicalNames[$name];
+ } else {
+ $cName = $this->canonicalizeName($name);
+ }
}
- if ($this->canCreate(array($cName, $rName), $checkAbstractFactories)) {
+ if (
+ isset($this->invokableClasses[$cName])
+ || isset($this->factories[$cName])
+ || isset($this->aliases[$cName])
+ || isset($this->instances[$cName])
+ || ($checkAbstractFactories && $this->canCreateFromAbstractFactory($cName, $name))
+ ) {
return true;
}
@@ -595,15 +610,12 @@ public function has($name, $checkAbstractFactories = true, $usePeeringServiceMan
public function canCreateFromAbstractFactory($cName, $rName)
{
// check abstract factories
- foreach ($this->abstractFactories as $index => $abstractFactory) {
- // Support string abstract factory class names
- if (is_string($abstractFactory) && class_exists($abstractFactory, true)) {
- $this->abstractFactories[$index] = $abstractFactory = new $abstractFactory();
- }
+ foreach ($this->abstractFactories as $abstractFactory) {
+ $factoryClass = get_class($abstractFactory);
if (
- isset($this->pendingAbstractFactoryRequests[get_class($abstractFactory)])
- && $this->pendingAbstractFactoryRequests[get_class($abstractFactory)] == $rName
+ isset($this->pendingAbstractFactoryRequests[$factoryClass])
+ && $this->pendingAbstractFactoryRequests[$factoryClass] == $rName
) {
return false;
}
@@ -655,8 +667,7 @@ public function setAlias($alias, $nameOrAlias)
*/
public function hasAlias($alias)
{
- $alias = $this->canonicalizeName($alias);
- return (isset($this->aliases[$alias]));
+ return isset($this->aliases[$this->canonicalizeName($alias)]);
}
/**
@@ -939,6 +950,8 @@ protected function createFromAbstractFactory($canonicalName, $requestedName)
* @param string $className
* @param string $type
* @return bool
+ *
+ * @deprecated this method is being deprecated as of zendframework 2.2, and may be removed in future major versions
*/
protected static function isSubclassOf($className, $type)
{
Something went wrong with that request. Please try again.