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(...);

Related:
#1863816: Allow plugins to have services injected

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

donquixote’s picture

Status: Active » Needs review
FileSize
3.95 KB

The 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.

Status: Needs review » Needs work

The last submitted patch, d8-lazy-service-1973618-1.patch, failed testing.

donquixote’s picture

Status: Needs work » Needs review
FileSize
4.66 KB

Fixing the test

donquixote’s picture

Issue summary: View changes

simplify "A depends on B"

Crell’s picture

There 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.)

donquixote’s picture

Oh, 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

Crell’s picture

There'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.

yched’s picture

donquixote’s picture

Thanks 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:

  1. A proxy is meant to be indistinguishable from the original object.
  2. There will be one seperate proxy class per proxied service, to mimic the complete signature. (I hope I am correct on this one)
  3. The consumer does not know whether it has a proxy or the real thing.
  4. A service X that receives a proxy is meant to use the proxy instead of the real thing, throughout the lifetime of X, resulting in an indirection overhead each time the proxy is used.
  5. The $container->get($key) either always returns a proxy for service Y, or it always returns the real Y.
  6. $container->getDefinitions('my_super_slow_class')->setLazy(true); tells the DIC to always return a proxy instead of the real thing.
  7. It is a damn huge library.

LazyService from #3:

  1. The lazy service behaves mostly like the original, but only via magic methods. They are easily exposed by method_exists(), get_class() etc. Also it has a method "bindReference()" which is likely not present in the wrapped object (and if it was, it would probably have a different purpose).
  2. There is only one LazyService class. All lazy service wrappers are from the same class.
  3. The consumer knows that it receives a proxy object instead of the real thing. It even expects that.
  4. The consumer's variable that holds the proxy, will be replaced with the real thing once the instantiation triggers. This way, we avoid the indirection overhead most of the time when the depended-on service is used.
    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.
  5. The $container->get($key) always returns the real thing. We'd have a $container->getProxy($key) to return a proxy instead.
  6. A new mechanic in services.yml tells the DIC that a specific service X wants a proxy of Z, instead of the real Z.
    At the same time, another service Y might want the real Z, and not a proxy.
  7. It is done with a pretty small amount of code.
donquixote’s picture

donquixote’s picture

Conclusion:
(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.

Ocramius’s picture

Just passing by to link some currently existing implementations.

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.

dawehner’s picture

Maybe I'm to harsh about it, but isn't that over abstraction/optimization without actually dealing with a problem yet.

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.

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.

donquixote’s picture

@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.

yched’s picture

Another 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.

donquixote’s picture

I would say a keyword is "conditional dependencies".

Crell’s picture

Is 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.

donquixote’s picture

Status: Needs review » Closed (won't fix)

If 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.

tim.plunkett’s picture

Crell’s picture

Status: Active » Postponed

I'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.

tim.plunkett’s picture

Plus we'd need to wait on #1989230: Update to Symfony 2.3 anyway. Postponed is still better than won't fix.

tim.plunkett’s picture

plach’s picture

Being 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?

Crell’s picture

We 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

donquixote’s picture

Status: Postponed » Active

Symfony components are on 2.4.* now, which means we can put this issue back to life, right?

Mile23’s picture

+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.

Mile23’s picture

Is 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.

Mile23’s picture

Status: Active » Needs review
FileSize
399 bytes

Grr. It missed my patch.

Wim Leers’s picture

Issue tags: +Performance

I 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:

paramconverter:
  abstract: true
  lazy: true
  tags:
    - { name: paramconverter }

And then:

paramconverter.entity:
  parent: paramconverter
  class: Drupal\Core\ParamConverter\EntityConverter
  arguments: ['@entity.manager']

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.)

donquixote’s picture

Title: DIC: Lazy instantiation of service dependencies » DIC: Lazy instantiation of service dependencies (ProxyManager for "proxy services")

"ProxyManager" should be in the issue title.

Wim Leers’s picture

I just posted the profiling where I discovered what's described in #27. See https://www.drupal.org/node/2301317#step2.

dawehner’s picture

I once had a discussion with lsmith to optimize the performance of lazy proxy managers: https://github.com/symfony/symfony/pull/9596

catch’s picture

Category: Feature request » Task
Priority: Normal » Major
Wim Leers’s picture

Now that beta has been tagged, we should start looking into this again. Let's just try it and see what the performance impact is.

Fabianx’s picture

This is an important task to improve performance I think for dependencies that are seldom used.

dawehner’s picture

Working on it.

dawehner’s picture

Okay, so this adds lazyness to lock and log

The last submitted patch, 35: 1973618-drupal-only.patch, failed testing.

Fabianx’s picture

The code this produces is rather horrible in terms of performance ...

 public function lockMayBeAvailable($name)
    {
        $this->initializer54363d2469330108404861 && $this->initializer54363d2469330108404861->__invoke($this->valueHolder54363d246930a163896492, $this, 'lockMayBeAvailable', array('name' => $name), $this->initializer54363d2469330108404861);

        return $this->valueHolder54363d246930a163896492->lockMayBeAvailable($name);
    }

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.

Wim Leers’s picture

#37 suggests that https://github.com/symfony/symfony/pull/9596 might be necessary for Drupal 8.

dawehner’s picture

Yeah 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?

Fabianx’s picture

In 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.

Wim Leers’s picture

While 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.

Fabianx’s picture

The 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.

Fabianx’s picture

Here 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:

public function setLazyServices($lazy_manager, array $definition) {
  $lazy_manager->registerLazyServices($this, $definition);
}

That just passes through the $definition to the lazy_manager with the current class.

Syntax in core.services.yml:

Before:

class: Foo
arguments: ['@entity_manager']
calls:
 - [ 'setPluginManager', '@plugin_manager']
 - [ 'setLogger', '@logger']

After:

class: Foo
arguments: ['@entity_manager', null, null]
calls:
  - ['setLazyServices', '@lazy_manager', ['setPluginManager': 'plugin_manager', 'setLogger': 'logger' ]]

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:


public function registerLazyServices($caller, array $definition) {
  foreach ($definition as $setter => $service) {
    $lazy_service = $this->getLazyService($caller, $service);
    call_user_func(array($caller, $setter), $lazy_service);
  }
}

The LazyService object _instance_ would look like:


// Lazy Service Interface is the only way to distinguish between the real and fake class / service
// all other interfaces and the class to extend from are the same.
// - similar to how PHPUnit / Mockery "full" mock objects work.
class LazyService_for_Logger_123456 extends LoggerClass implements LoggerInterface, LazyServiceInterface {

public function __call($method, $args) {
  // This injects the real service gotten from the container now via setter injection.
  $service = $this->lm_LazyManager->unlazify($caller, $this->lm_ServiceName, $this->lm_ServiceSetter);

  // And then fakes that this call was already to the real service, so transparent exchange of the object.
  return call_user_func_array(array($service, $method)), $args);
}

// This is created via reflection class ...
public function somemethod() {
  return $this->__call(__FUNCTION__, func_get_args());
}

}

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.

donquixote’s picture

I 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:

class: Foo
arguments: ['@plugin_manager', '@logger#proxy']

"#proxy" means that the container is allowed to pass in a proxy object instead of the real thing.

in the class Foo:

class Foo {
  function __construct($plugin_manager, LoggerInterface $logger) {
    $this->pluginManager = $plugin_manager;
    $this->logger = $logger;
    if ($logger instanceof ProxyInterface) {
      // Not sure about the use ($this)
      $logger->onProxyInstantiation(function(LoggerInterface $real_logger) use ($this) {
        $this->logger = $real_logger;
      });
    }
  }
}

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:

    $this->logger = $logger;
    if ($logger instanceof ProxyInterface) {
      // Pass $this->logger by reference.
      $logger->onProxyInstantiationSet($this->logger);
    }

EDIT:
It has to be if ($logger instanceof ProxyInterface), not LoggerInterface.

Fabianx’s picture

To 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:

foo:
  class: 'Foo'
  arguments: ['@entity_manager', '@logger#proxy:setLogger']

or in Symfony land:

$container->setDefinition('foo', new Definition( 'Foo', array(new LazyReference('logger', 'setLogger'));

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):

class Bar extends Foo {
  use LazyServiceTrait;

  public function setLogger($logger) {
    $this->logger = $logger;
  }
}
foo: 
  class: Bar
  arguments: [...]
  calls:
  - ['setLazyServices', ['setLogger': 'logger']]

already now. So its possible to lazify any service - obviously syntax sugar is more nice, but even then more setter methods might be needed ...

dawehner’s picture

The 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.

Fabianx’s picture

#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.

plach’s picture

I 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:

Regular instantiation: 0.5475s
Lazy instantiation: 0.7445s
Consumer method call: 0.3194s
Lazy consumer method call: 0.3219s

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 :)

donquixote’s picture

@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:

  • If someone tries to access the property from outside, __get() will always be triggered, because the property is declared as protected.
  • If the property is not initialized yet, accessing it from outside will initialize it and leak the value to the public.
  • If the property is already initialized, accessing it from outside will throw an InvalidArgumentException.
  • If the property is initialized with value NULL, trying to access it from outside will re-initialize it.
    (probably not an issue, because $container->get() is not supposed to return NULL)

Besides this, I see some architectural deficits:

  • The names of protected or private properties of the consumer service have to be known in the service definition. Or in other words, the consumer service now depends on an array of service names by property name. So the external dependency signature now depends on internal property names.
  • The lazy dependency mechanics are implemented in the consumer class. Imo, the consumer class should rather be agnostic about this.
  • The consumer now depends on __initLazyAccessor() being called directly after the constructor. We no longer have the reliability of constructor injection.
  • The container is theoretically exposed to the consumer service (even though the $container property is private), which could simply override the __initLazyAccessor() method to "steal" the container.
  • We are replacing a very simple object-lifetime invariant with a more complex one.
    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.
  • String maps (and arrays in general) are fragile, that is, open to mistakes. The IDE cannot verify their structure, and we don't do any runtime validation either.
  • Overall, there is a lot of magic here: Relying on a language oddity, and then juggling with string property names. I am not generally against magic, but I think we can do better.

Alternative

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:

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:

  1. Create a ProxyInterface with ProxyInterface::onProxyInstantiation($function)
  2. Create a new service "proxy_logger" with class ProxyLogger implementing LoggerInterface and ProxyInterface.
    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.
  3. Create a service Foo depending on LoggerInterface (constructor injection).
    At this point, the class does not know that it receives a proxy logger instead of a real one.
  4. In the services.yml: arguments: ['@proxy_logger'] instead of arguments: ['@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.

  1. Add a setLogger() or setRealLogger() or replaceLogger() method to the Foo service class.
  2. Let the container call $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.
  3. Alternatively, we could call $proxy_logger->onProxyInstantiation(array($this, 'setLogger')) in the constructor, as suggested in #44.

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.

donquixote’s picture

@dawehner (#46), @Fabianx (#47)

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.

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.

donquixote’s picture

Adding 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.

plach’s picture

@donquixote:

If the property is not initialized yet, accessing it from outside will initialize it and leak the value to the public.

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.

The names of protected or private properties of the consumer service have to be known in the service definition. Or in other words, the consumer service now depends on an array of service names by property name. So the external dependency signature now depends on internal property names.

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.

The lazy dependency mechanics are implemented in the consumer class. Imo, the consumer class should rather be agnostic about this.

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 of TestConsumer will tell you it's a regular service exploiting constructor DI.

The consumer now depends on __initLazyAccessor() being called directly after the constructor. We no longer have the reliability of constructor injection.

This is supposed to be performed by the container internally when instantiating the consumer, not something developers should worry about.

The container is theoretically exposed to the consumer service (even though the $container property is private), which could simply override the __initLazyAccessor() method to "steal" the container.

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().

We are replacing a very simple object-lifetime invariant with a more complex one.
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.

Yes, but the resulting DX is way better IMHO :)

String maps (and arrays in general) are fragile, that is, open to mistakes. The IDE cannot verify their structure, and we don't do any runtime validation either.

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.

Fabianx’s picture

#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

  logger:
    class: Logger
    arguments: ['@some_other_service']
  logger_lazy:
    lazy: 1
    class: LoggerInterface
    factory_class: 'ContainerAwareLazyServiceFactory'
    factory_method: 'get'
    arguments: ['@service_container', 'logger']
  foo:
    class: Foo
    arguments: ['@logger_lazy']
  bar:
     class: Bar
     arguments: ['@logger']
  class ContainerAwareLazyServiceFactory {
     public function get($container, $service_name) {
        return $container->get($service_name);
     }
  }

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.:

  logger_lazy:
    lazy: 1
    class: LoggerInterface
    factory_class: 'LazyServiceFactory'
    factory_method: 'get'
    arguments: ['@logger']
  class LazyServiceFactory {
     public function get($service) {
        return $service;
     }
  }

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:

  logger:
    class: Logger
    arguments: ['@some_other_service']
    lazy_dependency: 1

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!

donquixote’s picture

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.

If 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:

class ProxyLogger {
  [..]
  function log($message) {
    if (!isset($this->logger)) {
      $this->logger = $this->container->get('logger');
    }
    $this->logger->log($message);
  }
}

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.

plach’s picture

@donquixote:

Sure, thanks :)

Fabianx’s picture

#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 ...

dawehner’s picture

I'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:

  • The problem of services is that they initialize everything in the chain (this is what it makes slow)
  • Any kind of service and especially controller/plugin could depend on that service (logger here) which results in the chain

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

Crell’s picture

I 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:

class Foo {

  public function __construct(ThingieInterface $t, StuffInterface $s) { ... }

  public function bar() {}

  public function baz($a) {}
}

class Foo_Proxy extends Foo { // So that type hints will always be valid

  public function __construct(ContainerInterface $container) {
    $this->container = $container;
  }

  public function bar() {
    return $this->getProxied()->bar();
  }

  public function baz($a) {
    return $this->getProxied()->baz($a);
  }

  protected function getProxied() {
    return $this->container->get('foo_for_realz');
  }
}

(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.

Fabianx’s picture

Uhm, 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.

donquixote’s picture

I 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.

donquixote’s picture

Just 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.

znerol’s picture

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.

This 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.

Fabianx’s picture

\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 ...

donquixote’s picture

\Drupal::theme()->render() is a bad example, because in that case we would just get the real service instead ...

In 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)

plach’s picture

@dawehner:

Make the consumer container aware and use a __get magic trait to fetch the dependencies when they are needed.

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 :)

dawehner’s picture

FileSize
6.71 KB

So indeed loggers are a bit tricky due to factory in between.

For other services, like cron, the following approach works.

class Drupal_Core_Cron_Proxy implements Drupal\Core\CronInterface {

 /**
  * @var string
  */
  protected $serviceId;


  /**
   * @var \Drupal\Core\Cron
   */
  protected $service;
public function __construct($service_id) {
  $this->serviceId = $service_id;
}

protected function lazyLoadItself() {
  if (!isset($this->service)) {
    $method_name = 'get' . Container::camelize($this->serviceId) . 'Service';
    $this->service = \Drupal::getContainer()->$method_name(FALSE);
  }

  return $this->service;
}
public function run() {
  return $this->lazyLoadItself()->run();
}

}
Wim Leers’s picture

Nice work!

chx’s picture

This 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??

donquixote’s picture

    $this->service = \Drupal::getContainer()->$method_name(FALSE);

Any reason not to have the container as a dependency, instead of calling \Drupal::getContainer()? Hibernation-between-request worries?

chx’s picture

I missed buildParameter , nevermind!

dawehner’s picture

FileSize
8.55 KB
6.25 KB

Some 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:

dawehner’s picture

FileSize
18.45 KB
11.16 KB

This time with tests.

chx’s picture

Please use <<<'EOS' for multiline syntax. (This is nowdoc available since PHP 5.3)

Crell’s picture

  1. +++ b/core/lib/Drupal/Component/ProxyBuilder/ProxyBuilder.php
    @@ -0,0 +1,217 @@
    +    if ($interfaces = $reflection->getInterfaceNames()) {
    +      foreach ($interfaces as &$interface) {
    +        $interface = '\\' . $interface;
    +      }
    +      $output .= ' implements ' . implode(', ', $interfaces);
    +    }
    

    Why 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.

  2. +++ b/core/lib/Drupal/Component/ProxyBuilder/ProxyBuilder.php
    @@ -0,0 +1,217 @@
    +    // Intend the output.
    

    Indent. :-)

dawehner’s picture

FileSize
18.43 KB
1.97 KB

Please use <<<'EOS' for multiline syntax. (This is nowdoc available since PHP 5.3)

<3

Why 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.

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.

Indent. :-)

HA!

Crell’s picture

I 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?

donquixote’s picture

In 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.

dawehner’s picture

In most cases it won't cause any technical problems, but it would feel better to avoid that.

I 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.

dawehner’s picture

FileSize
18.93 KB
1.19 KB

Added some more services to be lazy.

dawehner’s picture

Added some more services to be lazy.

dawehner’s picture

Added some more services to be lazy.

dawehner’s picture

Added some more services to be lazy.

dawehner’s picture

Added some more services to be lazy.

dawehner’s picture

Added some more services to be lazy.

dawehner’s picture

Added some more services to be lazy.

dawehner’s picture

Added some more services to be lazy.

dawehner’s picture

Added some more services to be lazy.

dawehner’s picture

Added some more services to be lazy.

dawehner’s picture

Added some more services to be lazy.

dawehner’s picture

Added some more services to be lazy.

dawehner’s picture

Added some more services to be lazy.

dawehner’s picture

Added some more services to be lazy.

dawehner’s picture

Added some more services to be lazy.

Status: Needs review » Needs work

The last submitted patch, 79: 1973618-79.patch, failed testing.

plach’s picture

You forgot "... and I'm fine with it" ;)

chx’s picture

If you'd use <<<'EOS' instead of <<<EOS you could use just $ without escaping. It's nowdoc vs heredoc.

dawehner’s picture

Status: Needs work » Needs review
FileSize
145.1 KB
2.19 KB

@chx
Ah that makes things easier in some cases.

Crell’s picture

Status: Needs review » Needs work

#97 looks like it has a number of other patches mixed up in it. :-(

dawehner’s picture

Status: Needs work » Needs review

... #97 still has a patch, why is it on needs work?

znerol’s picture

Status: Needs review » Needs work

The file size of the patch in #97 is 145.1 KB, please post a proper patch.

dawehner’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
19.21 KB

Thank you for the pointer.

Updated the IS a bit.

dawehner’s picture

Is 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.

dawehner’s picture

FileSize
18.81 KB
1.36 KB

Fixed the point made from CHX

chx’s picture

I 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.

dawehner’s picture

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.

That 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.

Status: Needs review » Needs work

The last submitted patch, 105: 1973618-105.patch, failed testing.

chx’s picture

Well, 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.

dawehner’s picture

Status: Needs work » Needs review
FileSize
20.3 KB
5 KB

* Ensured that we wrap static methods, its better to be safe than broken
* Uses EOS in tests as well.

dawehner’s picture

FileSize
971 bytes
20.34 KB

One last time

chx’s picture

Status: Needs review » Reviewed & tested by the community

Moshe'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.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/lib/Drupal/Component/ProxyBuilder/ProxyBuilder.php
    @@ -0,0 +1,240 @@
    +    $output = '';
    +    $class_start = 'class ' . $this->buildProxyClassName($class_name);
    ...
    +    // Indent the output.
    +    $output = implode("\n", array_map(function($value) {
    +      if ($value === '') {
    +        return $value;
    +      }
    +      return "  $value";
    +    }, explode("\n", $output)));
    

    Why 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).

  2. +++ b/core/lib/Drupal/Component/ProxyBuilder/ProxyBuilder.php
    @@ -0,0 +1,240 @@
    +    $class_start = 'class ' . $this->buildProxyClassName($class_name);
    ...
    +    if ($interfaces = $reflection->getInterfaceNames()) {
    +      foreach ($interfaces as &$interface) {
    +        $interface = '\\' . $interface;
    +      }
    +      $output .= ' implements ' . implode(', ', $interfaces);
    +    }
    +
    +    $properties = <<<'EOS'
    + {
    

    This results in lot of sapce...
    class Drupal_Core_Cron_Proxy implements \Drupal\Core\CronInterface {
    class Drupal_Core_Plugin_CachedDiscoveryClearer_Proxy {

  3. +++ b/core/lib/Drupal/Component/ProxyBuilder/ProxyBuilder.php
    @@ -0,0 +1,240 @@
    +    $this->service = \Drupal::getContainer()->$method_name(FALSE);
    

    Dependency on \Drupal in component code? Let's inject the container into the Proxy object.

  4. +++ b/core/tests/Drupal/Tests/Component/ProxyBuilder/ProxyDumperTest.php
    @@ -0,0 +1,107 @@
    +    // Existing and lazy service.
    +
    

    Seems to be an unnecessary comment.

dawehner’s picture

Status: Needs work » Needs review
FileSize
20.91 KB
9.78 KB

Why 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.

Hopefully I managed to fix the codestyle properly, I never get it right in PRs.

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).

Note: We proxy the class, not the service ID directly.
The best we can do is so something like this:

/**
 * Provides a proxy class for \{{ class }}
 *
 * @see \Drupal\Component\ProxyBuilder
 */
Dependency on \Drupal in component code? Let's inject the container into the Proxy object.

Okay, let's inject things

Seems to be an unnecessary comment.

Fixed.

Here is an example of a generated class:


/**
 * Provides a proxy class for \Drupal\Core\Cron.
 *
 * @see \Drupal\Component\ProxyBuilder
 */
class Drupal_Core_Cron_Proxy implements \Drupal\Core\CronInterface
{

  /**
   * @var string
   */
  protected $serviceId;

  /**
   * @var \Drupal\Core\Cron
   */
  protected $service;

  public function __construct(ContainerInterface $container, $serviceId)
  {
      $this->container = $container;
      $this->serviceId = $serviceId;
  }

  protected function lazyLoadItself()
  {
      if (!isset($this->service)) {
          $method_name = 'get' . Container::camelize($this->serviceId) . 'Service';
          $this->service = $this->container->$method_name(false);
      }

      return $this->service;
  }

  public function run()
  {
      return $this->lazyLoadItself()->run();
  }

}
dawehner’s picture

FileSize
26.84 KB
11.55 KB
  • Fixed the indentation
  • Added the container as protected property
  • Added the dependency serialiazation trait to a core specific extension
  • Added a composer.json file
effulgentsia’s picture

Although 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?

+++ b/core/lib/Drupal/Core/DrupalKernel.php
@@ -7,6 +7,8 @@
+use Drupal\Component\ProxyBuilder\ProxyBuilder;

Should this be using Core's ProxyBuilder instead of Component's? If not, I don't see when Core's ever gets used.

effulgentsia’s picture

Also, 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?

chx’s picture

I just realized we forgot to support functions returning a reference.

dawehner’s picture

FileSize
27.89 KB
3.05 KB

Should this be using Core's ProxyBuilder instead of Component's? If not, I don't see when Core's ever gets used.

Ha, good point :)

I just realized we forgot to support functions returning a reference.

It is not needed to get everything right, absolutely. fixed it.

Also, 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?

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.

What's the advantage of this custom lightweight ProxyBuilder over Symfony's?

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.

Wim Leers’s picture

This 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:

  • this issue needs to profile the code that the custom proxybuilder generates; comparing it to the existing one. Profiling this is difficult; so it's really about comparing generated code, and verifying whether it's not more expensive, or whether it's acceptably more expensive.
  • we need a follow-up issue to investigate which services should be marked as lazy, and profile the results of that

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.

dawehner’s picture

this issue needs to profile the code that the custom proxybuilder generates; comparing it to the existing one. Profiling this is difficult; so it's really about comparing generated code, and verifying whether it's not more expensive, or whether it's acceptably more expensive.

Given 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.

Wim Leers’s picture

Issue tags: -needs profiling

Given that this would be mainly added to not often used services, we will have a hard time in seeing a lot.

Right! 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.

effulgentsia’s picture

Issue summary: View changes
Issue tags: -Needs issue summary update

Updated 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.

dawehner’s picture

Thank you @effulgentsia to update the issue summary!!

chx’s picture

Status: Needs review » Reviewed & tested by the community

I did review those implementations; several times over and I believe they are good so per #121 I think this is a go.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

This 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!

  • alexpott committed 2f628af on 8.0.x
    Issue #1973618 by dawehner, donquixote, plach, Mile23: DIC: Lazy...
Wim Leers’s picture

znerol’s picture

This has some weird side-effects on DrupalKernelTest.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

Fabianx’s picture

Really 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 :).

fgm’s picture

Issue summary: View changes