-
Notifications
You must be signed in to change notification settings - Fork 89
Performance optimize invokables (bug fix also) #232
Performance optimize invokables (bug fix also) #232
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This patch appears to have a ton of changes from other PRs that have already been merged; additionally, it reverts changes that were already made for internal consistency. Please rebase and revert changes as marked so we can review the specific changes you're proposing.
/** | ||
* @link http://github.com/zendframework/zend-servicemanager for the canonical source repository | ||
* @copyright Copyright (c) 2016 Zend Technologies USA Inc. (http://www.zend.com) | ||
* @license http://framework.zend.com/license/new-bsd New BSD License |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On this and other file-level docblocks for new files introduced by this patch, please do the following:
- Use https scheme for all URLs
- Date is 2018
benchmarks/SetNewServicesBench.php
Outdated
public function benchSetInvokableClass() | ||
{ | ||
|
||
// @todo @link https://github.com/phpbench/phpbench/issues/304 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On this and the following new methods:
- Move the
@todo
annotation to a method-level docblock - Remove the blank line at the top of the method
src/ServiceManager.php
Outdated
} | ||
|
||
$this->mapAliasToTarget($alias, $target); | ||
throw ContainerModificationsNotAllowedException::fromExistingService($alias); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please revert this change; exceptions should be thrown within conditionals for internal consistency and with other components.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My idea behind that is, that by default a configuration and proposed (called) operations on it should get assumed to be error free (they finally are after all errors got fixed). This assumed, the ||
clause (with appropriate ordering, which I am not quite sure yet what the best is (my assumption here is that allowOverride is (should be) false in most production environments)) can pass successfully (true
) after evaluation of just the first expression of the ||
clause at a high probability. If the second part of the clause needs evalution at all, we are half-way down the road to throw.
Inverting the condition would mean, that one maybe two evalutions get necessary to determine not to throw. The assumption here is more pessimistic. Because in most cases we are not gonna throw because people don't request things which are not allowed (my assumption). it's more likely that we have to evaluate both parts of the &&
condition to find out that everything is still ok.
So I believe, there is a (probable) slight performance advantage for error free configs and proper requests. That's what I am heading for. But it's all about assuming things and and estimating probability here.
What do you think? Did I forget to consider something important?
I hope my English is good enough to express my thoughts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are two points I was making:
-
Consistency. Throughout the framework, we generally (as in greater than 99% of cases) throw from a conditional. The method body is where the work of the method is done. In these cases, you're setting an alias, setting a factory, etc. That's the work of the methods. The conditional should be used to flag exceptional cases that prevent that work from happening.
-
Common use case. The primary use case for zend-servicemanager is to configure once. These setters are not the primary path. As such, having them readable and consistent with the code base is more important than micro-optimizations from conditionals.
If you are worried about the conditional performance, due to needing to evaluate two conditions, put the one most likely to be false
first — which is how these were done (i.e., checking to see if the service/alias/factory/etc. has already been set). If the service has not been previously set, only the first part of the condition will be evaluated, and we'll then move on to the next statement.
On top of that, boolean checks (&& ! $this->allowOverride
) are quite fast, and inverting the method workflow for this is truly a micro-optimization at the expense of consistency and readability.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok.
src/ServiceManager.php
Outdated
} | ||
|
||
$this->createAliasesAndFactoriesForInvokables([$name => $class ?? $name]); | ||
throw ContainerModificationsNotAllowedException::fromExistingService($name); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Invert the conditional and throw from the conditional, please.
src/ServiceManager.php
Outdated
} | ||
|
||
$this->factories[$name] = $factory; | ||
throw ContainerModificationsNotAllowedException::fromExistingService($name); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please revert this change; exceptions should be thrown within conditionals for internal consistency and with other components.
src/ServiceManager.php
Outdated
} | ||
$this->services[$name] = $service; | ||
throw ContainerModificationsNotAllowedException::fromExistingService($name); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please revert this change; exceptions should be thrown within conditionals for internal consistency and with other components.
src/ServiceManager.php
Outdated
} | ||
|
||
$this->shared[$name] = (bool) $flag; | ||
throw ContainerModificationsNotAllowedException::fromExistingService($name); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please revert this change; exceptions should be thrown within conditionals for internal consistency and with other components.
src/ServiceManager.php
Outdated
* connected components (i.e. cycles in our case), we throw. | ||
* If nodes are not strongly connected (i.e. resolvable in | ||
* our case), they get resolved. | ||
* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes in this method appear to be from #230; please rebase, so that this patch only contains the changes you propose.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
*/ | ||
private function resolveAbstractFactoryInstance($abstractFactory) | ||
private function resolveAbstractFactories($abstractFactories) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes in this method appear to be from a previously merged commit; please rebase, so that this patch only contains the changes you propose.
]); | ||
$this->assertSame($sm->get('alias1'), $sm->get('alias2')); | ||
$this->assertSame($sm->get(stdClass::class), $sm->get('alias1')); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These two changes are from #230; please rebase.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
5f84975
to
0f6b631
Compare
0f6b631
to
0b51895
Compare
Closed because this primarily a bug fix for invokable handling. See #242. |
Current master and develop branches provide a special treatment for invokables within sm configuration phase. This treatment requires to loop through the invokables array and perform actions and create an InvokableFactory entry for each Invokable and an alias if the name of the invokable is different from the invokable class name. This is a major bottleneck for configuration speed. This treatment alone takes about 30% of overall configuration time.
Here is the benchmark comparing master to this PR.
The replacement of an invokable by a factory and an alias can also be considered as a bug. Current service resolution precedence is:
As @Ocramius said, there are use cases, where users want to provide a factory for an invokable class, because the particular application wants to apply configuration to the invokable object.
By defining an alias for the invokable class, this factory gets disabled efffectively.
This PR changes the treatment of invokables. They are stored in an $invokables array like the other items.
Now invokables get resolved to an InvokableFactory in getFactory now. The new resolution precedence is
I tried to discuss the problems of alias resolution precedence in #222. I do not know if there are discussions around that topic actually. At least nobody wanted to share his/her thoughts since I published that article weeks ago. Trouble with aliases could get eliminated by reducing them to aliases. That would mean to change the resolution precedence to