search

zendframework / zend-servicemanager

ServiceManager component from Zend Framework
BSD 3-Clause "New" or "Revised" License
188 stars 89 forks source link

BUGFIXES: Processing of preconfigured settings and invokable resolution #242

Closed fhein closed 6 years ago

fhein commented 6 years ago

This PR adds tests and fixes for several issues around configuration and resolution of Invokables.

  1. Preconfigured abstract factories are not resolved.
  2. Preconfigured initializers are not resolved.
  3. Preconfigured invokables are not resolved.
  4. Invokables defined as name => InvokableClass can override delegators and factories.

A performance gain more than 100% in FetchNewServiceManagerBench.php is a welcome side effect.

fhein commented 6 years ago

This is ready for review.

fhein commented 6 years ago

Heres the benchmark for ServiceManagerCreation. Done with --revs=500 --iterations=50.

$ vendor\bin\phpbench report --file=..\master.FetchNewServiceManager.xml --file=..\PR242.FetchNewServiceManager.xml --report=compare
benchmark: FetchNewServiceManagerBench
+----------------------------------+-------------------+------------------+
| subject                          | suite:master:mean | suite:PR242:mean |
+----------------------------------+-------------------+------------------+
| benchFetchServiceManagerCreation | 878.050µs         | 387.422µs        |
+----------------------------------+-------------------+------------------+
fhein commented 6 years ago

Here are the comparison reports for the rest of the benchmarks. Tests which feature the creation of invokables profit from the fix.

$ vendor\bin\phpbench report --file=..\master.all.xml --file=..\PR242.all.xml --report=compare
benchmark: FetchCachedServicesBench
+----------------------------------+-------------------+------------------+
| subject                          | suite:master:mean | suite:PR242:mean |
+----------------------------------+-------------------+------------------+
| benchFetchFactory1               | 0.452µs           | 0.459µs          |
| benchFetchInvokable1             | 0.473µs           | 0.474µs          |
| benchFetchService1               | 0.457µs           | 0.461µs          |
| benchFetchAlias1                 | 0.458µs           | 0.459µs          |
| benchFetchRecursiveAlias1        | 0.474µs           | 0.477µs          |
| benchFetchRecursiveAlias2        | 0.468µs           | 0.473µs          |
| benchFetchAbstractFactoryService | 2.450µs           | 2.529µs          |
+----------------------------------+-------------------+------------------+

benchmark: FetchNewServiceUsingConfigAbstractFactoryAsFactoryBench
+-------------------------------------+-------------------+------------------+
| subject                             | suite:master:mean | suite:PR242:mean |
+-------------------------------------+-------------------+------------------+
| benchFetchServiceWithNoDependencies | 5.042µs           | 4.823µs          |
| benchBuildServiceWithNoDependencies | 4.613µs           | 4.414µs          |
| benchFetchServiceDependingOnConfig  | 5.744µs           | 5.443µs          |
| benchBuildServiceDependingOnConfig  | 5.306µs           | 5.021µs          |
| benchFetchServiceWithDependency     | 5.681µs           | 5.402µs          |
| benchBuildServiceWithDependency     | 5.210µs           | 4.970µs          |
+-------------------------------------+-------------------+------------------+

benchmark: FetchNewServiceUsingReflectionAbstractFactoryAsFactoryBench
+-------------------------------------+-------------------+------------------+
| subject                             | suite:master:mean | suite:PR242:mean |
+-------------------------------------+-------------------+------------------+
| benchFetchServiceWithNoDependencies | 3.963µs           | 3.779µs          |
| benchBuildServiceWithNoDependencies | 3.537µs           | 3.314µs          |
| benchFetchServiceDependingOnConfig  | 7.089µs           | 6.746µs          |
| benchBuildServiceDependingOnConfig  | 6.650µs           | 6.303µs          |
| benchFetchServiceWithDependency     | 8.432µs           | 7.954µs          |
| benchBuildServiceWithDependency     | 7.960µs           | 7.559µs          |
+-------------------------------------+-------------------+------------------+

benchmark: FetchNewServiceViaConfigAbstractFactoryBench
+-------------------------------------+-------------------+------------------+
| subject                             | suite:master:mean | suite:PR242:mean |
+-------------------------------------+-------------------+------------------+
| benchFetchServiceWithNoDependencies | 5.489µs           | 5.228µs          |
| benchBuildServiceWithNoDependencies | 4.922µs           | 4.681µs          |
| benchFetchServiceDependingOnConfig  | 6.143µs           | 6.225µs          |
| benchBuildServiceDependingOnConfig  | 5.601µs           | 5.322µs          |
| benchFetchServiceWithDependency     | 6.122µs           | 5.825µs          |
| benchBuildServiceWithDependency     | 5.564µs           | 5.267µs          |
+-------------------------------------+-------------------+------------------+

benchmark: FetchNewServiceViaReflectionAbstractFactoryBench
+-------------------------------------+-------------------+------------------+
| subject                             | suite:master:mean | suite:PR242:mean |
+-------------------------------------+-------------------+------------------+
| benchFetchServiceWithNoDependencies | 3.434µs           | 3.304µs          |
| benchBuildServiceWithNoDependencies | 2.919µs           | 2.702µs          |
| benchFetchServiceDependingOnConfig  | 6.766µs           | 6.397µs          |
| benchBuildServiceDependingOnConfig  | 6.221µs           | 5.844µs          |
| benchFetchServiceWithDependency     | 8.095µs           | 7.682µs          |
| benchBuildServiceWithDependency     | 7.555µs           | 7.149µs          |
+-------------------------------------+-------------------+------------------+

benchmark: FetchNewServicesBench
+----------------------------------+-------------------+------------------+
| subject                          | suite:master:mean | suite:PR242:mean |
+----------------------------------+-------------------+------------------+
| benchFetchFactory1               | 2.820µs           | 2.839µs          |
| benchBuildFactory1               | 2.395µs           | 2.184µs          |
| benchFetchInvokable1             | 3.315µs           | 2.340µs          |
| benchBuildInvokable1             | 2.620µs           | 1.938µs          |
| benchFetchService1               | 0.455µs           | 0.468µs          |
| benchFetchFactoryAlias1          | 2.454µs           | 2.341µs          |
| benchBuildFactoryAlias1          | 2.461µs           | 2.340µs          |
| benchFetchRecursiveFactoryAlias1 | 2.475µs           | 2.340µs          |
| benchBuildRecursiveFactoryAlias1 | 2.490µs           | 2.341µs          |
| benchFetchRecursiveFactoryAlias2 | 2.497µs           | 2.340µs          |
| benchBuildRecursiveFactoryAlias2 | 2.473µs           | 2.392µs          |
| benchFetchAbstractFactoryFoo     | 2.407µs           | 2.408µs          |
| benchBuildAbstractFactoryFoo     | 1.947µs           | 1.892µs          |
+----------------------------------+-------------------+------------------+

benchmark: HasBench
+-------------------------+-------------------+------------------+
| subject                 | suite:master:mean | suite:PR242:mean |
+-------------------------+-------------------+------------------+
| benchHasFactory1        | 0.526µs           | 0.546µs          |
| benchHasInvokable1      | 0.603µs           | 0.589µs          |
| benchHasService1        | 0.482µs           | 0.511µs          |
| benchHasAlias1          | 0.584µs           | 0.612µs          |
| benchHasRecursiveAlias1 | 0.605µs           | 0.633µs          |
| benchHasRecursiveAlias2 | 0.603µs           | 0.629µs          |
| benchHasAbstractFactory | 0.839µs           | 0.895µs          |
| benchHasNot             | 0.851µs           | 0.911µs          |
+-------------------------+-------------------+------------------+

benchmark: SetNewServicesBench
+------------------------------------+-------------------+------------------+
| subject                            | suite:master:mean | suite:PR242:mean |
+------------------------------------+-------------------+------------------+
| benchSetService                    | 2.027µs           | 2.047µs          |
| benchSetFactory                    | 4.350µs           | 4.285µs          |
| benchSetAlias                      | 11.946µs          | 11.693µs         |
| benchOverrideAlias                 | 36.493µs          | 36.031µs         |
| benchSetInvokableClass             | 5.359µs           | 1.976µs          |
| benchAddDelegator                  | 2.090µs           | 2.109µs          |
| benchAddInitializerByClassName     | 2.473µs           | 2.518µs          |
| benchAddInitializerByInstance      | 1.764µs           | 1.815µs          |
| benchAddAbstractFactoryByClassName | 3.488µs           | 3.544µs          |
| benchAddAbstractFactoryByInstance  | 3.118µs           | 3.116µs          |
+------------------------------------+-------------------+------------------+
fhein commented 6 years ago

The tests deleted checked the particular implementation of invokables (i.e. the transformation to alias and factory). Kind of white box testing. As the implementation changed, these tests got obsolete. To be complete, I could have introduced a white box test verifying that an invokable really gets stored in the invokables array. I believe, that would be overdone a bit.

weierophinney commented 6 years ago

@fhein I've merged locally. However, this patch is wildly different than what is on develop currently, with regards to invokables, which is making merging there quite difficult.

Should I leave the code on develop as-is, or attempt a merge?

fhein commented 6 years ago

Is there a way you could leave the pain up to me? I think, the bug fix is most important. I offer to take care of develop by rebasing if that's possible.

The main difference of this to develop is that alias handling was introduced to develop which is not contained in this PR.

But I will have a closer look on the diffs.

fhein commented 6 years ago

Puh. Aliases and setter optimization and minor adjustments.

Would a process like this work?

1, Merge this with master. 2, I'd create a PR based on master applying all differences of current develop. 3, Next? I am not gitalist enough to know. :(

weierophinney commented 6 years ago

Would a process like this work?

1, Merge this with master. 2, I'd create a PR based on master applying all differences of current develop. 3, Next? I am not gitalist enough to know. :(

No, this doesn't work. It creates a merge conflict when we're ready to merge develop back to master in order to prep the next minor release, which means having to resolve it all again anyways. (Learned this the hard way.)

Looks like there are other changes requested, so I'll wait for now.

fhein commented 6 years ago

Something more I should do?

fhein commented 6 years ago

I noticed that is quite a lot of work to rebase develop to master after #242 was applied to master.

With PR #251 I provide a branch which represents a rebase of current develop onto (current master with PR #242 applied). This can obviously not get automatically be merged with develop, but it delivers a reference, how develop should look like after integration of PR #242.

I hope this helps. If I can do more, please let me know.

fhein commented 6 years ago

This was replaced by #253. Please see documentation there.