Switching to non-static getServices method returning callables #20
Conversation
|
The only other change I might recommend is allowing factories to be any of:
This latter has some interesting benefits in terms of caching the results of service providers:
|
|
@weierophinney if we allow that then it opens the door to things like:
Also this PR is quite long to review already, maybe it would be better to have one change per PR?
@moufmouf I think this should be a separate PR too. And if the service provider is only a provider of a |
Disallow it within the specification. I.e., specifying a functor implies a constructor-less class, or a constructor that allows no arguments. Essentially, if a functor class has constructor arguments, you'd need to instantiate it when returning the list of factories: return [
Foo::class => new FooFactory($argument, $argument2),
];
For the reasons I specified:
This may seem like a micro-optimization, but we discovered quite the opposite with Zend Framework. You cannot serialize closures easily or in a performant fashion (even super-closure notes in its own README that "In general, however, serializing closures should probably be avoided."). Without the option of providing a functor class name, you're left with static methods as the only serializable option for factories returned by a service-provider. Sure, these could wrap the functor: public static function createFoo($container, callable $getPrevious = null)
{
return (new FooFactory())($container, $getPrevious);
}but this then leads to two function calls instead of one. Containers can store the string class name, and instantiate it only when required: if (! is_callable($this->factories[$name])) {
$factory = new $this->factories[$name];
if (! is_callable($factory)) {
throw new InvalidFactoryException();
}
$this->factories[$name] = $factory;
return $factory($this, $getPrevious);
}
$factory = $this->factories[$name];
return $factory($this, $getPrevious);We observed 10-15% improvements when using this approach, over an approach that required always providing a callable, and measurable memory reduction as well. |
|
That's really interesting. However:
And I think with that idea we are circling back to static methods, or at least to methods on the service provider which may or may not be static… So to me, if we want to optimize for performances here are the best options:
|
|
Hew @weierophinney Thanks a lot for the great feedback. If I understand correctly your suggestion, by adding fully qualified class name resolving to a functor as a possible return type (in addition to callables), you allow for an optimization, as the So: return [
Foo::class => FooFactory::class,
];is faster than: return [
Foo::class => new FooFactory(),
];(if I'm correct, we save the autoloading of class Now, I'm really wondering if we might actually save a fair amount of performance. Assuming you have an application with 50 service providers, each containing about 5 services. That's about 250 services. Let's assume all these services are declared as functor. In a "naive" implementation of service providers (like the one in Simplex), this means having 50 calls to But the truth is that if I want to optimize things, I'd prefer completely saving the call to So rather than calling the Now, your optimization still makes sense. If a service provider contains 5 services, and if I fetch only one, I'm creating 5 functors instances for nothing (see example below); return [
Foo::class => new FooFactory(),
Foo2::class => new Foo2Factory(),
Foo3::class => new Foo3Factory(),
Foo4::class => new Foo4Factory(),
Foo5::class => new Foo5Factory(),
];If I only make a call to In practice however, I feel that there is going to be a tight coupling between the entries of a same service provider. I mean: if I fetch a service from one provider, there is a high likelihood that I'll fetch other services from the same service provider. Here is a sample: a Doctrine DBAL service provider: https://github.com/xhuberty/dbal-universal-module/blob/dbal-service-provider-with-parameter-definition/src/DbalServiceProvider.php So in practice, I'm not sure that allowing functors FQCN as a possible return type would change much for performance. And this comes at the price of simplicity (having an array of callables as the return type of However, that doesn't mean we cannot find ways to optimize this. Here are a few ideas that pop out of my head:
class MyServiceLocator implements ServiceLocator {
public function getServices() {
return [
"my_alias" => new Alias('my_service');
];
}
}
interface CacheableFunctor {
public function getConstructorArguments() : array;
}
class Alias implements CacheableFunctor {
private $alias;
public function __construct(string $alias) {
$this->alias = $alias;
}
// Returns the list of constructor arguments we can use to create back this object
public function getConstructorArguments() {
return [ $this->alias ];
}
public function __invoke(ContainerInterface $container) {
return $container->get($this->alias);
}
}Containers interested in performance could "cache" the constructor arguments and reuse those to create the functor. Actually, thinking about it, it would maybe be easier to cache the whole functor (for instance using APCu). Also, another idea I like very much is the idea to have functors the implement interfaces that semantically mean something. Using my alias example above: interface AliasInterface {
public function getAlias() : string;
}
class Alias implements AliasInterface {
private $alias;
public function __construct(string $alias) {
$this->alias = $alias;
}
// Returns the list of constructor arguments we can use to create back this object
public function getAlias() {
return $this->alias;
}
public function __invoke(ContainerInterface $container) {
return $container->get($this->alias);
}
}Container aware of the As noted by @mnapoli however, we need to agree on a common set of interfaces for those functors and that might be quite hard to agree on (definition-interop might be a good starting point). The important part: all this can be done later, or in another project. Then, in another project, we could work on add-ons to Another example that came to my mind: What about a: interface LazyInterface {
public function isLazyService(): bool;
}
class LazyService implements LazyInterface {
private $callable;
public function __construct(callable $callable) {
$this->callable = $callable;
}
public function isLazyService() {
return true;
}
public function __invoke(ContainerInterface $container, $previous) {
return $this->callable($container, $previous);
}
}The Does it make sense? |
I never said they weren't a good approach. (The only negative thing I said about static methods was that using them as wrappers to instantiation and invocation of a functor is not optimal.)
You're extrapolating based on a false assumption, that functors will only define the One advantage of functors is that they allow us to extract methods in order to reduce complexity. As an example, zend-mvc-i18n defines a TranslatorFactory. The functor method is 4 lines long. One of those lines calls another method in the class, which has paths for invoking other methods. This approach makes the code easier to understand and easier to test. Yes, this can be done with static methods as well. I personally find these far harder to test, the code harder to read, and much harder to extend (LSB has to be planned for, and visibility can strongly affect it).
If you're willing to consider that, why not also FQCN functors? It poses the same complexity (if not more) for consuming containers. (Particularly your assertion that they could optimize by caching the first instance. Introspection of callables formed this way requires a fair bit of logic; we did this in
And I'm tending to agree, particularly with the write-up @moufmouf created with this PR, with regards to "minimal way to fetch the service". In many cases, if a container is available, a developer could simply invoke the method with the container to get the instance required, which is powerful. That said, I still have two reservations regarding the current syntax:
Regarding this latter point, consider classes that can be directly instantiated without any arguments. In zend-servicemanager, we provide an Foo::class => [InvokableFactory::class, 'createService']The current spec does not allow for this, in two different ways. First, it limits to factories in the class itself. This means that you would need to wrap the above: public static function getServices()
{
return [
Foo::class => 'createService',
];
}
public static function createService(ContainerInterface $container, callable $getPrevious = null)
{
return InvokableFactory::createService($container, $getPrevious);
}Again, as noted in my previous discussions around functors, this is suboptimal, as it doubles the number of function calls required in order to create an instance. The more subtle issue, though, is that the service name is not passed to the factory. That means that the above factory method cannot be re-used for multiple services. As an example: public static function getServices()
{
return [
Foo::class => 'createService',
Bar::class => 'createService',
];
}
public static function createService(ContainerInterface $container, callable $getPrevious = null)
{
// What will this create?
return InvokableFactory::createService($container, $getPrevious);
}In zend-servicemanager, we are currently passing the requested service name to the factory, which allows re-use. If multiple services have the same instantiation pattern, we can define one factory, but map it to multiple services that return different instances. This is powerful for re-use. It also slightly complicates the signature, and modifies that "minimal" use case. This would likely result in one of the following factory signatures: function (ContainerInterface $container, $name = null, callable $getPrevious = null);
function (ContainerInterface $container, callable $getPrevious = null, $name = null);Considering that the callable for tl;dr: I am buying into the arguments for using static methods as the preferred and/or only way of defining factories, but would like to be able to have re-usable factories, which would (a) potentially expand what's allowed as a factory (i.e., allow any static method, using |
|
That's all really interesting.
Those where the benefits I listed of using methods over functors:
But I take your point though:
I'm not opposed to functors, it just looks heavier to implement (you have to write one class per service your module defines, which can be a bit of a burden).
As of right now, static or not static, we could achieve that by passing the requested entry name. Then you can That's not really ideal though (reuse through inheritance). But is it a problem? I'm just suggesting the idea, I don't have a strong opinion right now. Else, if we want to avoid non-cacheable/serializable/compilable stuff like closures, we could allow And with that you could do what you suggested: Foo::class => [InvokableFactory::class, 'createService']You could even be compatible with invokable classes: Foo::class => [InvokableFactory::class, '__invoke']Anyway what I'm arguing for is to restrict what we allow to:
I'm fine if it's not static methods :) |
Not necessarily; as I noted elsewhere in my comment, you can, in zend-servicemanager, re-use these factories for multiple services. We even cache the factory instances, so that retrieval of services later can re-use them. (Pimple 3 has support for re-usable factories, with caching of the factory on first use, too.) That said, we can stop arguing around functors. I think the suggestion that we limit to One note:
The above wouldn't work, as
Agreed, as noted. To summarize the changes needed to the current specification:
|
I'd really argue in the opposite direction. First, about simplicity: For container implementations, simplicity is not an issue. Yes, supporting Furthermore, simplicity is really important for people implementing the Also, having the ability to pass an argument in the constructor reduces the need for a third Regarding optimizations: I don't think we should impose optimizations in the standard. As @weierophinney noted, this can end-up in a counter-productive code. You cannot force a developer to write optimized code. Some people will write service providers that suck. The important part is that we should communicate on the best practices to respect to optimize service providers. Those best practices could eventually change as containers get more clever. Finally, I feel that static methods are a dead end regarding performances. We think we are improving performance but we are actually doing the opposite. @webmozart first spotted this when looking at the interface. He asked us: do we have the equivalent to Symfony tags (i.e. can we create easily a list of services?) Our answer was that we could "extend" a service that would actually be an array of services. Each service provide could add a service to this list of services. This is a fairly common use-case. We might want to create a list of log handlers, a list of routers, a list of controllers, a list of actions, a list of twig helpers, a list of commands for a Symfony console, etc... A typical service provider would look like this: class MyControllerServiceProvider extends ServiceProvider {
public function getServices() {
return [
MyController::class => [ self::class, 'getMyController' ],
"controllers" => [ self::class, 'addMyController' ]
];
}
public static function getMyController() {
return new MyController();
}
// This very generic piece of code will be repeated ad-nauseam in every service provider that must add a service to an array.
public static function addMyController(ContainerInterface $container, callable $getPrevious = null) {
// Let's check if a previous value exists. If yes, let's resove it.
$previous = ($getPrevious === null) ? [] : $getPrevious();
$previous[] = $container->get(MyController::class);
return $previous;
}
}Now, let's assume we have 20 controllers in the array When my MVC framework will fetch the array
This is the most minimal thing we can do to instantiate this Let's compare this to what could happen with a functor implementing a specially crafted interface A typical service provider would look like this: class MyControllerServiceProvider extends ServiceProvider {
public function getServices() {
return [
MyController::class => [ self::class, 'getMyController' ],
"controllers" => new AddToArray(MyController::class)
];
}
public static function getMyController() {
return new MyController();
}
}of course, there is an additional interface AddToArrayInterface {
public function getAddedService();
}
class AddToArray implements AddToArrayInterface {
private $serviceName;
public function __construct(string $serviceName) {
$this->serviceName = $serviceName;
}
public function getAddedService() {
return $this->serviceName;
}
public function __invoke(ContainerInterface $container, callable $getPrevious = null) {
// Let's check if a previous value exists. If yes, let's resove it.
$previous = ($getPrevious === null) ? [] : $getPrevious();
$previous[] = $container->get($this->serviceName);
return $previous;
}
}Performance-wise: For a non optimized container: When my MVC framework will fetch the array
So yes, this clearly requires more work than with the static methods. But let's have a look at the optimizations we can apply. For an optimized container: A clever cached/compiled container could:
In the end, compiled code could look like this: And this is as optimized as you can ever get:
You cannot do any better, and this is way way faster than what you can get with static methods. I know there are a lot of assumptions here (like the fact that we can agree on a common ... plus there are added benefits: service providers are easier to read, there is less duplicated code... I also know this is harder to implement for container developers, but to me, this is clearly a non-issue because:
Finally, there may be completely different reasons to prefer having callables to public static methods. Apart for all the reasons raised by @dragoonis and @Giuseppe-Mazzapica in #5, one issue that comes to my mind was raised by @pmjones. In a mail, he said:
I don't want to speak instead of Paul but if I'm understanding is thoughts correctly, I'm sure he would prefer something like: class MyControllerServiceProvider extends ServiceProvider {
public function getServices() {
return [
MyController::class => function getMyController() {
return new MyController();
}
];
}
}because in the code above, a beginner developer cannot say: "Hey, look, it's easy to get an instance of the controller, just use @weierophinney @mnapoli I know you are reluctant to this idea. I was too when we first started working on service providers. @stof convinced me to give it a try and see how it goes. After testing it, I'm convinced it has far more potential than going "static methods only". I'd really like to invite you to give it a try, just like I did, and see how it feels. I'm not saying it's flawless, but I think it has a greater potential in the end. Anyway, I'm keen to have your thoughts on this! |
|
@moufmouf this is starting to convince me. And I have exactly these kind of objects in PHP-DI so it would be really easy to integrate. I see the following issues:
And just because I have to:
I disagree a lot with the fact that it's a problem. Many people said the same about |
Allowing callables means allowing the
totally agreed about not needing an interface. These should be simple value objects. And if a container does not support one of them, it will just treat it as a callable thanks to these value objects being invokable, which is the beauty of this proposal. The PSR standard does not even need to be the one shipping these value objects. |
I completely agree that the
What about offering both? Containers should rely on the interface, in case someone wants to write an alternative implementation, and the base package could still offer a default implementation, next to the interface. Note that I'm not opposed to the idea of only providing an implementation and no interface. This has a nice benefit: one of the shortcomings of implementing funktors with interface is that implementations must be "coherent". For instance, if the So I have really no strong opinion on this.
In my experience, implementing closures in containers is the most difficult task. A solution with only |
|
Few unordered points:
|
|
👍 thanks for both of your answers. Regarding allowing
Of course but containers care about the implementation of such callables. Closures and objects are a pain to deal with for serialized/compiled/cached containers. Let's summarize:
Do you agree with that table? Then we can go from there and see if we want to eliminate stuff that is incompatible with some containers? |
|
@Giuseppe-Mazzapica |
|
@mnapoli not an expert of compiled containers, but can't the name of service retrieved by |
|
Adding a service to an array means for the current entry (so we know the entry name) which is an array we add a new item to the array which is another service (which is why we need to know the service name). In pseudo-code: [
'event_subscribers' => [
$container->get('foo')
]
]So we need to know that we are adding |
|
@mnapoli I do agree with your table (although I would not put "++" everywhere for classic container since they have worse performance than compiled/cached containers). However, I think your table is not showing the relationship between the options. If you think about it: If your container supports optimisable entries ( Once you have support for "invokable objects" in your container, you are only one step away from having support to closures (they are really the same kind of beast). So optimisable entries =(implies support for)=> invokable objects =(almost equivalent to)=> closures. |
|
I was referring to this code posted by @moufmouf class MyControllerServiceProvider extends ServiceProvider {
public function getServices() {
return [
MyController::class => [ self::class, 'getMyController' ],
"controllers" => new AddToArray(MyController::class)
];
}
public static function getMyController() {
return new MyController();
}
}and I thought that both But as I said, I'm not an expert on the field, and is possible i'm saying something terribly wrong. |
|
I think I understand the point... when you compile statically you use reflections (I guess) which can't give you the service name... |
|
@Giuseppe-Mazzapica I'm not sure I understand your thoughts but when you say:
we definitely don't want to do a statistical analysis of What I'm suggesting here is that compiled/cached containers could search all callables implementing the AddToArray instance. From those services, we can build (using the |
|
Is there any reason not to rely on the Delegate lookup feature? class MyControllerServiceProvider implements \Interop\Container\ContainerInterface
{
protected $container;
public function __construct(\Interop\Container\ContainerInterface $container)
{
$this->container = $container;
}
public function getServices()
{
return [
'MyService1',
'MyService2'
];
}
public function has($id)
{
// ...
}
public function get($id)
{
// ...
}
} |
|
Hey Stephan, Actually, we've been thinking about this. I think that was also suggested by @pmjones, if I recall correctly. The shortcoming with this approach is the way we can deal with "extended" services (i.e. how we do to extend a service declared in another container/service-provider). Suddenly, that means we should change the Also, in the current proposal, a service-provider creates the instances but does not need to ensure their unicity (this is the role of the container). That being said, when we first started thinking about service providers, I tried making them extend |
|
I think we somehow need a solution for passing the parent container (the one used by the application) to the service providers because the instances configured by the service providers should be able to access to "global" dependency scope. Only that way I am able to pass instances (e.g. a configured Doctrine instance) to one of the classes built by the service provider. Relying on a ContainerInterface instance might be sufficient, given the parent container is aware of the delegate lookup feature. I thought that as the only reason we came up with idea of the delegate lookup :) - Somehow the "configuration code" needs to be merged with the configuration of the application. Returning concrete, configured instances does not seems to solve the problem (for me). |
|
Hey Stephan, In this PR, the expected format for a factory method is: function(ContainerInterface $container, callable $getPrevious = null) {
// Some code that returns the service/entry
}This might not be clear, but I always assumed that Does it solve your problem? Should we maybe open a new PR to make it clear that |
|
To be fair it is not easy following the discussion here, the code plus what is is happening in gitter chat. I was just referring to the code samples I saw above and was not sure what you guys where thinking ;) |
|
Hey @mnapoli , Hope the holidays were cool. |
|
@moufmouf I'm confused though, I think what came up in the latest discussions on Gitter is that allowing any kind of callable will be too complex. Functors are a solution but:
In any case I feel this PR changes too many things at once. There's one detail that we can merge directly: removing Quotes from the Gitter discussion I'm re-reading: Argument against
https://gitter.im/container-interop/definition-interop?at=5717ba7a3307b26736e326ef Argument against factories as functors:
https://gitter.im/container-interop/definition-interop?at=5717bb2e98c544f1396cf3ca
https://gitter.im/container-interop/definition-interop?at=5717b979599a529856d9983b |
|
Hey Matthieu,
I can definitely split this PR in 2. However, I have a hard time figuring out the interest of having a
We speak a lot about complexity, but that's really hardly complex. I wanted to test that complexity, so I wrote a bunch of tests. It's really not that hard to keep some code optimized.
In the worst case scenario, it takes ~10 lines of code to handle. So the argument of complexity does not stand the tests.
Regarding this statement, see my argument above: #20 (comment)
Regarding caching, this is a non-issue. See my test in PHP-DI: if the factory is a public static method, it is easily cachable. If it is not, we can still cache the index of the service provider and the name of the service (as shown here: https://github.com/moufmouf/PHP-DI/blob/callables/src/DI/Definition/InteropDefinition.php). This is a great way to not put the service provider in the cache (obviously bad), and still get some caching benefits. Regarding performances: Of course, with non static factories, we need to instantiate the service provider and call the So, shall I first split the PR in 2 to make it more readable? |
|
OK I want this to move forward too. Let's merge it and switch to allowing For the record I disagree with a few things you said, e.g. what I meant about simplicity wasn't about the implementation, but rather about the standard itself (users, module developers understanding it, etc.). The less options we have, the simpler it is. Now users have plenty of options and we have to feed them best practices/a manual: we wouldn't have to if it was really "simple". And yes in PHP-DI I can cache static calls, but the rest I can't, so it doesn't matter if users don't use that. I don't call "caching the entry key" as caching, it's just not cacheable like static calls, but I can live with that if users follow "best practices". Anyway let's try it out. I'll be opening further pull requests for discussions afterwards, e.g. one to remove completely the interface and fall back to |
|
|
||
| By contrast, cached/compiled container will avoid calling `getServices` on each call by caching the results or dumping a PHP file containing the container. Therefore, service provider writers should not rely on the `getServices` method being always called before the factories. | ||
|
|
||
| A good approximation is to say that service providers SHOULD be immutable. Once initialized, the state of the service provider should not change. |
There was a problem hiding this comment.
I think all that section can be removed, the README is already super long I'd rather keep it as simple as we can. In the end the information here is:
service provider writers should not rely on the
getServicesmethod being always called before the factories.
And I don't think it's crucial, if it affected anyone it means there are doing something really wrong.
There was a problem hiding this comment.
Ok, I'll remove both "Best practices > Playing nice with all containers" and "Best practices > Performance considerations". I completely agree they don't belong this README. They should be put in a "best practices guide" or whatever we call that.
|
Done! Took your comments into account. I completely agree we needed to shorten the README. Most of its content needs to be put in a best practice guide and we can discuss that later anyway. If you have no further comment, we can merge this and go for the next round of PR/issues :) |
|
Let's go! |
|
@mnapoli Is it fine with you if we tag a v0.3.0 based on this? |
|
@moufmouf Yes go ahead. |
Hey,
Following discussion on #5 and #18, here is a proposal for a new version of the
ServiceProviderinterface with:getServicesmethodgetServicesmethod that returns an array of callablesI'm having a hard time wondering if we should not rename the
getServicesmethod (as it actually returns an array of factories).Maybe to something like
getFactoriesorserviceFactories?Also, I've added a fair amount of text. Not sure everything belongs to
README.md. Comments are welcome!