Performance enhancements for has(), setService(), setFactory(), setShared(), setAlias(), setInvokableClass() and configure() #221
Conversation
fhein
changed the title
|
Here is an overall comparison of develop against this PR. The benchs of develop which are faster are for your convenience marked with x. The benchs where this PR outoperforms develop by much are marked with !. @dantleech: This is a showcase where it would help that the comparison report would pull suite::context (as documented) instead of suite::uuid. I had to manually reformat. Not much fun. And, if you want me to beg, I'll do: For you it's just a finger snip to hardcode a comparison column (like 1.24x as in reports=aggregate). Assume that the first --file is the base, the second --file is the - hopefully - faster implementation. Even if you would do that just for me, I bet, tons of users would use that. Please! Please! Please! Pleeeeaaaaseee? :) |
It uses UUID because the Could you create an issue on PHPBench with the exact format of the report that you're after? |
|
A 970-word essay imploring the maintainers to accept your PR (before they've added a single comment) seems like the wrong approach to open source contributing. |
|
Accepted. Will not happen again.
Mit IBM Verse von meinem iPhone aus gesendet
Am 06.01.2018 um 16:57:33 hat [email protected] folgendes geschrieben:
From: [email protected]
To: [email protected]
Cc: [email protected], [email protected]
Date: 06.01.2018, 16:57:33
Subject: Re: [zendframework/zend-servicemanager] Performance enhancements for setService, setFactory, setShared and setAlias(!) (#221)
A 970-word essay imploring the maintainers to accept your PR (before they've added a single comment) seems like the wrong approach.
—You are receiving this because you authored the thread.Reply to this email directly, view it on GitHub, or mute the thread.
|
|
Essays removed. I felt somewhat treated like McStupid in the first time (other PRs). Did not understand why, And with a bottle of wine input I tend to get verbous. |
|
@fhein We perform code review for a number of reasons:
If we are not pointing out the why behind a request, ask. But do so in a collegial way. For instance, "I disagree with that request; why should I do that?" is combative and defensive, while "Could you explain the rationale behind that request? Is it part of a coding style? If so, can you provide some examples for me?" indicates a collaborative approach indicating you're willing to learn. If the reviewer cannot provide a rationale, then you can point out your own, so that we can learn. Additionally, in some cases, one reviewer will ask another to jump in. In a previous patch of yours, @Ocramius was asking for my opinion on a potential future change regarding aliases. This may not affect the patch you're making, but if a future change might make the need for your patch obsolete, there's no point in us making a change we do not plan to maintain. Having a discussion around what we choose to maintain is important. Finally, I read your original essay, and I want to point out something: your aspersions at "how could such code even make it into v3" are totally out of line. Why? Because in a year or two, somebody else is going to come along with a huge performance patch, and ask the same question, and now you are the target of that. Asking such questions is in no way helpful. Software evolves. The language evolves. Our understanding of the capabilities of the language evolves. As such, treat everyone as you would ask us to treat you. When in doubt, refer to our Code of Conduct. |
|
I understand, that there can be rationales behind requests to change a PR, which may not be obvious for the contributor. And that the contributor should ask then. But please allow a review of that particular request, which literally was 'Variable is redundant'. My code was like I did it the same way the other test functions do to be compliant with the coding style. It actually did not even come to my mind to ask "Could you explain the rationale behind that request? Is it part of a coding style? If so, can you provide some examples for me?" What came to my mind, was "What variable do you mean? $config?" and that's what I asked. I did as requested. Now my test looks different from the others. I still do not understand the rationale behind that. Could you please explain? |
|
I generally review code from my phone while walking or in a coffee break,
so I only have an overview on the diff displayed, not the entire test.
Marco Pivetta
http://twitter.com/Ocramius
http://ocramius.github.com/
…On Mon, Jan 8, 2018 at 4:37 PM, Frank Hein ***@***.***> wrote:
I understand, that there can be rationales behind requests to change a PR,
which may not be obvious for the contributor. And that the contributor
should ask then.
But please allow a review of that particular request, which literally was
'Variable is redundant'.
My code was like
public function textXYZ() {
$config = [ ... ];
$sm =new ServiceManager($config);
// following code did not use $config
}
I did it the same way the other test functions do to be compliant with the
coding style.
In ServiceManagerTest.php please review
testSetAliasShouldWorkWithRecursiveAlias()
testAliasToAnExplicitServiceShouldWork()
testSharedServicesReferencingAliasShouldBeHonored()
testSharedServicesReferencingInvokableAliasShouldBeHonored()
testMapsNonSymmetricInvokablesAsAliasPlusInvokableFactory()
testMapsOneToOneInvokablesAsInvokableFactoriesInternally()
testShareability($sharedByDefault, $serviceShared, $serviceDefined, $shouldBeSameInstance)
testCanWrapCreationInDelegators()
It actually did not even come to my mind to ask "Could you explain the
rationale behind that request? Is it part of a coding style? If so, can you
provide some examples for me?" What came to my mind, was "What variable do
you mean? $config?" and that's what I asked.
I did as requested. Now my test looks different from the others. I still
do not understand the rationale behind that. Could you please explain?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#221 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAJakNoUtjXqfoOPpft0cwlJ8sMbHNZJks5tIjZIgaJpZM4RUClU>
.
|
|
Am I supposed to know that? |
|
No, but you asked:
I did as requested. Now my test looks different from the others. I still
do not understand the rationale behind that. Could you please explain?
Marco Pivetta
http://twitter.com/Ocramius
http://ocramius.github.com/
…On Mon, Jan 8, 2018 at 4:47 PM, Frank Hein ***@***.***> wrote:
Am I supposed to know that?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#221 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAJakHJCPGhDaSwMnu3uh1F0dHWQDS_0ks5tIjiVgaJpZM4RUClU>
.
|
|
Thanks for the rationale. It's very personal. Regarding the optimizations of the 'setters'. I consider it as bad practice to discard caller supplied information and regather it again. This was done in all setters. What I mean is Is that really a matter of software evolution and PHP language evolution? I don't think so. But maybe I do not have the whole picture. Please explain. |
|
The setters are not hot path - besides `configure()`, the setters are
extremely edge case usage scenario, and should generally be avoided.
Re-using configure() internally is just to make it simple and nice to read.
Marco Pivetta
http://twitter.com/Ocramius
http://ocramius.github.com/
…On Mon, Jan 8, 2018 at 5:01 PM, Frank Hein ***@***.***> wrote:
Thanks for the rationale. It's very personal.
Regarding the optimizations of the 'setters'. I consider it as bad
practice to discard caller supplied information and regather it again. This
was done in all setters. What I mean is
function setService($name, $service)
{
// here we know already that the user wants to set a service
$this->configure( ['services' => [$name, $service]]); // here we drop that information
// configure then has to find out again, what (in cases routed from setXYZ) the
// the user wanted via isset($config['services'])
}
Is that really a matter of software evolution and PHP language evolution?
I don't think so. But maybe I do not have the whole picture. Please explain.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#221 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAJakMenHwb1kEaCKcsX1KJBBVvkY9z7ks5tIjvygaJpZM4RUClU>
.
|
|
Nicer to read than the performance optimized versions? That's how they look now. Performance and readibility don't mutually exclude each other. |
This last part is fairly important; after the container is configured initially, we do not want somebody altering it such that a different service is returned for two subsequent requests to the same service name. This can only be done if all setters use the same logic, and As @Ocramius notes, the various setter methods should generally not be used unless you are building the container service entries programmatically. Typical usage (see the zend-mvc and Expressive skeletons) uses a configuration-driven approach instead, where all services are injected during instantiation. |
|
As of my PR the setters and configure utilize the same implemenations to address your bullets. There is no code duplication. And even this is not a matter of software and language evolution. See Knuth, The Art of Computer Programming. Even when I studied serveral decades ago we had that as a recommended reference. If you review the code, you will find that all setters use the same logic as configure. A setter uses the logic for the particular single request. configure uses said logic to handle all of the settings (requests) provided. This PR also maintains the current handling of Invokables as you described it. This PR does not contradict any of the goals you are striving at. The only thing which changes is performance. Please explain the rationale behind deprecating Invokables. Please see #223. |
|
Invokables provide an alternative way to define an implicit factory. The
fact that two separate config keys exists for invokables and factories
leads to hard-to-override configuration as well as more confusion (from a
user perspective) on priority in service resolution.
In most scenarios, in a ZF app, what happens is that an invokable defined
in one module requires more elaborate configuration needed by consumers,
and adding a factory leads to a service definition collision.
Making the implicit factory explicit simplifies things.
…On 8 Jan 2018 17:34, "Frank Hein" ***@***.***> wrote:
As of my PR the setters and configure utilize the same implemenations to
address your bullets. There is no code duplication. validate guards
against overriding. private doSetAlias does all of the alias resolution
inclduding cycle detection. Not more required to handle all of this than
private function doSetAlias($alias, $target)
{
$this->aliases[$alias] = $target;
$this->resolvedAliases[$alias] =
isset($this->resolvedAliases[$target]) ? $this->resolvedAliases[$target] : $target;
if ($alias === $this->resolvedAliases[$alias]) {
throw CyclicAliasException::fromAliasesMap([$alias]);
}
if (in_array($alias, $this->resolvedAliases)) {
$r = array_intersect($this->resolvedAliases, [ $alias ]);
foreach ($r as $name => $service) {
$this->resolvedAliases[$name] = $target;
}
}
}
And even this is not matter of software and language evolution. See Knuth,
The Art of Computer Programming
<https://en.wikipedia.org/wiki/The_Art_of_Computer_Programming>.
If you review the code, you will find that all setters use the same logic
as configure. A setter uses the logic for the particular single request.
configure uses said logic to handle all of the settings (requests) provided.
This PR also maintains the current handling of Invokables as you described
it.
Please explain the rationale behind deprecating Invokables. Please see
#223 <#223>.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#221 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAJakD9XyVUdtwiZR02GYsDHPIBees6zks5tIkObgaJpZM4RUClU>
.
|
|
Thanks. I will think about all the implications that has. Nothing for a quick shot reply. Anyway, this PR does not touch how Invokables are handled. Semantics at all is not touched by this PR. It's about performance only. I opened issues to discuss feature requests and changes that could change behaviour . (#222, #223, #224, #225). |
fhein
changed the title
| use function sprintf; | ||
|
|
||
| class CyclicAliasException extends InvalidArgumentException | ||
| { | ||
| public static $cycle; |
| * @param string[] $aliases map of referenced services, indexed by alias name (string) | ||
| * | ||
| * @return self | ||
| */ | ||
| public static function fromAliasesMap(array $aliases) | ||
| public static function fromCyclicAlias($alias, $aliases) |
There was a problem hiding this comment.
Agreed; create a new method, and leave the original in place. We can mark the original deprecated if it is no longer used internally.
| * @return array|null | ||
| */ | ||
| private static function getCycleFor(array $aliases, $alias) | ||
| public function getCycle() |
There was a problem hiding this comment.
Also:
- Why is it non-static?
- I don't see this method ever used, so why does it exist?
src/ServiceManager.php
Outdated
| @@ -429,7 +422,8 @@ private function configureAliases(array $aliases) | |||
| */ | |||
| public function setAlias($alias, $target) | |||
| { | |||
| $this->configure(['aliases' => [$alias => $target]]); | |||
| $this->validate($alias); | |||
There was a problem hiding this comment.
The naming validate() is too shallow - need to roughly express what it does in the method name
There was a problem hiding this comment.
Agreed. There's another consideration as well: AbstractPluginManager also defines a validate() method, used to determine if an instance is a valid plugin. As such, having a validate() method in both could lead to a conflict and unexpected results.
Please rename the method to validateServiceName.
| @@ -248,21 +253,26 @@ public function build($name, array $options = null) | |||
| */ | |||
| public function has($name) | |||
| { | |||
| $name = $this->resolvedAliases[$name] ?? $name; | |||
There was a problem hiding this comment.
When rebasing on develop, you will notice that this change leads to a test failure
There was a problem hiding this comment.
This PR is based on develop. Please explain a little bit more. All tests pass locally.
src/ServiceManager.php
Outdated
| @@ -519,59 +515,52 @@ public function setService($name, $service) | |||
| */ | |||
| public function setShared($name, $flag) | |||
| { | |||
| $this->configure(['shared' => [$name => (bool) $flag]]); | |||
| $this->validate($name); | |||
| $this->shared[$name] = (bool) $flag; | |||
There was a problem hiding this comment.
We should just add bool to the argument, but that may indeed be a BC break for subclasses and consumers being too lax. Possibly an issue for 4.0.0 :-\
| @@ -18,137 +18,86 @@ class CyclicAliasExceptionTest extends TestCase | |||
| /** | |||
| * @dataProvider aliasesProvider | |||
| * | |||
| * @param string conflicting alias key | |||
There was a problem hiding this comment.
Changes in this file should be completely reverted
There was a problem hiding this comment.
I've marked a bunch of changes I would like to see, but there are two general ones I only reference a few times throughout:
- Check for function invocations. All PHP functions used should be imported explicitly at the top of the file. During invocation, you do not need to qualify them, since they are already imported.
- I am not a fan of having method names called
do<Original Method Name Here>(); I feel it's lazy and generally doesn't indicate the actual purpose of the method. I've indicated two places where I'd change these specifically, and provide more semantically informed names when I do.
More concerning, however, is that in some cases, you create code duplication in order to save function calls, but in others, you introduce more function calls (e.g., ServiceManager::configure() now performs a function call for each and every abstract factory registered, instead of one for the entire set; it does the same when resolving aliases, adding a function call per alias instead of one for all aliases). This makes me suspect that initial creation of the container will be more expensive for the primary use case of a configuration-driven container. Interestingly, you do not provide benchmark comparisons for that scenario, and that is one of the ones we optimized for for version 3.
I recognize that you and one other person have indicated you use the setter methods as your primary interface. That said, they are not the primary interface we intend at this time. Both the zend-mvc skeleton and Expressive assume a "configure at instantiation" approach, and the zend-config-aggregator component exists to help aid such scenarios. Considering that these are the primary targets for zend-servicemanager, these are the needs where any performance optimizations need to focus.
As such, can you please provide such benchmarks? They would need to address a container with a mix of aliases, factories, invokables, delegators, abstract factories, etc., with multiple definitions for each service type, ideally in the dozens or more.
| { | ||
| $this->validate($service); | ||
| parent::setService($name, $service); | ||
| } |
There was a problem hiding this comment.
I would argue this is a bugfix, and should be submitted separately, along with a unit test for the changed behavior.
There was a problem hiding this comment.
To explain: AbstractPluginManager relied on ServiceManager::setService to call configure. Therefore APM implemented configure to do plugin validation. This broke with the new setService implementation of SM. So I had to catch setService in APM.
| * @param string[] $aliases map of referenced services, indexed by alias name (string) | ||
| * | ||
| * @return self | ||
| */ | ||
| public static function fromAliasesMap(array $aliases) | ||
| public static function fromCyclicAlias($alias, $aliases) |
There was a problem hiding this comment.
Agreed; create a new method, and leave the original in place. We can mark the original deprecated if it is no longer used internally.
| * @return array|null | ||
| */ | ||
| private static function getCycleFor(array $aliases, $alias) | ||
| public function getCycle() |
There was a problem hiding this comment.
Also:
- Why is it non-static?
- I don't see this method ever used, so why does it exist?
src/ServiceManager.php
Outdated
| // /** | ||
| // * @var string[] | ||
| // */ | ||
| // private $resolvedAliases = []; |
There was a problem hiding this comment.
Please do not leave commented-out functionality; if it needs to be revisited, that's what VCS history is for.
src/ServiceManager.php
Outdated
| $object = $this->doCreate($resolvedName); | ||
|
|
||
| // Cache the object for later, if it is supposed to be shared. | ||
| if (($sharedService)) { |
There was a problem hiding this comment.
Remove the duplicate parens, please.
src/ServiceManager.php
Outdated
| */ | ||
| private function validateOverrideSet(array $services, $type) | ||
| private function doAddAbstractFactory($abstractFactory) |
There was a problem hiding this comment.
Please rename this method to resolveAbstractFactoryInstance().
src/ServiceManager.php
Outdated
| foreach ($services as $service) { | ||
| if (isset($this->services[$service])) { | ||
| $detected[] = $service; | ||
| if (\is_string($abstractFactory) && \class_exists($abstractFactory)) { |
There was a problem hiding this comment.
If these functions are not imported already, please do. Then drop the global resolution, as they'll already be imported.
There was a problem hiding this comment.
Ok, I will check the import of all functions. But that's not code I touched. I think, in_array is the only one I introduced.
src/ServiceManager.php
Outdated
| if (isset($this->services[$service])) { | ||
| $detected[] = $service; | ||
| if (\is_string($abstractFactory) && \class_exists($abstractFactory)) { | ||
| //Cached string |
There was a problem hiding this comment.
Space between a comment opener and the comment text, please.
There was a problem hiding this comment.
Ok again. But I did not write that.
src/ServiceManager.php
Outdated
| } | ||
|
|
||
| if (empty($detected)) { | ||
| if ($abstractFactory instanceof Factory\AbstractFactoryInterface) { | ||
| $abstractFactoryObjHash = \spl_object_hash($abstractFactory); |
There was a problem hiding this comment.
spl_object_hash is already imported, so no need to globally qualify it.
There was a problem hiding this comment.
I did not write that either. I'll correct.
src/ServiceManager.php
Outdated
| sprintf( | ||
| 'An invalid abstract factory was registered; resolved to class "%s" ' . | ||
| 'which does not exist; please provide a valid class name resolving ' . | ||
| 'to an implementation of %s', |
There was a problem hiding this comment.
Put concatenation operators on the following lines, please.
Also, inline the sprintf with the line calling new, in order to reduce a level of indentation.
Personally, I'd likely create named constructors for this and the following throw statements, in order to make the code here simpler: throw InvalidArgumentException::forInvalidAbstractFactoryClassName($abstractFactory);
There was a problem hiding this comment.
I'll do. But I did not touch that either. :)
|
Currently one test on travis fails in line 294 of ServiceManagerTest.php: My ServiceManagerTest.php does not contain named test end ends with line 287. I compared several branches, but I could not find a version of this file containing that test. Please help. |
|
Thank you for the thorough review. Highly appreciated. In particular, thanks @weierophinney for the pointer to configure() performance. There is a benchmark already in place ( I run my tests usually with the whole suite --revs=100000. Because FetchNewServiceManager can not stand that much revs (or better, my machine can't stand it) I left that out the last days. Shame on me. :( I was shocked to see that configure() slowed down dramatically. I tracked that down locally to array_intersect which is horribly expensive. To proof that configure() can get accelerated significantly I adapted Tarjan's Strongly Connected Components Algorithm to the alias resolution and cycle detection problem. Please see the comparison against develop following in the next note. |
|
Here are the benchmark results. The test creates a SM with 1,001 factories, invokables, services and aliases each. |
fhein
changed the title
|
May I please ask again for the |
|
You need to perform a git rebase
…On 10 Jan 2018 19:03, "Frank Hein" ***@***.***> wrote:
May I please ask again for the ServiceManagerTest.php which is different
from my local version.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#221 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAJakMwEy4u_wo4Wm_SAqkmzf3sYQijGks5tJPuEgaJpZM4RUClU>
.
|
|
To upstream develop? Says: Already up to date. |
|
Yes, your previous patches were merged, so:
1. git checkout develop
2. git pull --ff-only origin develop
3. git checkout your-branch
4. git rebase develop
…On 10 Jan 2018 19:06, "Frank Hein" ***@***.***> wrote:
To upstream develop?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#221 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAJakIUWKtEpUo9JOl_mzLjGZpsT9B-aks5tJPwIgaJpZM4RUClU>
.
|
| * | ||
| * @return self | ||
| */ | ||
| public static function fromCyclicAlias($alias, $aliases) |
There was a problem hiding this comment.
Looks like we can have typehint array on the second parameter
| @@ -8,10 +8,32 @@ | |||
| namespace Zend\ServiceManager\Exception; | |||
|
|
|||
| use InvalidArgumentException as SplInvalidArgumentException; | |||
| use Zend\ServiceManager\Initializer\InitializerInterface; | |||
| use Zend\ServiceManager\AbstractFactoryInterface; | |||
There was a problem hiding this comment.
Alphabetical order of imports please.
| use function is_callable; | ||
| use function is_object; | ||
| use function is_string; | ||
| use function spl_autoload_register; | ||
| use function spl_object_hash; | ||
| use function sprintf; | ||
| use function trigger_error; | ||
| use function in_array; |
There was a problem hiding this comment.
Alphabetical order of imported function please.
|
@weierophinney: Thank you very much. I feel honoured that this piece of work made it. :) |
|
Thanks, @fhein; this one is now merged to develop. |
|
@fhein Also, be sure to rebase your other open PRs based on this one against current develop; I did a number of minor consistency changes when merging that may cause conflicts. |
|
@weierophinney; Please be aware, that this PR still has that nasty bug I discovered while adding tests to increase code coverage. None of the current tests fails on this bug. A test and a bug fix is provided in #227. |
|
@weierophinney; Minimum action required: Copy and replace mapAliasesToTargets member function from #227 to develop. This will not include the newly introduced test, but it will restore a stable state for develop. Alternatively you may consider to accept #227. This will add the test which fails on current develp also. Future PRs will be more self-contained. I am sorry. |
|
Can you try and open a new PR just with the test and the proposed fix?
Patching without tests is a no-go
…On 17 Jan 2018 07:20, "Frank Hein" ***@***.***> wrote:
@weierophinney <https://github.com/weierophinney>; Minimum action
required: Copy and replace mapAliasesToTargets member function from #227
<#227> to
develop. This will not include the newly introduced test, but it will
restore a stable state for develop. Alternatively you may consider to
accept #227
<#227>. This
will add the test which fails on current develp also.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#221 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAJakFDrxrCoJ8yMTQCh36wg1si52EQfks5tLZEWgaJpZM4RUClU>
.
|
PERFORMANCE-Optimize-configuration-speed-contd
This PR is about performance of the setter APIs (or injector APIs if you like) as well as has(). Here's the current comparison report. Thanks @dantleech for faster than light fix.
This is the develop branch:
Here comes this PR: