Write a more scalable dispatcher

The model I'd been looking into before for this was to have it as a separate object from the container. Basically it would compile to a container-aware object vaguely like this, where dispatch('foo', $event) would map to:

public function dispatch($name, $event) {
  $method = "dispatch_" . $name;
  return $this->$method($name, $event);
}

public function dispatch_foo($name, $event) {
  $this->container->get('subscriber_1')->knownMethod($event);
  $this->container->get('subscriber_1')->someMethod($event);
  $this->container->get('subscriber_1')->aMethod($event);
  return $event;
}

(The dispatch_foo() method would do more than that, like checking for stopPropagation(), but that's the basic idea.)

That should be a separate object from the container, but you can easily put the compiled class into the container instead of the uncompiled one, and then neither the container nor other code would be any the wiser.

Simple example for now.

1. copy-paste ContainerAwareEventDispatcher into the Drupal namespace

Please no, if we really need to rewrite the class, then let's not inherit from EventDispatcher and just implement the interface. The git history of ContainerAwareEventDispatcher reveals that this is not the most polished piece of code inside Symfony and therefore it is not a good starting point for a better implementation.

4. Change RegisterEventSubscribers::process so that it doesn't add a method call but instead inlines the relevant parts of addSubscriberService to gather the listener ids.

It seems to me that with this patch we will lose the ability to add new listeners ad-hoc (via code) and that violates the event-dispatcher contract. If we do that, we need really good justification. If we want to keep class loading to a minimum there would be the option to not use addSubscriberService but rather addListenerService. This will then not result in a static invocation of the subscriber each time the event-dispatcher is instantiated.

Finally I'd like to humbly point to my version of the ContainerAwareEventDispatcher.php (because this is easier to read than the patch) which does still comply with the contract but cuts the time necessary to get to the first request-event listener to half (in the standard profile).

So, znerol wrote over there:

• Standard ContainerAwareEventDispatcher: Time to first request-listener: ~130ms, number of instantiated services: 132 of 369
• Modified ContainerAwareEventDispatcher: Time to first request-listener: ~70ms, number of instantiated services: 36 of 369

I interpret this as: this optimization makes Drupal requests up to 60 ms/page load faster, depending on how many services are actually used. Is this correct? If so, this is HUGE.

This is correct, but only for early request listeners. For example if you set a redirect response way earlier than the router kicks in. It makes no difference for the last-request listener and anything following from there on.

@znerol: what's your total server response time for the front page (which is heavy), the /contact page (which is light) and the /system/ajax page (without arguments, so yes, this will show an error, but it's a very light route)? That would provide some perspective on those numbers. Preferably both with and without your patch.

My patch does not influence the runtime of full-page requests (i.e. requests going through the router and controllers). The only thing it does is that services are instantiated as needed when dispatching an event. This only has an effect if a listener stops event propagation early.

I'm referring to EventDispatcherInterface::addListener().

Initially I was not sure whether this change really makes a noticeable difference performance wise. Therefore I did put together i small module which is capable of generating as much noop-subscribers as desired. The linked sandbox also provides a script to test the effects via the command line.

I performed the test 11 times with 0, 8, 16, 32, 64, 128, 256, 512, 1024, 2048, 4096 generated subscribers once for vanilla drupal and once with chx patch from #20 applied.

The performance gain realized by replacing the method calls by an array during service construction are massive:

        HEAD [ms]   Chx 20 [ms]
62      8.9296      0.6676
70      9.1120      0.6502
78      9.2026      0.6526
94      9.3840      0.6390
126     9.5620      0.6864
190     10.0585     0.7179
318     10.9575     0.7895
574     12.8790     0.9475
1086    16.7350     1.2685
2110    24.4159     1.9675
4158    40.6904     3.3300

The performance gains of the optimized dispatch() function are not that spectacular but still noteworthy.

initial call to dispatch

        HEAD [ms]   Chx 20 [ms]
0       0.1196      0.1230
8       0.3780      0.4220
16      0.5679      0.5279
32      0.9900      0.7349
64      1.7204      1.1519
128     3.2570      1.9960
256     6.3459      3.7090
512     12.3584     7.0381
1024    24.6564     13.8191
2048    49.8090     27.3554
4096    114.6154    54.2390

subsequent call to dispatch

        HEAD [ms]   Chx 20 [ms]
0       0.0081      0.0060
8       0.1365      0.0565
16      0.2439      0.1040
32      0.4859      0.1919
64      0.9209      0.3749
128     1.8065      0.7195
256     3.5678      1.4355
512     7.1014      2.8760
1024    14.2070     5.8249
2048    29.8120     11.9214
4096    74.6585     23.8910

1. +++ b/core/lib/Drupal/Core/EventSubscriber/ContainerAwareEventDispatcher.php
   @@ -0,0 +1,213 @@
   +  public function removeListener($eventName, $listener) {
   

   Needs docblock.

2. +++ b/core/lib/Drupal/Core/EventSubscriber/ContainerAwareEventDispatcher.php
   @@ -0,0 +1,213 @@
   +        $this->listenerIds[$eventName][] = array(
   +          $serviceId,
   +          $params[0],
   +          isset($params[1]) ? $params[1] : 0
   +        );
   

   If this is in our own code, can we make this a keyed array rather than positional? That lets us avoid the list() call below and makes the code more self-documenting. (If it has to be the same format as something in Symfony, then :-( )

znerol's benchmarks are compelling. However, there's 2 questions outstanding that I have:

1) As above, does this preclude adding arbitrary callable listeners at arbitrary times? I don't think it does but I want to be sure.

2) It looks from the code (unless I'm misreading it entirely) like the performance boost comes mostly from pushing more work to the container compile time. Classy, but nothing Drupal-specific. Could any of this be pushed upstream to Symfony, so it would benefit us and everyone else?

I've no idea what is "common" for Symfony projects vis a vis non-container listeners. It's worth asking someone. I don't know why you'd need to muck with private vs protected, but if there's a valid reason to do so there are protected methods in Symfony already.

It would be great if someone could review / redo the benchmark. It would be stupid if we'd base our decisions on bogus numbers.

Re #22.2: It looks from this comment on a pull request proposing a similar optimization, that the current Symfony design is on purpose. This was done such that code-changes in event-subscribers get picked up by the application without needing to recompile the container and clearing the cache.

This was done such that code-changes in event-subscribers get picked up by the application without needing to recompile the container and clearing the cache.

I'd be very fine with not supporting that feature in Drupal.

I'm working on this. My plan is to write a standalone version of the ContainerAwareEventDispatcher. Specifically it should:

1. Accept a list of ordered definitions [serviceid, method] for lazy loaded subscribers/listeners (cut the service construction time)
2. Implement a fast version of dispatch()
3. Implement the EventDispatcherInterface (support adding/removing listeners at runtime).

Also I propose to drop addSubscriberService and addListenerService because those methods do not make much sense outside of the compiler pass.

Very rough initial version. This includes chx compiler pass. Next step is to see whether symfony unit-tests pass and then I will do some benchmarks.

Hum, 0 bytes, not good.

This one passes symfony EventDispatcherTest.

+++ b/core/lib/Drupal/Component/EventDispatcher/ContainerAwareEventDispatcher.php
@@ -0,0 +1,199 @@
+          call_user_func($definition['callable'], $event, $event_name, $this);

Micro-optimization: $function is faster than call_user_func() and just as capable.

Oh, I did not know that this works with callables of the form array($object, 'method') but apparently it does:

class C {
  function m() {
    error_log('works');
  }
}
$f = [new C, 'm'];
$f();

The performance test yields comparable results to chx version with this changes:

1. Actually mark the listeners as sorted when they were sorted
2. Implement #33
3. Cache callable after resolving a service

Might be worth moving the nested loops into a functional-style each method, such that we can reuse it in all three places. This comes with a slight performance cost, but it is still quite a bit faster than HEAD.

Also the removeListener now trigger instantiation of all listeners. I do not expect that to be a problem because I think it makes most sense to alter the container service listeners/subscribers at compile time. Also that is no different than in HEAD, its just one optimization less for the benefit of a more consistent implementation.

Updated performance test including the patches in 35 and 36, also attaching the spreadsheet with the raw numbers.

event subscriber service construction

        HEAD [ms]   Chx 20 [ms] Zn 35 [ms]  Zn 36 [ms]
62       8.9296     0.6676      0.4485      0.4580
70       9.1120     0.6502      0.4580      0.4655
78       9.2026     0.6526      0.4506      0.4520
94       9.3840     0.6390      0.4680      0.4715
126      9.5620     0.6864      0.5100      0.5140
190     10.0585     0.7179      0.5310      0.5380
318     10.9575     0.7895      0.6189      0.6249
574     12.8790     0.9475      0.8440      0.8606
1086    16.7350     1.2685      1.2381      1.2605
2110    24.4159     1.9675      2.0260      2.0900
4158    40.6904     3.3300      3.6474      3.8096

initial call to dispatch

        HEAD [ms]   Chx 20 [ms] Zn 35 [ms]  Zn 36 [ms]
0         0.1196     0.1230      0.1180      0.1221
8         0.3780     0.4220      0.4325      0.4654
16        0.5679     0.5279      0.5615      0.5990
32        0.9900     0.7349      0.8039      0.8615
64        1.7204     1.1519      1.3044      1.4040
128       3.2570     1.9960      2.2550      2.4959
256       6.3459     3.7090      4.2239      4.6500
512      12.3584     7.0381      8.0614      8.8689
1024     24.6564    13.8191     15.8570     17.4510
2048     49.8090    27.3554     31.5109     34.5354
4096    114.6154    54.2390     66.5710     72.1431

subsequent call to dispatch

        HEAD [ms]   Chx 20 [ms] Zn 35 [ms]   Zn 36 [ms]
0        0.0081      0.0060      0.0060      0.0060
8        0.1365      0.0565      0.0229      0.0376
16       0.2439      0.1040      0.0370      0.0631
32       0.4859      0.1919      0.0641      0.1075
64       0.9209      0.3749      0.1190      0.1994
128      1.8065      0.7195      0.2291      0.3921
256      3.5678      1.4355      0.4435      0.7646
512      7.1014      2.8760      0.9025      1.5122
1024    14.2070      5.8249      1.9932      3.2442
2048    29.8120     11.9214      4.3085      6.7981
4096    74.6585     23.8910     12.4741     17.3085

+      $this->eachListener($event_name, function($callable) use ($event, $event_name, $event_dispatcher) {
+        $callable($event, $event_name, $event_dispatcher);
+        if ($event->isPropagationStopped()) {
+          return FALSE;
+        }
+      });

This can definitely be rewritten to look like PHP.

Re #39 certainly. #35 and #36 are equivalent, the only difference is the loop style. The problem is that I'm not comfortable with the duplicated code in #35 and frankly I'm a bit stuck on how to resolve that without introducing a performance regression in dispatch(). Note that reusing getListeners() is out of question because of reasons illustrated in Symfony issue 12007.

Ok, let's drop #36. This is #35 with better documentation and the event dispatcher unit test from Symfony.

I hope that the Symfony pr 12131 eventually lands, this will allow us to subclass the unit tests instead of copy pasting them.

Invert your sorted logic please.

Of course!

RTBC - if chx agrees this is ready now.

Patch looked great in my review!

Chx is happy with this, too.

Therefore after my review and with this great benchmarks !!! RTBC :).

Thanks @Fabianx, but #25 is not done yet. This is such an intrusive change, I'd prefer if someone would peer-review/reality-check the benchmarks before this gets committed.

How did you perform the benchmarks then?

See #21, there is a sandbox linked containing a module capable of generating X subscriber services. It a also would be valuable if the performance test would be carried out using another method in order to reality-check the results.

This even saves around 2-4% on a stock Drupal site measured via XHProf.

Also algorithmic this makes a lot of sense and this unblocks other issues, so is not only about performance.

Cool, thanks Fabianx!

This even saves around 2-4% on a stock Drupal site measured via XHProf.

For "regular" page loads? i.e. front page, nodes, etc?

Just the normal front page, saves 2% of all function calls for that simple scenario - which is 1000 function calls.

So this does have a pretty big real-world performance impact on a simple Drupal page (1000 less function calls for a mere 6 dispatched events!). As the number of dispatched events and event listeners increases, this number is likely to rise further.

This is kind of scary. It makes me wonder about the performance of other Symfony components. Especially because big parts of our critical path are now Symfony.

Future readers, also see https://github.com/symfony/symfony/pull/12019 for the upstream discussion.

Saving 2–4% is huge at this point in the cycle, so: great work, znerol! Thanks!

The improvement mostly stems from the refactored service construction. The dispatch() call alone is not a big deal.

Looking at the compiled container, I can identify two additional services we might want to examine closer (i.e. many method calls during service construction): access_manager and plugin.cache_clearer. I hope the latter is not used on every request though.

I think that "classic" Symfony applications mostly are business oriented software that use Symfony as a framework whereas Drupal is a generic all-purpose highly complex application. I think that in most classic Symfony applications the number of various listeners and services is very low compared to a stock Drupal. This kind of performance consideration in most cases probably is insignificant. What's scary isn't Symfony components but the usage Drupal does of them.

plugin.cache_clearer should only by used in module install/uninstall() and cache clears.

#65 I only did the benchmarks - It is chx's and znerols genius.

Maybe we should open a major meta child of "resolve known perf reg. in D8" to audit the symfony components we are using for performance?

#65: Indeed. My wording was bad. Symfony's components might be perfectly fine for Symfony. But our usage patterns are very different, so it's very well possible that some Symfony components simply aren't a good match.

On the wider question of Symfony components. I agree that it comes down to a case of 'just not a good match' most of the time - i.e. that those components are written for use cases that never get close to the scale our subsystems have to. In those situations we have to be very, very careful whenever we apply them, and we've not been careful enough so far. Still time to fix that though, and this patch and the performance results from it are fantastic (although still need to review in more depth before I feel comfortable committing). The fact we get these performance numbers despite me individually pushing back on almost every introduction of events in core is worrying though- we'd have many, many more events by now without that push back.

I would note that while it's OK to say things just aren't a good match now, the initial adoption of Symfony components was also enthusiastically encouraged from the Symfony side (both technical and commercial). So when I see upstream PRs rejected or stalled that allow those components to meet our use cases and requirements I get increasingly concerned. Eventually we'll end up using only a few interfaces and very little runtime Symfony code.

+++ b/core/lib/Drupal/Component/EventDispatcher/ContainerAwareEventDispatcher.php
@@ -0,0 +1,238 @@
+            $definition['callable'] = [$this->container->get($definition['service'][0]), $definition['service'][1]];

Isn't this incompatible with marking listeners as not public in the container? This was discussed elsewhere.

It's a shame we have to copy the unit test rather than subclass, hopefully that upstream PR at least for the test will get in.

I didn't do my own profiling but trust between znerol and FabianX on the numbers, the concepts here look great and it's good we managed to come up with a completely compatible dispatcher without compromising.

Leaving RTBC for that one question.

Isn't this incompatible with marking listeners as not public in the container?

Indeed, it is. Any container-aware service is affected by that. Lazy-loading is incompatible with non-public services. Issue is: #2350545: Protect some services from being referenced from outside container.

@znerol thanks for the confirmation.

That's unfortunate - it means that once this patch lands we'll be stuck with event listeners showing up in IDE autocomplete with no way to remove them.

However a longer autocomplete list for IDEs vs. a base performance improvement of 2-4% is an OK trade-off for me. We may need to annotate event listeners to make it explicit they aren't part of the API, even if they're available via container get.

No worse than hook implementations getting called directly as functions, but not an improvement.

The performance issue would get significantly worse if we'd converted more hooks (or added #1972304: Add a HookEvent). Now we've fixed part of the performance issue, at a DX cost if there's more conversions/listeners, good reason to think carefully before going further along the Symfony events route.

Also I had quick look through EventDispatcher and I think we're only using the Event class and maybe a couple dozen other lines of non-interface code now. Not great but if you 'get off the island' and find out you're flying to a prison colony, subclassing/re-implementation of the interface is OK for an emergency landing.

Committed/pushed to 8.0.x, thanks!

That's unfortunate - it means that once this patch lands we'll be stuck with event listeners showing up in IDE autocomplete with no way to remove them.

Any container aware service is affected, including Symfonys ContainerAwareEventDispatcher we used before.

Yeah not blaming the patch, just unfortunate in general.

edit: actually reading back I was blaming the patch, but wrongly so. I'd not come to the realisation that ContainerAwareEventDispatcher prevents this until now.

Yeah, and not even that difficult to add a $container->injectMe('private_service') class, which calls injectMe('private_service', theService) or whatever.

I still think neither setter DI nor constructor DI are enough for our extended use cases ...

Here is the small update on the upstream PR [EventDispatcher] Add Drupal EventDispatcher is closed in favor of [DI][EventDispatcher] Add & wire closure-proxy argument type which is reverted in [Di] Remove closure-proxy arguments and symfony added [EventDispatcher] Handle laziness internally instead of relying on ClosureProxyArgument in symfony 3.3. Over in #2874909-52: Update Symfony components to 3.3.* I'm trying to fix the EventDispatcher fails. Can we drop Drupal EventDispatcher in favour of Symfony EventDispatcher now?

Created #2909185: Replace ContainerAwareEventDispatcher with Symfony EventDispatcher for further discussion.