Problem/Motivation
Currently, if a service A depends on another service B, then B needs to be instantiated at the same time or before the instantiation of A.
This happens even if A might never actually use B during the script lifetime.
The alternative is to have A depend on the container instead of B, so A can get B from the container at the time it is needed, and not before.
The downside: This makes A less predictable, because it now has access to *all the stuff in the container*, not just B.
One example is the cron service ... its loaded on every request,
as it is a dependency on system.automatic_cron, but the actual logic is nearly never needed.
Proposed resolution
For simple services (not using a factory) introduce a flag: lazy: true
Once this flag is set, there will be an automatic wrapper written into the dumped container. This wrapper wraps every public method,
initializes the underlying service, and calls its methods.
This is the same approach as Symfony documents, except instead of using the Ocramius ProxyManager library that Symfony recommends, we use a custom lightweight implementation, which is faster (e.g., calls the proxied object's methods directly rather than via reflection) but not as complete (e.g., doesn't proxy property access, only method calls, and doesn't proxy magic methods). We think that's a worthwhile trade-off, since Drupal services don't use public properties, magic methods, etc.: the whole point of a service is to be defined by an interface.
Remaining tasks
User interface changes
API changes
Original report by @donquixote
Suggested solution:
A "LazyService" wrapper, that gives A access to exactly one service from the container, but not more.
The wrapper itself has access to the container, but it will only ever use it for one specific key.
I am going to upload an implementation with some added magic.
The idea is that
- the wrapper can be used in place of the real service.
- the first operation on the wrapper will trigger instantiation of the real service.
- the reference to the wrapper will be replaced with the real service, once instantiated.
- any further operations can happen directly on the real service, without any indirection overhead.
- nothing (except the wrapper) has a reference to the container.
Usage:
$builder = new LazyService(Drupal::getContainer(), 'breadcrumb');
// This will tell the wrapper to replace $builder with the real service, once instantiated.
$builder->bindReference($builder);
assert('Drupal\Component\Container\LazyService' === get_class($builder));
// This will trigger the lazy instantiation via magic __call().
$breadcrumb = $builder->build(...);
assert('Drupal\Core\Breadcrumb\BreadcrumbManager' === get_class($builder));
// Additional calls happen directly on the service, without indirection.
$breadcrumb2 = $builder->build(...);
Comment | File | Size | Author |
---|---|---|---|
#117 | interdiff.txt | 3.05 KB | dawehner |
#117 | 1973618-117.patch | 27.89 KB | dawehner |
Comments
Comment #1
donquixote CreditAttribution: donquixote commentedThe patch introduces a LazyService class, and a very rough LazyServiceTest with MockContainer and MockService.
Unit tests don't work for me atm, so I cannot run this one either.
What is not included:
There is no mechanic in the DIC yet that would actually inject those wrappers into any service.
Comment #3
donquixote CreditAttribution: donquixote commentedFixing the test
Comment #3.0
donquixote CreditAttribution: donquixote commentedsimplify "A depends on B"
Comment #4
Crell CreditAttribution: Crell commentedThere has been a ton of discussion around this topic over in Symfony. The buzzword there I think is "proxies". I'm not against this in concept, but since it's generically useful we should do this work upstream in Symfony, not here. Lukas Smith is probably the person to talk to first.
(Note that Symfony 2.3 is going into feature freeze any day now, but as long as there's no BC break something like this could be accepted for 2.4.)
Comment #5
donquixote CreditAttribution: donquixote commentedOh, good idea.
Could you have a quick look at the patch, so we can go to Symfony with something "mature" ?
EDIT:
I also mentioned this once in DC Munich, maybe you remember :)
EDIT:
Teaser link: https://groups.google.com/forum/#!topic/symfony-devs/v66Nd5D015w
Comment #6
Crell CreditAttribution: Crell commentedThere's already some work going on here: https://github.com/symfony/symfony/pull/7527
Probably best to review that work and jump over there as needed.
Comment #7
yched CreditAttribution: yched commentedI guess @Crell is talking about this PR: https://github.com/symfony/symfony/pull/7527Also found: http://ocramius.github.io/blog/zf2-and-symfony-service-proxies-with-doct...
[edit: crosspost :-) ]
Comment #8
donquixote CreditAttribution: donquixote commentedThanks for the link.
I am still looking for a good place to engage there and will do so soon.
(and still trying to figure out how exactly the suggested system is going to work)
Some major differences I see, from my current level of understanding.
ProxyManager:
$container->getDefinitions('my_super_slow_class')->setLazy(true);
tells the DIC to always return a proxy instead of the real thing.LazyService from #3:
E.g. if the consumer does
$this->someLazyService->foo()
ten times, then only the first of these will have the indirection overhead. The other nine will work directly on the real thing.At the same time, another service Y might want the real Z, and not a proxy.
Comment #9
donquixote CreditAttribution: donquixote commentedTo be continued over here:
https://github.com/Ocramius/ProxyManager/issues/32
Comment #10
donquixote CreditAttribution: donquixote commentedConclusion:
(from the linked issue and some nice IRC talk with @ocramius)
- My observations and blind assumptions about ProxyManager (#8) were all correct -> yay!
- The LazyService implementation from #3 has some minor performance wins, BUT this comes at a *terrible* price:
- It bites with the LSP, because the "proxy" is not the same type (interfaces) as the real thing. (knew that, but didn't care)
- It "breaks identity", by replacing the reference. (knew that, but didn't care)
-> This makes it ugly and potentially fragile. A fake object with uncertain identity. Although we did not really pinpoint specific situations where it would break.
- All of this said, the LazyService thingie does have some ideas that might become part of ProxyManager some day.
Overall, I think it is wise to opt for the more mature solution (ProxyManager).
Use cases:
The idea of ProxyManager is having a rather small number of proxies (e.g. 20 in a project).
If we start using this in Drupal, I think we can expect having a lot more than that. Esp, if its use would be automated e.g. in plugin managers.
Overhead:
Proxies are meant to improve performance, but they add overhead in two ways:
- More (generated) classes to autoload.
(ocramius suggested they could all be put into the same file with the container, to not hit the filesystem as much)
- +1 level of indirection if you call $proxy->foo() instead of $original->foo().
This only matters if you really call foo() a lot.
The thing in #3 prevents these overheads, but I am now fairly convinced that the price is too high to accept this for D8 core.
Comment #11
Ocramius CreditAttribution: Ocramius commentedJust passing by to link some currently existing implementations.
at each run. When deploying the code, everything (including generated proxy classes) is dumped into the compiled DIC
Ideally, the implementation based on top of ProxyManager should not know what strategy is used to save/load proxies. How and where proxies are stored should be up to ProxyManager itself, and the consumers should just configure that.
I prefer the single-file-per-proxy-class approach, since it works really well with opcode caches. Dumping everything into a single file has the nasty side effect of causing lots of calls to the autoloader for all the parent classes.
For the implementation suggested by donquixote, it may land into ProxyManager later on, but it's really too limited and dangerous to be used in my opinion, so its requirement should be first backed by strong reasoning.
Comment #12
dawehnerMaybe I'm to harsh about it, but isn't that over abstraction/optimization without actually dealing with a problem yet.
If A depends on B it will use B at some point. If you don't use it, you seem to have a object with too many responsibilities.
Comment #13
donquixote CreditAttribution: donquixote commented@dawehner:
A nice example is breadcrumb builders.
The job of a breadcrumb builder is to
1. For a given route, decide whether this builder is responsible or not. This might need no dependencies at all, or just a few cheap ones.
2. If the builder thinks it is responsible, it does the hard job of actually building the breadcrumb. This may need a number of potentially expensive dependencies.
Lazy-loading can help us to only load the "expensive dependencies" when they are actually needed.
Either the "expensive dependencies" of the specific breadcrumb builder are lazy proxies.
Or the builder itself is lazy-loaded, with the "gatekeeper logic" done by something else.
Disclaimer:
I don't want to dive too deep into breadcrumb builders here, I just hope I am making it easier to imagine why one would want lazy proxy services.
Comment #14
yched CreditAttribution: yched commentedAnother example would be services that are only needed in the save() method of an entity or storage controller.
--> The class has a dependency on the service, so the service is instanciated each time you use the class, even though in 90% requests it's not actually needed.
Comment #15
donquixote CreditAttribution: donquixote commentedI would say a keyword is "conditional dependencies".
Comment #16
Crell CreditAttribution: Crell commentedIs there anything else to do in this issue given that work on it is happening upstream in Symfony where it belongs, and if it gets added there will get that capability anyway? I'm inclined to won't-fix.
Comment #17
donquixote CreditAttribution: donquixote commentedIf this does get into Symfony, then we need to discuss if and how we want to use it.
This could be done in a new issue, or by reopening this one.
Comment #18
tim.plunkettIt looks like this made it in!
http://symfony.com/doc/master/components/dependency_injection/lazy_servi...
https://github.com/symfony/symfony/pull/7890#ref-pullrequest-14069579
Comment #19
Crell CreditAttribution: Crell commentedI'd recommend waiting until July or August once things settle down, or even later. Then we can examine to see what dependencies seem to be "loaded but unused" the most, and mark those. That should be something we can do fairly late in the game once the code is fairly well settled down.
Comment #20
tim.plunkettPlus we'd need to wait on #1989230: Update to Symfony 2.3 anyway. Postponed is still better than won't fix.
Comment #20.0
tim.plunkettLink to #1863816: Allow plugins to have services injected
Comment #21
plachBeing able to rely on lazy services would be really useful in #1862202: Objectify the language system. They made into Symfony 2.3 but to exploit them we need to add the ProxyManager component.
According to the documentation, we can start using lazy services before we actually have proxy managers, because the DIC will fall back to just instantiating them.
However this looks like to right place to get proxy managers in, should we unpostpone this?
Comment #22
Crell CreditAttribution: Crell commentedWe should probably pull in ProxyManager when we update to Symfony 2.4. Which we should do right soon now: http://symfony.com/blog/symfony-2-4-0-released
See: #2161397: Update to Symfony 2.4.1
Comment #23
donquixote CreditAttribution: donquixote commentedSymfony components are on 2.4.* now, which means we can put this issue back to life, right?
Comment #24
Mile23+1 on pulling in ProxyManager.
Makes it much easier to define the Drupal console and console commands as services: #2242947: Integrate Symfony Console component to natively support command line operations
Thanks donquixote for the heads-up.
Comment #25
Mile23Is this really all that needs to happen? :-)
There should be a policy on what constitutes the need for lazy: true.
I'd suggest that if your service requires a database or a writable file system that you must mark your service as lazy. This allows for generating the container at any point in the bootstrap process.
Comment #26
Mile23Grr. It missed my patch.
Comment #27
Wim LeersI think one example of what we might want to do, is making all
paramconverter.*
services into lazy services. A lot of instantiation is happening there that is not necessary (i.e. the instantiated parameter converter services are never used, and they typically depend on expensive services).Something like:
And then:
I'm not at all familiar with all the details of the routing system, so I might be wrong here, but I think we should evaluate in which typical patterns in Drupal 8 we should recommend or enforce lazy services.
Another clear example is event subscriber services: by their very nature, they always need to be instantiated (since they contain logic that determines when to react), but their dependencies are only rarely needed: when the event actually fires. So their dependencies are again good candidates for lazy instantiation.
Finally, a good way to perform benchmarking and profiling with any of the above suggestions applied, is by requesting non-HTML response routes, since they are typically significantly cheaper. E.g.
/system/ajax
.(I discovered this problem as part of profiling for another issue; will link that here as soon as I've posted it.)
Comment #28
donquixote CreditAttribution: donquixote commented"ProxyManager" should be in the issue title.
Comment #29
Wim LeersI just posted the profiling where I discovered what's described in #27. See https://www.drupal.org/node/2301317#step2.
Comment #30
dawehnerI once had a discussion with lsmith to optimize the performance of lazy proxy managers: https://github.com/symfony/symfony/pull/9596
Comment #31
catchComment #32
Wim LeersNow that beta has been tagged, we should start looking into this again. Let's just try it and see what the performance impact is.
Comment #33
Fabianx CreditAttribution: Fabianx commentedThis is an important task to improve performance I think for dependencies that are seldom used.
Comment #34
dawehnerWorking on it.
Comment #35
dawehnerOkay, so this adds lazyness to lock and log
Comment #37
Fabianx CreditAttribution: Fabianx commentedThe code this produces is rather horrible in terms of performance ...
I still think we could just silently re-inject the right object in the parent class for true lazy injection instead of doing this all over again and again.
Without any added interface this would work nicely as there is no side-effect, but that is an implementation detail.
Comment #38
Wim Leers#37 suggests that https://github.com/symfony/symfony/pull/9596 might be necessary for Drupal 8.
Comment #39
dawehnerYeah we could indeed do that or start from a proper one from the beginning.
@fabianx
Did you tried to do some benchmarks for this little change here?
Comment #40
Fabianx CreditAttribution: Fabianx commentedIn a very quick and simple benchmark (front page), there was not much different, around 154 function calls more and 300 kB more memory overall a little slower, so not a huge win for such a scenario.
This needs probably a more specialized setup to make sense.
Comment #41
Wim LeersWhile profiling "baseline performance", I once again stumbled upon a massive service dependency tree, where lazy services could be a solution. See #2368275: EntityRouteEnhancer and ContentFormControllerSubscriber implicitly depend on too many services.
Comment #42
Fabianx CreditAttribution: Fabianx commentedThe problem is these proxy services have _massive_ overhead.
They also care for all the edge cases (__get/__set, etc.), while what we need in Drupal is slick and small and should make the service lazy per "usage" not globally.
I have a proposal designed for a potential syntax, that I will post here hopefully this week.
Comment #43
Fabianx CreditAttribution: Fabianx commentedHere is the proposal using a lazy service manager that creates a simple 80% cases-it-will-work stub similar to a PHPUnit mock, that all call back to __call(). So really simple, but won't work for complicated classes / services using magic __call or __get / __set methods or public properties.
The proposal is you have lazy service _dependencies_, not lazy services itself and the concept is called 'deferred setter injection'. This could be accomplished also by changing symfony and introducing a LazyReference for the container and e.g. a @!service syntax in YML, but this proposal is the MVP that could work _now_.
We need a LazyServiceTrait:
That just passes through the $definition to the lazy_manager with the current class.
Syntax in core.services.yml:
Before:
After:
And thats it, the class still needs to provide setters for the injection, but could use common traits obviously for re-usability.
What happens is that:
setLazyServices is called during service retrieval and will create the corresponding mock services - similar to the symfony proxy - and then inject them into the class via the given set methods:
The LazyService object _instance_ would look like:
This is just an example the information could also be stored in the lazy manager itself based on a map per object instance.
(The __call callback is the strategy how e.g. mockery builds their mock objects, other strategies are possible and this is a detail as it needs to be transparently exchangeable.)
But the idea is pretty simple and we can always beautify the syntax if we deem the concept useful.
Comment #44
donquixote CreditAttribution: donquixote commentedI generally support per-usage proxying, and totally agree with the overhead concerns in #42.
However, I think I would prefer a different system.
(Not sure if this exact syntax would work, but something equivalent)
For a consumer of proxy services in service.yml:
"#proxy" means that the container is allowed to pass in a proxy object instead of the real thing.
in the class Foo:
This means, the class Foo can be written proxy-aware for performance gain, but doesn't need to be.
And we don't have to give up constructor injection.
For my taste, the proxy class should be auto-generated based on an interface (or based on a class, but I'd prefer interface), with an additional onProxyInstantiation() method, using the observer pattern.
The Logger implementation does not really need to care that it is going to be proxied.
EDIT: Maybe as a shortcut:
EDIT:
It has to be
if ($logger instanceof ProxyInterface)
, notLoggerInterface
.Comment #45
Fabianx CreditAttribution: Fabianx commentedTo clarify some more:
This proposal and especially the LazyServiceTrait is meant as a band-aid until we have determined if the approach is worth to introduce to Symfony:
The wish implementation is:
- Use something like:
or in Symfony land:
However it is important to profile the approach first to see what gains we get and where and that is where the trait is most useful. This still uses the proven concept of setter injection (albeit calling it when the service is wanted), but is besides that completely transparent to the class.
E.g. if you want to proxy a service that only uses constructor injection, you can do (without the syntax sugar):
already now. So its possible to lazify any service - obviously syntax sugar is more nice, but even then more setter methods might be needed ...
Comment #46
dawehnerThe first thing I try to understand from the various proposed solutions is why we don't
mark the service itself as lazy but rather adapt all the users.
Based upon that I wonder what happens if any contrib module misses a lazy logger. Accidentally you get
all of it initialized ... which is what you wanted to avoid in the first place.
Please enlighten me.
Comment #47
Fabianx CreditAttribution: Fabianx commented#46: There is two use cases, symfony proxies solves only one of them.
1) A service that is always lazy and should always be initialized on demand and is not performance critical. A logger falls into that category, proxy pattern is fine here.
2) A service that uses another service, but only 10% of the time. This is lazy service dependencies. The dependency is initialized on demand.
This is still cleaner than making all those services container aware.
2) is what we are trying to solve here.
Comment #48
plachI was talking about a solution similar to #43 with @Crell a few months ago: he didn't seem quite happy about it so I forgot it. However @catch was interested in having a look to the code and since it's similar to what Fabian is suggesting, I thought it would be worth posting it, maybe there's some good idea after all ;)
As I said, the concept is similar to what Fabian is proposing: lazy service instantiation is performed by a container-aware trait that is passed a set of services the consumer is allowed to instantiate. When instantiating the consumer a child class extending it and using the trait is dynamically defined via eval. This approach allows to make any class use lazy services transparently, as long as they use properties to store references.
If I didn't do something stupid with git diff, the attached patch should provide a test module that performs some very raw micro benchmarks with my experimental code. These are my numbers with 100.000 iterations:
Lazy instantiation is around 30% slower than an almost empty instantiation, but here we are obviously not measuring the advantages of lazy services themselves, as those are always instantiated during the test. Moreover if we assume 1000 lazy service instantiations during a page request, which should be a lot, we are talking about 7ms, which should be affordable :)
Comment #49
donquixote CreditAttribution: donquixote commented@plach (#48), @Fabianx (#45)
Since I was a bit confused about the trait solution, here is a quick explanation. Maybe it helps others.
The trait solution relies on the fact that after unsetting a property with unset(), trying to access it (even from a method in the same class) triggers magic __get().
Setting the property to NULL instead of unsetting it does NOT have this effect.
The effect equally applies for public, protected and private properties, but the __get() will always be public.
So the idea is a self-initializing property.
There are some quirks with this, if trying to access the property from outside:
(probably not an issue, because $container->get() is not supposed to return NULL)
Besides this, I see some architectural deficits:
The original invariant was that after __construct(), the $foo->logger property would always contain a valid logger object. This could be a proxy logger instead of the real one, but we don't care, because the proxy logger IS a valid logger.
The new invariant is that after __construct(), the $foo->logger property, whenever it is accessed, will magically provide a valid logger object. This is equivalent in most of the cases we care about, but it is a more complex invariant.
Alternative
I understand this was meant as a band-aid. But I think we can do better than this, even for a one-off solution.
Starting from #44, we can do this less generic and more one-off, like this:
All the LoggerInterface methods are implemented in a way that triggers the proxy instantiation. This can be manually written, assuming there will only be very few such services.
At this point, the class does not know that it receives a proxy logger instead of a real one.
arguments: ['@proxy_logger']
instead ofarguments: ['@logger']
.So far, so good. This should already work now.
And so far, the Foo service is totally agnostic of any proxy mechanics.
Now we add a some details to replace the foo service's proxy logger reference with a real one.
$proxy_logger->onProxyInstantiation(array($foo, 'setLogger'));
directly after the foo service construction. Maybe the only way to do this is using a factory to create the foo service.Depending on the implementation, the Foo class is now either directly aware of the proxy mechanic (if onProxyInstantiation() is called in the constructor), or it just has a general-purpose setLogger() method, without really knowing that it is meant for proxy replacement.
This entire stuff can be implemented without modifying the basic container mechanics. The container will think of "proxy_logger" simply as a separate service.
The factory, if we need one, could be shared across all proxy consumers that want to use this mechanic.
Comment #50
donquixote CreditAttribution: donquixote commented@dawehner (#46), @Fabianx (#47)
Of course it would be nice if we could identify some examples that fall into category 2). Any ideas?
Also, category 2) can be split in two sub-categories:
2.a) A service uses another service, but only 10% of the time (*). And in those cases where it does, it will only make one or very few calls to this service. So we don't care about indirection overhead.
2.b) A service uses another service, but only 10% of the time (*). But in those cases where it does, it will make a bazillion of calls to this service. So any indirection in these calls adds a perceivable overhead. This is where the onProxyInstantiation() becomes interesting.
(*) "10% of the time" could mean either "every 1 out of 10 requests" (e.g. only on cache clear), but it could also mean "in 1 out of 10 Drupal sites". In other words, there could be sites where the 10% case happens all the time, and other sites where it never happens.
Comment #51
donquixote CreditAttribution: donquixote commentedAdding to #49:
Another interesting case is when a consumer service tries to pass the (proxy) object to other classes.
With the #43 / #45 / #48 __get() implementation, passing $this->logger to another object will trigger instantiation, and then pass the real logger. With the #44 / #50 proxy implementation, passing $this->logger to another object will pass the proxy object.
I am not saying which of these is more desirable, just stating it as an observation.
Comment #52
plach@donquixote:
True, however devs should not make assumptions on the service implementation they are dealing with, and even if they do they have no way to tell whether the property has already been initialized. Thus in practice the property stays protected.
True, but this is how 99% of our services work, so I don't see this as a big limitation actually. Anyway, I think we could find some workaround for this, the code above is far from a final version. For instance we could exploit setter injection and let the consumer class actually manage references.
This is not exactly true: the consumer class is completely unaware of what's going on (see
TestConsumer
), all the magic happens in the trait, and the trait is added to a subclass of the consumer dynamically. In fact you can (and should) code your consumer class without making any assumption on how dependencies are instantiated. A static analysis ofTestConsumer
will tell you it's a regular service exploiting constructor DI.This is supposed to be performed by the container internally when instantiating the consumer, not something developers should worry about.
Again, a developer should not make assumptions on lazy services being around or on how they will be instantiated. If they do they are simply writing bad code, it would be exactly the same thing as assuming an implementation when dealing with a parameter type-hinted with an interface. Morever you can already have access to the container in a far easier way:
\Drupal::getContainer()
.Yes, but the resulting DX is way better IMHO :)
That array would be derived from the container configuration, exactly as all the other information about service instantiation. I don't see how this makes the process more fragile than it currently is: if you mess something up, your site is likely to blow up in your face immediately.
Comment #53
Fabianx CreditAttribution: Fabianx commented#50 / #51: That are very good points.
I believe 2) b) (the case where the 10% used service is called very often) to not appear very often and if it does, make it a getService() function and make that service container aware.
On #51, yes the proxy object might be pushed to somewhere else, which would null the lazyness effect and lead to more overhead.
That usually does not happen as we inject everything from the container itself - when passing a service, but it could.
But both bring us to a nice solution using stock Symfony lazy services:
Lazy Service Dependencies using ContainerAwareLazyServiceFactory
With that little helper class the proxy proxies to the exact same object as the real service and unless logger does not work with factories (which we need to test, but I think it should work seeing how factories are just special get methods in the compiled container), this should work very well and solve all our use cases.
Depending on how the symfony proxy works, it might be even feasible to just give the service to the method and return it directly, e.g.:
but again this depends on how the proxy is implemented. And might even use class: Logger instead - if that is what the proxy lazy flag works upon.
Also we could think of just adding a parameter to this services, like:
which could auto-generate the 'logger_lazy' class - though I think implicit might be better for now - and is not too much boilerplate.
Still I think this could work very well and we would still have full proxies (so object passing is not a problem), but only for the cases where we:
1) have debug code where performance does not matter
2) have the 10% case, where the proxied service is used so less that performance also does not matter.
Thanks!
Comment #54
donquixote CreditAttribution: donquixote commentedIf this is the case then we can stop caring about the indirection overhead, and directly pass in a proxy into these objects, without any onProxyInstantiation().
The consumer class does not need to change at all.
Scenario:
- Service "foo" (class Foo) require a logger, and will use it most of the time. So it should get the real thing.
- Service "bar" (class Bar) requires a logger, but will only use it 10% of the time. So it should get a proxy.
- Let's assume that creating the real logger is expensive.
Quick one-off solution:
- Create the "logger" service. This is the real one.
- Create a new service "proxy logger". Let it depend on the container to create the real thing when needed.
- Let service "foo" depend on "logger".
- Let service "bar" depend on "proxy logger".
That's it, problem solved.
More "advanced" future solution:
- Create the "logger" service. This is the real one.
- Let service "foo" depend on "logger".
- Let service "bar" depend on "logger#proxy"
- Let the container create a proxy logger service automatically when #proxy is appended to a service dependency name.
Proxy implementation:
Indirection overhead:
With these two solutions, whenever a Bar object does $this->logger->log(), we have a minor overhead caused by:
- An additional method call in the call stack (similar to a decorator)
- A check whether the real logger is initialized.
The "foo" service does not suffer this overhead.
However, all other components that receive a reference to the proxy logger from "bar" will have this overhead. But I think that is survivable.
We only care about this overhead in the case 2.b) in #50, which I think we agree is quite rare.
One case where I DID care about it was in xautoload, where I wanted to do two things:
- Skip or postpone registration of module namespaces, hook_xautoload() and other overhead on bootstrap, by using a proxy class loader in the cache autoload decorator.
- Have a competitive performance for autoload when the cache decorator is not hot or not active. The proxy indirection overhead mentioned above applies to every class being loaded, and it turned out that this broke the competitive benchmark.
For these edge cases, we can use the onProxyInstantiation() or onProxyInstantiationSet().
The drawback here is that this means the Bar service needs to be proxy-aware, IF it wants to win this performance edge.
The trait / __get() solution does fix this, because the trait is added by a container mechanic. But I am still not a big friend of it :)
@plach (#52):
You are correct in all of your points I think. But this does not invalidate my argument :) None of the things I mention in #49 was meant as a blocker. All I was doing there was to identify complexity that could be avoided.
Comment #55
plach@donquixote:
Sure, thanks :)
Comment #56
Fabianx CreditAttribution: Fabianx commented#54: As written in #53, we don't need any custom code anymore if we can use the full symfony proxies as described.
So any __get or trait discussion is moot at this point I think ...
Comment #57
dawehnerI'll try to summarize the discussion so far and make a point in what went wrong so far.
#43: Adapt the consumer to lazily fetch some of its dependencies. Some code needs to be adapted
#44: The consumer is adapted "manually" to be proxy aware: Some magic has to be configured in the constructor
#45: Adapt the consumer to provide a setter method which is used in case the service is requested
#48: Implementation example of plach:
Make the consumer container aware and use a __get magic trait to fetch the dependencies when they are needed.
#49: Alternative: Manually write proxy services for the things we know are slow. Use those in cases where your service is not used often.
On runtime replace the instance on all services with the real one, in case it is needed. (Note: You could leverage synchronized services here).
#53: Elaborate on previous ideas and make the syntax even less verbose in the container definition.
#54: Iterating on the manual proxy
Great discussion so far, so many iterations on existing ideas.
Let's step back for a while again:
Most ideas here though starts with the mind model of being in control of all the code out here.
Sadly this is really not the case in the world of custom and contrib code. If ANY service actually
calls out to 'logger', or does not use the Trait you are lost.
Given that, at least for the logger example,
you should set the default to be a lazy loading service. This would be totally doable with the manual proxy approach,
but instead of having proxy_logger you would end up with having logger and logger.non_lazy. In cases you then log
all the time, you use the non lazy one and you pay the cost for it, as you have to initialize it anyway.
Other examples like
theme manager -> theme negotiator -> theme handler -> config installer
are kinda different.Theme handler is not needed in most cases for the theme negotiator but I can easily imagine cases where its needed all the time.
For this example I would argue that
Comment #58
Crell CreditAttribution: Crell commentedI don't know why #57 was cut off... :-)
Daniel and I discussed this issue at BADCamp. In short, trying to solve the universal case seems pointless. Also, as Daniel noted above we cannot realistically depend on services to be "proxy aware". That breaks the encapsulation and decoupling we've fought so hard to introduce. Lazy-instantiation is a concept that only the container/compilation process should know about; neither the lazy service or the consumer of a lazy service should be aware of it. That's what decoupling buys us.
Instead, if we feel that the ProxyManager component is too complex (as it seems to be) we can write our own that covers the majority case. Eg, it wouldn't support public properties on services but... who cares. Services shouldn't have public properties anyway.
Instead, we can simply autogenerate a proxy class that does blind forwarding to the wrapped class. Ie, something vaguely like:
(That could probably be optimized better but you get the idea.)
Any service that is not compatible with that simple approach, well, it doesn't get to be proxied. If you want to be proxyable, don't do silly things!
This keeps both the consumer and service ignorant of all proxying, which is the point.
Comment #59
Fabianx CreditAttribution: Fabianx commentedUhm, I think people still misunderstand #53:
I was originally of the opinion that the Symfony Proxy Manager is not useful too us as it has too much overhead and we need something custom.
I have changed my mind and we don't need anything custom anymore, because the biggest drawback of setting a service to lazy is that it will always keep being a proxy.
#53 is not about a syntax change, but about something that allows to re-use the same service once lazy and once not lazy.
The reason why this works for 95% of core's cases is that:
a) The service can be lazy because it is something that is only used for debugging, e.g. a logger
b) The service is part of the dependency chain of another service, but only used 10% of the time, but then also called some, the service can be a _lazy dependency_.
If we ever have the use case that we have a service that uses another service only like 10% of the time, but then very heavily, we can always split this up into two services or make the original service container aware.
Now lets find out which services should be true lazy (not in the critical path), logger, debugger, etc. and which services we can make lazy dependencies as a first start.
Comment #60
donquixote CreditAttribution: donquixote commentedI was going to write something longer but in short: I am personally not opposed to using or at least trying Symfony "lazy services" now, and discussing edge cases when they actually become relevant. We could then still throw an alternative solution into the mix.
EDIT: Or if others still think that ProxyManager is too heavy, I would also be ok with Crell's solution of writing this ourselves. But remember we need code generation! This is not without some work.
Comment #61
donquixote CreditAttribution: donquixote commentedJust to illustrate that the edge cases may still become relevant in the future, just not immediately:
Imagine we would make "theme.manager" a lazy service.
I think we call \Drupal::theme()->render() really a lot. So then the overhead of an additional conditional check and an additional method call each time (see #) would actually become measurable and maybe relevant for performance.
Some of the alternative solutions so far would mitigate this overhead. But I still think that ProxyManager or #58 is good enough for the common use case.
The other overhead, of ProxyManager supposedly being a too heavy library, only applies on rebuild. After that, we simply use the generated class.
Comment #62
znerol CreditAttribution: znerol commentedThis might be easy to measure before deciding on an approach. Just replace the theme service with a hard-coded decorator and do some profiling before and after.
Comment #63
Fabianx CreditAttribution: Fabianx commented\Drupal::theme()->render() is a bad example, because in that case we would just get the real service instead ...
e.g.
theme_manager.real
because for static class calls it does not matter ...
Or even @donquixote's little patch to the proxy, which would store the real service in the container after initialization, so further calls, don't get the proxy anymore, but injected dependencies keep being the proxy would be enough here ...
Comment #64
donquixote CreditAttribution: donquixote commentedIn an ideal world, \Drupal::theme() would not exist, and instead, services that need it would have the theme.manager injected.
Proxying it might actually make this one step more feasible than it is now.
But tbh, I was just looking for a method on a service that is called a lot of times per request.
(and sorry, I had no time yet to test this)
Comment #65
plach@dawehner:
Just a small but IMO important remark, just in case someone missed the point: the consumer is not container-aware in code, only the runtime-created subclass using the magic trait is :)
Comment #66
dawehnerSo indeed loggers are a bit tricky due to factory in between.
For other services, like cron, the following approach works.
Comment #67
Wim LeersNice work!
Comment #68
chx CreditAttribution: chx commentedThis is shaping up very nicely, however...
+ $signature_line .= implode(', ', $parameters);
this is problematic -- what about the default values? Perhaps typehinting? More importantly, how come this is something the phpunit mocking library can't do??Comment #69
donquixote CreditAttribution: donquixote commentedAny reason not to have the container as a dependency, instead of calling \Drupal::getContainer()? Hibernation-between-request worries?
Comment #70
chx CreditAttribution: chx commentedI missed buildParameter , nevermind!
Comment #71
dawehnerSome work on the nicety of the output.
A couple of issues which would be maybe worth to investigate in regards of too many initialized services:
Comment #72
dawehnerThis time with tests.
Comment #73
chx CreditAttribution: chx commentedPlease use
<<<'EOS'
for multiline syntax. (This is nowdoc available since PHP 5.3)Comment #74
Crell CreditAttribution: Crell commentedWhy not extend the class being proxied instead? That way we're guaranteed that all type checks will pass, whether they're interface checks or not.
Indent. :-)
Comment #75
dawehner<3
Well, the problem is that I really want to limit the usecases of the proxy. If we want to support every edge case, you end up in a proxy which looks as complicated as the
ProxyManager
one.This is really the standard and simple case. One problem for example then is that you end up calling to the parent class, even that one is not initialized properly yet.
There are more and more things you can do. Implement __get and __set etc. You can argue, sure, but from my point of view supporting more usecases results in problems.
HA!
Comment #76
Crell CreditAttribution: Crell commentedI agree we shouldn't try to cover every edge case, just those that are straighforward to address. It seems like extending falls into that category. (Messing around with __get/__set on a service definitely is not.) I can't parse what "One problem for example then is that you end up calling to the parent class, even that one is not initialized properly yet." means, though. You mean it's not straightforward to address?
Comment #77
donquixote CreditAttribution: donquixote commentedIn theory, the consumer of a service should only care about the interface of a dependency, not the implementation.
This way, services can be exchangeable not just with a subclass, but anything implementing the same interface. Using a proxy is exactly such a case, imo. Very similar to a decorator.
The reason why ProxyManager does not do this is that in a lot of real-world PHP, only half of the classes have interfaces.
(This is what I remember from a discussion long time ago, but it might have changed since then)
Can we be better than this?
Inheriting properties and private/protected methods which are then never used, feels semantically wrong.
In most cases it won't cause any technical problems, but it would feel better to avoid that.
Comment #78
dawehnerI would rather argue that we should support only a subset of funcionality which themselves are considered to be good practice. If people
write such complex code, and they really need proxy services, you can also always write your own decorator for it.
Comment #79
dawehnerAdded some more services to be lazy.
Comment #80
dawehnerAdded some more services to be lazy.
Comment #81
dawehnerAdded some more services to be lazy.
Comment #82
dawehnerAdded some more services to be lazy.
Comment #83
dawehnerAdded some more services to be lazy.
Comment #84
dawehnerAdded some more services to be lazy.
Comment #85
dawehnerAdded some more services to be lazy.
Comment #86
dawehnerAdded some more services to be lazy.
Comment #87
dawehnerAdded some more services to be lazy.
Comment #88
dawehnerAdded some more services to be lazy.
Comment #89
dawehnerAdded some more services to be lazy.
Comment #90
dawehnerAdded some more services to be lazy.
Comment #91
dawehnerAdded some more services to be lazy.
Comment #92
dawehnerAdded some more services to be lazy.
Comment #93
dawehnerAdded some more services to be lazy.
Comment #95
plachYou forgot "... and I'm fine with it" ;)
Comment #96
chx CreditAttribution: chx commentedIf you'd use
<<<'EOS'
instead of<<<EOS
you could use just$
without escaping. It's nowdoc vs heredoc.Comment #97
dawehner@chx
Ah that makes things easier in some cases.
Comment #98
Crell CreditAttribution: Crell commented#97 looks like it has a number of other patches mixed up in it. :-(
Comment #99
dawehner... #97 still has a patch, why is it on needs work?
Comment #100
znerol CreditAttribution: znerol commentedThe file size of the patch in #97 is 145.1 KB, please post a proper patch.
Comment #101
dawehnerThank you for the pointer.
Updated the IS a bit.
Comment #102
dawehnerIs anything willing the review the patch in the correct form?
We obviously also need profiling with some testcases (slim bootstrap, full site, maybe also berdir's example).
Once we get this in we could easily tag more services, in case needed as lazy.
Comment #103
dawehnerFixed the point made from CHX
Comment #104
chx CreditAttribution: chx commentedI suggested such templating for getProxyFactoryCode and buildExpectedClass as well (most of the variables are escaped here, after all). Also, the assertEquals in testGetProxyFactoryCode doesn't need double quotes and that'd allow to get rid of more \$. All in all, we could get rid of every single escaped dollar sign for (much) easier readability.
One more important thing: I notice static functions, in general, aren't supported. Are we sure that's what we want? Probably yes, but let's ponder on it a bit.
Comment #105
dawehnerThat is a good question ... personally I always considered this issue to solve the simple common case: an interface with public methods and that is all.
Comment #107
chx CreditAttribution: chx commentedWell, I did think on this and since these are services, you can't get the classname directly (that's the crux of the matter) but rather an instance of it like
$service = $container->get('foo')
and then you can do$service->whatever()
and there's no need of$service::whatever()
. You can call static methods as instances methods; it's the other way 'round that PHP throws a hissy fit (strict error).This also means that we need to document at the fabled somewhere that service interfaces shouldn't contain public static methods.
Comment #108
dawehner* Ensured that we wrap static methods, its better to be safe than broken
* Uses EOS in tests as well.
Comment #109
dawehnerOne last time
Comment #110
chx CreditAttribution: chx commentedMoshe's flamegraphs made it crystal clear that finding and loading these classes take a lot of time. We will need to carefully evaluate which service to mark lazy in the future but having the capability and starting with these two is a good idea.
Comment #111
alexpottWhy do we need $class_start? Ah I see indentation. This results in some weirdness of coding standards. The container dump now has both our standards and symfony's. I think given this is a component that extends container functionality we should consider using Symfony's standards.
Also I think we should add a docblock for the class similar to the one that symfony adds to the container saying this has been auto-generated, for which service, and that the class is for runtime use only (or something like that).
This results in lot of sapce...
class Drupal_Core_Cron_Proxy implements \Drupal\Core\CronInterface {
class Drupal_Core_Plugin_CachedDiscoveryClearer_Proxy {
Dependency on \Drupal in component code? Let's inject the container into the Proxy object.
Seems to be an unnecessary comment.
Comment #112
dawehnerHopefully I managed to fix the codestyle properly, I never get it right in PRs.
Note: We proxy the class, not the service ID directly.
The best we can do is so something like this:
Okay, let's inject things
Fixed.
Here is an example of a generated class:
Comment #113
dawehnercomposer.json
fileComment #114
effulgentsia CreditAttribution: effulgentsia commentedAlthough I've been aware of this issue for quite some time, I just now skimmed the patch for the first time. Looks like some great thinking, discussion, and coding have gone into this.
What's the advantage of this custom lightweight ProxyBuilder over Symfony's? Is it runtime performance, and if so, do we have any benchmarks/profiling that show how much better ours is? Or is it some other benefit? Tagging for an issue summary update to include that info. Also, is there any plan to try to get this into upstream, or do we not expect that to be successful for some reason, and if so, what's that reason?
Should this be using Core's ProxyBuilder instead of Component's? If not, I don't see when Core's ever gets used.
Comment #115
effulgentsia CreditAttribution: effulgentsia commentedAlso, from what I can tell, this patch only solves #47.1, and not #47.2. I haven't read every comment since then thoroughly: has 47.2 been deemed unnecessary, or is the plan to open a followup for it?
Comment #116
chx CreditAttribution: chx commentedI just realized we forgot to support functions returning a reference.
Comment #117
dawehnerHa, good point :)
It is not needed to get everything right, absolutely. fixed it.
The proxy builder is the building block to also inject a proxied version of a specific dependency of a service. Sadly the symfony php dumper is written
in the most un-extendeable way you could imagine.
The generated code of https://github.com/Ocramius/ProxyManager, at least together used with the symfony bridge, produces really verbose code, with reflection
used to call methods.
Comment #118
Wim LeersThis issue still has a "needs profiling" tag. But it's not entirely clear what needs to be profiled: is it the custom proxybuilder itself, or is it the performance benefit on Drupal 8?
I originally the answer was "Drupal 8", but now the answer needs to be "the proxybuilder", because this issue only marks 2 services as lazy, hence the performance impact is going to be negligible. It's fine to only mark 2 services as lazy, because this issue is big enough already; we don't want to make it 3 times bigger still.
So I think:
In other words: this issue introduces new infrastructure, uses it for a few things, and a follow-up issue will see where else this can be put to good use.
Comment #119
dawehnerGiven that this would be mainly added to not often used services, we will have a hard time in seeing a lot. We might have less CPU spent,
due to less code loading/function calls. Even in case we initialize them, the amount of additional function calls would probably just appear in xhprof, because of the function calll profiling overhead.
Comment #120
Wim LeersRight! That point, together with the fact that chx and Alex Pott have already scrutinized the generated code, IMO allow me to remove the 'needs profiling' tag.
Comment #121
effulgentsia CreditAttribution: effulgentsia commentedUpdated issue summary. My comments from #114 and #115 have been addressed. I haven't reviewed the ProxyBuilder and ProxyDumper implementations thoroughly, so leave to someone else to RTBC.
Comment #122
dawehnerThank you @effulgentsia to update the issue summary!!
Comment #123
chx CreditAttribution: chx commentedI did review those implementations; several times over and I believe they are good so per #121 I think this is a go.
Comment #124
alexpottThis issue is a major task that will improve performance significantly and the disruption it introduces is limited. Per https://www.drupal.org/core/beta-changes, this is a good change to complete during the Drupal 8 beta phase. Committed 2f628af and pushed to 8.0.x. Thanks!
Comment #126
Wim LeersCreated a follow-up: #2407177: Profile to determine which services should be lazy.
Comment #127
znerol CreditAttribution: znerol commentedThis has some weird side-effects on
DrupalKernelTest
.Comment #129
Fabianx CreditAttribution: Fabianx commentedReally late answer to #115:
In https://www.drupal.org/node/1973618#comment-9319165 I have shown that with a 20 loc helper class it is possible - given a lazy service working with factories (which again not sure this is doable, yet, but then its just more boilerplate) to have a lazy service, which dynamically gets the real service.
lazy: true is a dependency for that (which was implemented here in a great way!).
So if we ever deem that we need lazy service dependencies, because something is highly performance critical, but only used 10% of the time, then we can still re-visit this approach for example.
For now I think _fast_ lazy services are enough :).
Comment #130
fgm