laravel / ideas

Issues board used for Laravel internals discussions.
938 stars 28 forks source link

A delayed event dispatcher that can also ignore specific events #984

Open Jalle19 opened 6 years ago

Jalle19 commented 6 years ago

The current event dispatcher has one weakness - it will call make() on all listeners and subscribers as soon as it is instantiated. This means that in an application where there are a lot of event subscribers, all subscribers will be "made" on each request, regardless of whether an event is actually dispatched. This increases the bootstrapping time considerably.

Additionally, some services will trigger events whether you want to or not, e.g. the cache repository which triggers CacheHit and CacheMiss events every time the cache is accessed. This means that even if you delay the construction of listeners and subscribers until an event is dispatched, you'll inevitable end up doing that anyway since these events are fairly common; for example if your application reads some configuration value from the cache on every request.

A proposed solution would be a "delayed, optional event dispatcher". Delaying listeners and subscribers is easy, we just keep track of them and resolve them once an event is dispatched. In order to solve the second problem (unwanted events triggering resolving of listeners/subscribers) the dispatcher should have a blacklist of events that it will not dispatch.

I have a prototype of such a dispatcher ready and will publish it on Packagist, I can link it here once that's done so you can get a more concrete understanding of what I'm trying to propose here.

sisve commented 6 years ago
  1. It sounds like something that can be published as a third-party package; why should this go into the framework?
  2. How much does this reduce the framework boot times? Do you have any measurements for this claim?
Jalle19 commented 6 years ago

@sisve thanks for your reply

  1. It can be a separate package, although I think it would be neat if the functionality was included since the interface doesn't have to change at all. I just pushed the initial version to https://github.com/Jalle19/laravel-deferred-event-dispatcher, the README contains a more detailed motivation for this than the post here.

  2. The gains can be quite substantial, in the application that spurred me to investigate this the gains were quite remarkable. The screenshots below are from the health check route, i.e. a route where the bootstrapping part is about 80% of the whole request time.

screenshot from 2018-02-02 11-10-33

screenshot from 2018-02-02 11-12-33

Additionally, while it's not a direct indication that something is faster, the size of an Xdebug profiler snapshot for this request went down from ~1000 KB to ~420 KB.

sisve commented 6 years ago

I'm not sure what I am looking at in your graph. I'm paranoid, and I believe that people will show graphs that includes all changes they did in a deploy, not their specific change they are arguing for. (This is based on previous life experiences...).

I don't understand why a health-check route would incur 20% cpu usage to begin with. It sounds like your events have lots of heavy dependencies.

Are these gains specific to your application, or have you been able to reproduce them in other open-source applications where we can confirm the numbers?

I'm pretty sure that I can create these graphs by having a dependency (for an non-existant event) that does a sleep(10); in the constructor. A delayed instantiation of the event listener would never create the listener, and never execute the sleep. Now, was that a framework problem, or an application problem?

There's downsides with a delayed evaluation; there's no feedback about missing dependencies until it becomes an exception in the production environment.

Jalle19 commented 6 years ago

I'm not sure what I am looking at in your graph. I'm paranoid, and I believe that people will show graphs that includes all changes they did in a deploy, not their specific change they are arguing for. (This is based on previous life experiences...).

I don't understand why a health-check route would incur 20% cpu usage to begin with. It sounds like your events have lots of heavy dependencies.

I used JMeter to simulate a lot of requests to the same route to get more accurate readings, that's why the CPU usage is fairly high.

Are these gains specific to your application, or have you been able to reproduce them in other open-source applications where we can confirm the numbers?

The application has a bunch of services that listen for created/updated/deleted events on a bunch of models, which means all those services have to be instantiated when the event service provider is registered. The overhead adds up after a while (we have about 10 event subscribers). In our case the slowest service provider to register was Doctrine and the related Gedmo extensions provider.

I'm pretty sure that I can create these graphs by having a dependency (for an non-existant event) that does a sleep(10); in the constructor. A delayed instantiation of the event listener would never create the listener, and never execute the sleep. Now, was that a framework problem, or an application problem?

I would say it's a framework problem, because registering a listener for a non-existing event is obviously unnecessary. The amount of work done during the bootstrap phase should IMO be minimized since it affects every single request to the application.

There's downsides with a delayed evaluation; there's no feedback about missing dependencies until it becomes an exception in the production environment.

There are many ways in which an application can crash only during certain scenarios, I don't think the framework should be protecting against that by doing as much as possible during the bootstrap phase.

vv12131415 commented 5 years ago

I don't understand why a health-check route would incur 20% cpu usage to begin with. It sounds like your events have lots of heavy dependencies.

But what if I need heavy dependencies in this specific subscriber? The other way to go is lazy loading of the services, but that, as I understood, halted/rejected as it said here https://github.com/LaravelBA/proxyvel/issues/2 The lazy loading is neat idea I think, I might open one of issues here about it.

There's downsides with a delayed evaluation; there's no feedback about missing dependencies until it becomes an exception in the production environment.

this is not a big problem, because you can say same for facades, it will crash only in runtime if there is no binding in the container for that facade.

vv12131415 commented 5 years ago

the other thing that you can do for deferring resolution of subscribers is something that will look like that

this one is easy to do

<?php

declare(strict_types=1);

namespace App\Events;

interface EventSubscriberInterface
{
    public static function getEventListenersList(): array;
}
<?php

declare(strict_types=1);

namespace App\Providers;

class DeferredEventServiceProvider extends \Illuminate\Foundation\Support\Providers\EventServiceProvider
{
    /** @var \App\Events\EventSubscriberInterface[] */
    protected $deferredSubscribers = [
        \App\Events\Subscribers\DeferredEventSubscriber::class
    ];

    public function boot(): void
    {
        parent::boot();
        foreach ($this->deferredSubscribers as $subscriber) {
            foreach ($subscriber::getEventListenersList() as $event => $listener) {
                \Illuminate\Support\Facades\Event::listen($event, $subscriber . '@' . $listener);
            }
        }
    }
}
<?php

declare(strict_types=1);

namespace App\Events\Subscribers;

class DeferredEventSubscriber implements \App\Events\EventSubscriberInterface
{
    /**
     * @return string[]
     */
    public static function getEventListenersList(): array
    {
        return [
            \App\Events\SomeEvent::class => 'onSomeEvent',
        ];
    }

    public function onSomeEvent(\App\Events\SomeEvent $event): void
    {
        //do something here
    }
}

that what I would like to have in framework, but that can also be in the package