This repository has been archived by the owner on Feb 6, 2020. It is now read-only.
Performance - Abstract factory request cache #226
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added nightly test
removed duplicate allow_failures
Updated packages in composer to latest stable version and PHP requirements changed to PHP ^7.1 Updated suggestion in composer.
These methods are not static.
Drop PHP 5.6 and 7.0
Those optimizations target improving OPCODES.
…nternal-functions Import PHP internal functions
Optimize PHP function calls
replace count() usage to empty() when possible
added 3 commits
January 12, 2018 17:06
will be called only once on consecutive requests for the same service. This feature can be turned on and off via config or at anytime during the life time of the service manager. If turned off the cache gets unset.
|
Reconsidered this one. The idea is good. The implementation is bullshit. We already have a 'hasCache'. It's called $this->factories. On the other side, the hasNotCache applies to all unknown services, not only for services, which abstract factories do not know about. Changed that to cache abstractfactories as factories and unknownServices caching now covers all requests. |
added 12 commits
January 13, 2018 00:03
addInitializer, addDelegator, mapLazyService. Configure: replaced (isset($config['xyz']) guards with ! empty($config['xyz']) guards. This has no negative performance impact, but is more precisely what we want to know. Added BenchAsset\DelegatorFactoryFoo for performance tests. Added some tests to setNewServicesBench.php.
Reintroduced reference to Tarjans Algorithm again. Added simple test for failing behaviour. Error was detected while adding tests to increase code coverage. One of the parts not covered was bad.
will be called only once on consecutive requests for the same service. This feature can be turned on and off via config or at anytime during the life time of the service manager. If turned off the cache gets unset.
…ithub.com/fhein/zend-servicemanager into PERFORMANCE-AbstractFactoryRequestCaching
|
This patch looks like a bad idea to me, since we didn't specify anywhere
that abstract factories have to be idempotent. That can be rectified, but
it is a BC break.
…On 12 Jan 2018 18:07, "Frank Hein" ***@***.***> wrote:
This patch provides caching for abstract factory requests, so canCreate
will be called only once on consecutive requests for the same service.
This feature can be turned on and off via config or anytime during the
life time of the service manager. If turned off the cache gets unset.
Recommended implementations of abstract factories should have an invariant
canCreate method. Same request, same answer the life time through. But if
users want to employ abstract factories where canCreate answers can change
during life time, this feature has to be turned off.
There is currently no benchmark in place which provides meaningful
information about the real life performance gain. Such a benchmark should
assume some cost for canCreate. I will prepare.
What you can see using the current FetchNewServiceViaConfigAbstra
ctFactoryBench is the small amout of overhead this feature introduces for
cache administration.
------------------------------
You can view, comment on, or merge this pull request online at:
#226
Commit Summary
- 3.3.0 readiness
- Bumped version
- spelling fixes
- Merge branch 'hotfix/delegator-invokable-example' into develop
- drop hhvm
- Merge branch 'feature/193' into develop
- updated travis build matrix lowest/locked/latest
- Merge branch 'feature/spelling' of https://github.com/ka7/zend-
servicemanager into feature/189
- Merge branch 'feature/189' into develop
- Update .travis.yml
- Dropped PHP 5.6 and PHP 7.0
- Updated travis configuration to be consistent with other packaes
- Changed self:: to $this-> for expectException* methods
- Use self:: instead of $this-> for assert* methods
- Removed unused variables
- Removed redundant parentheses
- Removed unused import
- Updated CONTRIBUTING.md
- Merge pull request #197 from webimpress/drop-php-5.6-and-7
- Optimize PHP function calls
- Optimize more PHP function calls and issets
- Import PHP internal functions
- Alphabetical order of use statements
- Merge pull request #198 from webimpress/feature/import-
internal-functions
- Merge pull request #196 from smuggli/optimize_opcodes
- replace count() usage to empty when possible
- replace empty() with !
- Merge pull request #199 from samsonasik/count-improve
- Merge branch 'hotfix/206' into develop
- Merge branch 'hotfix/200' into develop
- Merge branch 'hotfix/qa-tools' into develop
- Merge branch 'hotfix/203' into develop
- Merge branch 'hotfix/204' into develop
- 3.3.1 readiness
- Merge branch 'hotfix/208' into develop
- Updated Travis CI configuration
- Merge pull request #209 from webimpress/hotfix/travis
- Merge branch 'feature/209' into develop
- Remove PHP < 7 fix
- Merge pull request #219 from smuggli/remove_legacy_code
- Added a benchmark for ::has()
- Merge branch 'feature/#217-add-benchmark-for-servicemanager-has'
into develop
- Removed $found variable. Reordered service resolution, so that
aliases
- Introduced $resolvedName to avoid overriding an exsiting variable.
- #216 checking that the `AbstractFactory` is called with a resolved
alias name, and not the alias itself
- Revert "Introduced $resolvedName to avoid overriding an exsiting
variable."
- Revert "Removed $found variable. Reordered service resolution, so
that aliases"
- Merge branch 'feature/#216-enhancement-speed-up-servicemanager-has'
into develop
- Updated to php-coveralls/php-coveralls
- Merge branch 'hotfix/#211-php-coveralls' into develop
- Added a benchmark for setService.
- Speed up setAlias.
- Removed debug out. Puh.
- CS tweaks.
- Forgot to store the original param to aliases array. Necessary at
all?
- Polish.
- Started optimizing abstractFactories.
- Added todos.
- CS fixes, as usual.
- Compacted and added cycle visualization.
- Removed redundant $resolvedAliases array. Aliases are resolved when
- Added source code documentation to doSetAlias to explain alias
- Removed @todos.
- Merged the performance optimization of has (PR #220) because of the
- Fix bc break:
- Renamed validate members to validateServiceName.
- Corrected overall formatting of sprintfs. Reviewed an fixed all
imports
- Added optimized algorithm for batch addition of new aliases via
- Fixed bug in abstract factory mock in
- Finalized mapAliasToTarget and mapAliasesToTargets.
- Removed obsolete test setup items.
- Optimized performance for setInvokableClass and handling of
invokables
- Reintroduced $this->configure. Assumed, configure may be called once
- Fixed introduced bc break by adding Named Constructor for
CMNAException.
- Fixed my fix.
- Removed reference to Tarjan. Added explanation why there are
different
- CS tweaks.
- Unified redundant exception messages for invalid initializers and
- Introduced caches to speedup service name resolution via abstract
- This patch provides caching for abstract factory requests, so
canCreate
File Changes
- *M* .travis.yml
<https://github.com/zendframework/zend-servicemanager/pull/226/files#diff-0>
(33)
- *M* CHANGELOG.md
<https://github.com/zendframework/zend-servicemanager/pull/226/files#diff-1>
(26)
- *M* benchmarks/BenchAsset/AbstractFactoryFoo.php
<https://github.com/zendframework/zend-servicemanager/pull/226/files#diff-2>
(2)
- *M* benchmarks/BenchAsset/FactoryFoo.php
<https://github.com/zendframework/zend-servicemanager/pull/226/files#diff-3>
(2)
- *A* benchmarks/HasBench.php
<https://github.com/zendframework/zend-servicemanager/pull/226/files#diff-4>
(113)
- *M* benchmarks/SetNewServicesBench.php
<https://github.com/zendframework/zend-servicemanager/pull/226/files#diff-5>
(8)
- *M* composer.json
<https://github.com/zendframework/zend-servicemanager/pull/226/files#diff-6>
(15)
- *M* composer.lock
<https://github.com/zendframework/zend-servicemanager/pull/226/files#diff-7>
(4)
- *M* docs/book/configuring-the-service-manager.md
<https://github.com/zendframework/zend-servicemanager/pull/226/files#diff-8>
(4)
- *M* docs/book/cookbook/factories-vs-abstract-factories.md
<https://github.com/zendframework/zend-servicemanager/pull/226/files#diff-9>
(2)
- *M* src/AbstractFactory/ConfigAbstractFactory.php
<https://github.com/zendframework/zend-servicemanager/pull/226/files#diff-10>
(26)
- *M* src/AbstractFactory/ReflectionBasedAbstractFactory.php
<https://github.com/zendframework/zend-servicemanager/pull/226/files#diff-11>
(6)
- *M* src/AbstractPluginManager.php
<https://github.com/zendframework/zend-servicemanager/pull/226/files#diff-12>
(17)
- *M* src/Config.php
<https://github.com/zendframework/zend-servicemanager/pull/226/files#diff-13>
(13)
- *M* src/Exception/ContainerModificationsNotAllowedException.php
<https://github.com/zendframework/zend-servicemanager/pull/226/files#diff-14>
(9)
- *M* src/Exception/CyclicAliasException.php
<https://github.com/zendframework/zend-servicemanager/pull/226/files#diff-15>
(32)
- *M* src/Exception/InvalidArgumentException.php
<https://github.com/zendframework/zend-servicemanager/pull/226/files#diff-16>
(22)
- *M* src/Proxy/LazyServiceFactory.php
<https://github.com/zendframework/zend-servicemanager/pull/226/files#diff-17>
(2)
- *M* src/ServiceLocatorInterface.php
<https://github.com/zendframework/zend-servicemanager/pull/226/files#diff-18>
(4)
- *M* src/ServiceManager.php
<https://github.com/zendframework/zend-servicemanager/pull/226/files#diff-19>
(649)
- *M* src/Test/CommonPluginManagerTrait.php
<https://github.com/zendframework/zend-servicemanager/pull/226/files#diff-20>
(3)
- *M* src/Tool/ConfigDumper.php
<https://github.com/zendframework/zend-servicemanager/pull/226/files#diff-21>
(16)
- *M* src/Tool/ConfigDumperCommand.php
<https://github.com/zendframework/zend-servicemanager/pull/226/files#diff-22>
(15)
- *M* src/Tool/FactoryCreator.php
<https://github.com/zendframework/zend-servicemanager/pull/226/files#diff-23>
(11)
- *M* src/Tool/FactoryCreatorCommand.php
<https://github.com/zendframework/zend-servicemanager/pull/226/files#diff-24>
(8)
- *M* test/AbstractFactory/ConfigAbstractFactoryTest.php
<https://github.com/zendframework/zend-servicemanager/pull/226/files#diff-25>
(28)
- *M* test/AbstractFactory/ReflectionBasedAbstractFactoryTest.php
<https://github.com/zendframework/zend-servicemanager/pull/226/files#diff-26>
(36)
- *M* test/AbstractPluginManagerTest.php
<https://github.com/zendframework/zend-servicemanager/pull/226/files#diff-27>
(62)
- *M* test/CommonServiceLocatorBehaviorsTrait.php
<https://github.com/zendframework/zend-servicemanager/pull/226/files#diff-28>
(145)
- *M* test/ConfigTest.php
<https://github.com/zendframework/zend-servicemanager/pull/226/files#diff-29>
(6)
- *M* test/Exception/CyclicAliasExceptionTest.php
<https://github.com/zendframework/zend-servicemanager/pull/226/files#diff-30>
(95)
- *M* test/Factory/InvokableFactoryTest.php
<https://github.com/zendframework/zend-servicemanager/pull/226/files#diff-31>
(4)
- *M* test/LazyServiceIntegrationTest.php
<https://github.com/zendframework/zend-servicemanager/pull/226/files#diff-32>
(45)
- *M* test/Proxy/LazyServiceFactoryTest.php
<https://github.com/zendframework/zend-servicemanager/pull/226/files#diff-33>
(12)
- *M* test/ServiceManagerTest.php
<https://github.com/zendframework/zend-servicemanager/pull/226/files#diff-34>
(67)
- *M* test/TestAsset/FailingExceptionWithStringAsCodeFactory.php
<https://github.com/zendframework/zend-servicemanager/pull/226/files#diff-35>
(2)
- *M* test/TestAsset/V2ValidationPluginManager.php
<https://github.com/zendframework/zend-servicemanager/pull/226/files#diff-36>
(4)
- *M* test/TestAsset/V2v3PluginManager.php
<https://github.com/zendframework/zend-servicemanager/pull/226/files#diff-37>
(3)
- *M* test/Tool/ConfigDumperCommandTest.php
<https://github.com/zendframework/zend-servicemanager/pull/226/files#diff-38>
(75)
- *M* test/Tool/ConfigDumperTest.php
<https://github.com/zendframework/zend-servicemanager/pull/226/files#diff-39>
(47)
- *M* test/Tool/FactoryCreatorCommandTest.php
<https://github.com/zendframework/zend-servicemanager/pull/226/files#diff-40>
(13)
- *M* test/Tool/FactoryCreatorTest.php
<https://github.com/zendframework/zend-servicemanager/pull/226/files#diff-41>
(2)
Patch Links:
- https://github.com/zendframework/zend-servicemanager/pull/226.patch
- https://github.com/zendframework/zend-servicemanager/pull/226.diff
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#226>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAJakHSRH51NXZpaWGyIFtd8TdUOObYuks5tJ45TgaJpZM4RckHY>
.
|
|
Abstract factories wouldn't have to be idempotent. Idempotency is a funky word. As I see it here, the service manager would get idempotent in a sense that a request which can get resolved to an abstract factory would be resolved to the same abstract factory when the same name gets requested again. If the abstract factories are designed to produce the same service for a particular name requested (i.e, are idempotent) this feature would have significantly positive performance impact. I checked several real life scenarios. Most of them use this 'pattern'. I'd not recommend to use abstract factories which change behaviour at runtime at all. Problems which may arise can be quite difficult to track down. On the other hand, the service manager explicitly supports (there is a test for it) to register the same abstract factory twice. I do not completely understand how it would be accomplished to let the the first reference's canCreate return false and the second return true. But that this feature is supported and tested suggests somehow, that there are use cases for abstract factories, which are not idempotent. I find it difficult to imagine what these use cases are. I'd vote for integration of the abstract factory cache and an option to turn it off for bc and for particular use cases relying on non-idempotent abstract factories. |
|
I leave this open for discussion. Implementation is outdated. Too much things to manually merge. If the feature can be agreed on, I'll reimplement. |
|
Agree with "wouldn't recommend" on mutable factory behaviour, bit that's
still a change in how the system behaves, and someone out there will have
to adapt their code to this patch, which means BC break.
…On 16 Jan 2018 20:30, "Frank Hein" ***@***.***> wrote:
Abstract factories wouldn't have to be idempotent. Idempotency is a funky
word. As I see it here, the service manager would get idempotent in a sense
that a request which can get resolved to an abstract factory would be
resolved to the same abstract factory when the same name gets requested
again.
If the abstract factories are designed to produce the same service for a
particular name requested (i.e, are idempotent) this feature would have
significantly positive performance impact. I checked several real life
scenarios. Most of them use this 'pattern'.
I'd not recommend to use abstract factories which change behaviour at
runtime at all. Problems which may arise can be quite difficult to track
down.
On the other hand, the service manager explicitly supports (there is a
test for it) to register the same abstract factory twice. I do not
completely understand how it would be accomplished to let the the first
reference's canCreate return false and the second return true. But that
this feature is supported and tested suggests somehow, that there are use
cases for abstract factories, which are not idempotent. I find it difficult
to imagine what these use cases are.
I'd vote for integration of the abstract factory cache and an option to
turn it off for bc and for particular use cases relying on non-idempotent
abstract factories.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#226 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAJakCyBKftQ8FeFNFxYK85fYhwXd2WFks5tLPjngaJpZM4RckHY>
.
|
|
Not if configurable and default is bc. |
Let's just introduce a BC break, document it and delay for |
|
Do you think so? 'enableAbstractFactoryCache' => true, if people want it, does not seem to be to overcomplicate to me. (?) We could get some performance measures from practical use to justify further support of that feature. |
|
@fhein it is kinda problematic to introduce a flag that just serves as a filler until the next release is out. |
|
If we can agree on the feature and it would get default on without configuration option (abstract factory idempotence), then the flag would not harm if present in implementation. We could issue a deprecation note stating that the flag is ignored. |
|
This feature will speed up non-shared service creation only, of course, because shared services produced by an abstract factory are cached already. So it may be not on the 'hot path'. I think, not-so-hot pathes should be empowered as well as long as hot pathes don't suffer from doing so. |
|
@Ocramius: Service Manager caches abstract factories and other factories. So this feature is not a bc break. |
|
This one is obsolete. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Work in Progress ...
This patch provides caching for abstract factory requests, so canCreate will be called only once on consecutive requests for the same service.
This feature can be turned on and off via config or anytime during the life time of the service manager. If turned off the cache gets unset.
Recommended implementations of abstract factories should have an invariant canCreate method. Same request, same answer the life time through. But if users want to employ abstract factories where canCreate answers can change during life time, this feature has to be turned off.
There is currently no benchmark in place which provides meaningful information about the real life performance gain. Such a benchmark should assume some cost for canCreate. I will prepare.
What you can see using the current
FetchNewServiceViaConfigAbstractFactoryBenchis the small amout of overhead this feature introduces for cache administration.