[DI] Add "container.hot_path" tag to flag the hot path and inline related services #24872

nicolas-grekas · 2017-11-08T16:01:27Z

Q	A
Branch?	3.4
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	-
License	MIT
Doc PR	-

This PR is the result of my quest to squeeze some performance out of 3.4/4.0.

It builds on two ideas:

a new container.hot_path tag that identifies the services that are always needed. This tag is only applied to a very short list of bootstrapping services (router, event_dispatcher, http_kernel and request_stack only). Then, it is propagated to all dependencies of these services, with a special case for event listeners, where only listed events are propagated to their related listeners.
replacing the PHP autoloader by plain inlined require_once in generated service factories, with the benefit of completely bypassing the autoloader for services and their class hierarchy.

The end result is significant, even on a simple Hello World.
Here is the Blackfire profile, results are consistent with ab benchmarks:

https://blackfire.io/profiles/compare/b5fa5ef0-755c-4967-b990-572305f8f381/graph

lyrixx · 2017-11-08T17:06:11Z

I like it.

But i think the name container.inline is misleading. It can be confused with the service inlining ($instance = new A(new B())).

IIUC, here it inlines the factory in the dumped container + it pre-load the PHP Class.

nicolas-grekas · 2017-11-08T17:08:14Z

container.hot_path?
ppl shouldn't really use that anyway, because propagation is enough most of the time

lyrixx · 2017-11-08T17:19:30Z

container.hot_path?

Yes it's better. And like that the name of the tag will be the same as the CompilerPass ;)

ppl shouldn't really use that anyway, because propagation is enough most of the time

It depends. We already use some inlining feature in Blackfire. It could be useful anyway.

stof · 2017-11-08T17:33:15Z

src/Symfony/Bundle/FrameworkBundle/FrameworkBundle.php

+            KernelEvents::CONTROLLER_ARGUMENTS,
+            KernelEvents::RESPONSE,
+            KernelEvents::FINISH_REQUEST,
+            KernelEvents::TERMINATE,


I don't think terminate needs to be in the hot path. When using PHP-FPM, this code runs after sending the request.

stof · 2017-11-08T17:54:19Z

src/Symfony/Component/DependencyInjection/Dumper/PhpDumper.php

+            return;
+        }
+        try {
+            $r = new \ReflectionClass($class);


this might break if a parent class does not exist (ContainerBuilder is using a special autoloader to cover this when getting a tracked ReflectionClass)

stof · 2017-11-08T17:55:35Z

src/Symfony/Component/DependencyInjection/Dumper/PhpDumper.php

+            return;
+        }
+        $file = $r->getFileName();
+        if (!$file || $this->doExport($file) === $file = $this->export($file)) {


reassigning $file in the middle of an expression using it on both side makes the code quite hard to read. Please use a different variable name instead.

stof · 2017-11-08T18:28:48Z

src/Symfony/Bundle/FrameworkBundle/Resources/config/services.xml

        </service>
        <service id="Symfony\Component\HttpKernel\HttpKernelInterface" alias="http_kernel" />

-        <service id="request_stack" class="Symfony\Component\HttpFoundation\RequestStack" public="true" />
+        <service id="request_stack" class="Symfony\Component\HttpFoundation\RequestStack" public="true">
+            <tag name="container.hot_path" />


do we actually need this ? HttpKernel propagates it anyway (and if HttpKernel does not use the stack anymore, it is not hot anymore)

stof · 2017-11-08T18:29:26Z

src/Symfony/Component/EventDispatcher/DependencyInjection/RegisterListenersPass.php

@@ -40,6 +43,14 @@ public function __construct($dispatcherService = 'event_dispatcher', $listenerTa
        $this->subscriberTag = $subscriberTag;
    }

+    public function setHotPathListeners(array $hotPathListeners, $tagName = 'container.hot_path')


you are not setting listeners, you are setting events

stof · 2017-11-08T18:33:11Z

src/Symfony/Bundle/FrameworkBundle/FrameworkBundle.php

@@ -83,14 +84,23 @@ public function build(ContainerBuilder $container)
    {
        parent::build($container);

+        $hotPathListeners = array(


these are events, not listeners

stof · 2017-11-08T18:35:15Z

Tests are missing, for both the compiler pass and the new dumper behavior

nicolas-grekas · 2017-11-08T18:51:29Z

Now with a new container.dumper.inline_class_loader parameter to opt-in this optimization, so that ppl doing strange things with their autoloader are still fine.

@stof thanks, comments addressed.

nicolas-grekas · 2017-11-08T18:53:41Z

Just proposed to enable the parameter by default in symfony/recipes#241

nicolas-grekas · 2017-11-08T18:56:01Z

src/Symfony/Component/DependencyInjection/Dumper/PhpDumper.php

+        if (isset($lineage[$class])) {
+            return;
+        }
+        if (!$r = $this->container->getReflectionClass($class)) {


@stof this still throws, but in a more controlled way. Since this is running late, all services should be fine xor the container is broken already. Do we want really to not throw? WDYT?

beberlei · 2017-11-08T20:51:55Z

I feel container.inline is a little misleading, it feels something along the lines of container.require or container.preload is more correct naming wise. Otherwise i love the idea :)

Tobion · 2017-11-08T21:17:12Z

src/Symfony/Component/DependencyInjection/Dumper/PhpDumper.php

+            $code .= sprintf("        require_once %s;\n", $file);
+        }
+
+        if ("\n" === $code) {


how about ternary operator with return?

Tobion · 2017-11-08T21:23:50Z

src/Symfony/Component/DependencyInjection/Dumper/PhpDumper.php

@@ -114,10 +116,14 @@ public function dump(array $options = array())
            'namespace' => '',
            'as_files' => false,
            'debug' => true,
+            'hot_path_tag' => null,


Why does it not have a default?

I don't want it to be enabled by default, so this is opt-in.
This is the way to do it.

Tobion · 2017-11-08T21:24:36Z

src/Symfony/Component/DependencyInjection/Dumper/PhpDumper.php

@@ -1137,6 +1199,39 @@ private function addAliases()
        return $code."        );\n";
    }

+    private function addInlineRequires()
+    {
+        if (!$this->hotPathTag || !$this->inlineRequires) {


Why have two ways to disable this? Cant you just make hotPathTag required?

Because these are two independent settings (that play well together.)

Tobion · 2017-11-08T21:27:44Z

So the router is loaded always even in CLI where it is not used?

nicolas-grekas · 2017-11-08T21:36:41Z

The router classes are always loaded yes (not the objects). We don't care about the related CLI perf impact IMHO, it's negligible.

Tobion · 2017-11-08T22:02:57Z

Can't we preload the CLI classes as well then to speed up the CLI? If the impact of preloading unused classes is negligible, this shouldn't hurt webrequest as well.

nicolas-grekas · 2017-11-08T22:06:11Z

There's nothing to optimize for the CLI: the CLI is not called 700 times per second as the web is. Just forking the CLI process is an order of magnitude slower than these optimizations.

dmaicher · 2017-11-08T22:07:41Z

On my biggest monolith app I can also measure a slight performance benefit for one html page that I tested 👍

prod, debug=false
composer dump --classmap-authoritative && app/console cache:clear --env=prod --no-debug
php 7.1, opcache enabled
ab -n 10000 -c 1 "http://..."

your branch HEAD~1 (8cd2193): 41.112 ms
your commit (44fe030) with enabled feature: 40.289 ms

So for me its roughly 2% faster. Not quite as significant as in your benchmarks. Under which conditions did you benchmark @nicolas-grekas ?

nicolas-grekas · 2017-11-09T08:36:59Z

Now with tests, PR ready for a last (hopefully) round of review.

@dmaicher thanks for the confirmation! My test was on an annotated controller with a Twig-rendered Hello World, and using Blackfire to profile. Using ab, I'm more at 2% also.

nicolas-grekas · 2017-11-09T09:38:18Z

(failures are false-positives)

lyrixx · 2017-11-09T10:01:40Z

src/Symfony/Component/DependencyInjection/Dumper/PhpDumper.php

        $options = array_merge(array(
            'class' => 'ProjectServiceContainer',
            'base_class' => 'Container',
            'namespace' => '',
            'as_files' => false,
            'debug' => true,
+            'hot_path_tag' => null,
+            'inline_class_loader_parameter' => 'container.dumper.inline_class_loader',


Why is it configurable? Usually we don't want to add new options. IMHO, this is useless here.

I don't like hardcoding a parameter name into the dumper

fabpot · 2017-11-09T14:10:24Z

Can we make fabbot happy? Avoiding false-positives in the future would be great.

…ated services

nicolas-grekas · 2017-11-09T14:15:10Z

green it is now :)

fabpot · 2017-11-09T14:25:02Z

Thank you @nicolas-grekas.

…nd inline related services (nicolas-grekas) This PR was merged into the 3.4 branch. Discussion ---------- [DI] Add "container.hot_path" tag to flag the hot path and inline related services | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - This PR is the result of my quest to squeeze some performance out of 3.4/4.0. It builds on two ideas: - a new `container.inline` tag that identifies the services that are *always* needed. This tag is only applied to a very short list of bootstrapping services (`router`, `event_dispatcher`, `http_kernel` and `request_stack` only). Then, it is propagated to all dependencies of these services, with a special case for event listeners, where only listed events are propagated to their related listeners. - replacing the PHP autoloader by plain inlined `require_once` in generated service factories, with the benefit of completely bypassing the autoloader for services and their class hierarchy. The end result is significant, even on a simple Hello World. Here is the Blackfire profile, results are consistent with `ab` benchmarks: https://blackfire.io/profiles/compare/b5fa5ef0-755c-4967-b990-572305f8f381/graph ![capture du 2017-11-08 16-54-28](https://user-images.githubusercontent.com/243674/32558666-a3f439b2-c4a5-11e7-83a3-db588c3e21e5.png) Commits ------- f7cb559 [DI] Add "container.hot_path" tag to flag the hot path and inline related services

This PR was merged into the 3.4 branch. Discussion ---------- Add container.hot_path to built-in service tags Related to [symfony/symfony#24872](symfony/symfony#24872)  Commits ------- 1647b9a Add container.hot_path to built-in service tags

nicolas-grekas added the Performance label Nov 8, 2017

nicolas-grekas added this to the 3.4 milestone Nov 8, 2017

javiereguiluz added the DependencyInjection label Nov 8, 2017

nicolas-grekas force-pushed the optim branch 2 times, most recently from fcb8603 to b028e59 Compare November 8, 2017 16:30

nicolas-grekas force-pushed the optim branch from b028e59 to d6350a3 Compare November 8, 2017 18:10

stof reviewed Nov 8, 2017

View reviewed changes

nicolas-grekas force-pushed the optim branch from d6350a3 to be2cb66 Compare November 8, 2017 18:50

nicolas-grekas mentioned this pull request Nov 8, 2017

Enable container.dumper.inline_class_loader param by default symfony/recipes#241

Merged

nicolas-grekas commented Nov 8, 2017

View reviewed changes

nicolas-grekas force-pushed the optim branch from be2cb66 to eb45444 Compare November 8, 2017 18:59

nicolas-grekas changed the title ~~[DI] Add "container.inline" tag to flag the hot path and inline related services~~ [DI] Add "container.hot_path" tag to flag the hot path and inline related services Nov 8, 2017

nicolas-grekas force-pushed the optim branch from eb45444 to 44fe030 Compare November 8, 2017 20:47

Tobion reviewed Nov 8, 2017

View reviewed changes

nicolas-grekas force-pushed the optim branch from 44fe030 to 9780864 Compare November 8, 2017 21:22

Tobion reviewed Nov 8, 2017

View reviewed changes

nicolas-grekas force-pushed the optim branch from 9780864 to 17e6341 Compare November 8, 2017 21:56

nicolas-grekas force-pushed the optim branch from 17e6341 to 64abf2e Compare November 9, 2017 08:33

nicolas-grekas force-pushed the optim branch 4 times, most recently from 00e0c23 to e20e523 Compare November 9, 2017 09:25

nicolas-grekas force-pushed the optim branch from e20e523 to 3358faf Compare November 9, 2017 09:45

lyrixx reviewed Nov 9, 2017

View reviewed changes

xabbuh mentioned this pull request Nov 9, 2017

enable container.autowiring.strict_mode symfony/symfony-standard#1114

Merged

[DI] Add "container.hot_path" tag to flag the hot path and inline rel…

f7cb559

…ated services

nicolas-grekas force-pushed the optim branch from 3358faf to f7cb559 Compare November 9, 2017 14:14

carsonbot added the Status: Reviewed label Nov 9, 2017

fabpot approved these changes Nov 9, 2017

View reviewed changes

chalasr approved these changes Nov 9, 2017

View reviewed changes

fabpot merged commit f7cb559 into symfony:3.4 Nov 9, 2017

nicolas-grekas deleted the optim branch November 9, 2017 15:05

glickel mentioned this pull request Nov 24, 2019

Add container.hot_path to built-in service tags symfony/symfony-docs#12684

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DI] Add "container.hot_path" tag to flag the hot path and inline related services #24872

[DI] Add "container.hot_path" tag to flag the hot path and inline related services #24872

nicolas-grekas commented Nov 8, 2017 •

edited

Loading

lyrixx commented Nov 8, 2017

nicolas-grekas commented Nov 8, 2017

lyrixx commented Nov 8, 2017

stof Nov 8, 2017

stof Nov 8, 2017

stof Nov 8, 2017

stof Nov 8, 2017

stof Nov 8, 2017

stof Nov 8, 2017

stof commented Nov 8, 2017

nicolas-grekas commented Nov 8, 2017

nicolas-grekas commented Nov 8, 2017

nicolas-grekas Nov 8, 2017

beberlei commented Nov 8, 2017

Tobion Nov 8, 2017

Tobion Nov 8, 2017

nicolas-grekas Nov 8, 2017

Tobion Nov 8, 2017 •

edited

Loading

nicolas-grekas Nov 8, 2017

Tobion commented Nov 8, 2017

nicolas-grekas commented Nov 8, 2017

Tobion commented Nov 8, 2017

nicolas-grekas commented Nov 8, 2017 •

edited

Loading

dmaicher commented Nov 8, 2017 •

edited

Loading

nicolas-grekas commented Nov 9, 2017

nicolas-grekas commented Nov 9, 2017

lyrixx Nov 9, 2017

nicolas-grekas Nov 9, 2017

fabpot commented Nov 9, 2017

nicolas-grekas commented Nov 9, 2017

fabpot commented Nov 9, 2017

[DI] Add "container.hot_path" tag to flag the hot path and inline related services #24872

[DI] Add "container.hot_path" tag to flag the hot path and inline related services #24872

Conversation

nicolas-grekas commented Nov 8, 2017 • edited Loading

lyrixx commented Nov 8, 2017

nicolas-grekas commented Nov 8, 2017

lyrixx commented Nov 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stof commented Nov 8, 2017

nicolas-grekas commented Nov 8, 2017

nicolas-grekas commented Nov 8, 2017

Choose a reason for hiding this comment

beberlei commented Nov 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tobion Nov 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tobion commented Nov 8, 2017

nicolas-grekas commented Nov 8, 2017

Tobion commented Nov 8, 2017

nicolas-grekas commented Nov 8, 2017 • edited Loading

dmaicher commented Nov 8, 2017 • edited Loading

nicolas-grekas commented Nov 9, 2017

nicolas-grekas commented Nov 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabpot commented Nov 9, 2017

nicolas-grekas commented Nov 9, 2017

fabpot commented Nov 9, 2017

nicolas-grekas commented Nov 8, 2017 •

edited

Loading

Tobion Nov 8, 2017 •

edited

Loading

nicolas-grekas commented Nov 8, 2017 •

edited

Loading

dmaicher commented Nov 8, 2017 •

edited

Loading