feature #36535 [DI] skip preloading dependencies of non-preloaded services (nicolas-grekas)

fabpot · fabpot · commit ac3bd146a389 · 2020-04-24T10:23:03.000+02:00
This PR was merged into the 5.1-dev branch. Discussion ---------- [DI] skip preloading dependencies of non-preloaded services | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | Deprecations? | no | Tickets | - | License | MIT | Doc PR | - Suggested by @stof on Slack: this improves preloading by propagating the `container.no_preload` tag to services that are referenced only by not-preloaded services. The benefit is double: 1. this fixes potential over-preloading 2. this requires less work from the community: no need to add the tag anymore most of the time As a corollary, listeners of console events are tagged with `container.no_preload` automatically now. Commits ------- add8670 [DI] skip preloading dependencies of non-preloaded services
diff --git a/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php b/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php
@@ -436,8 +436,7 @@ public function load(array $configs, ContainerBuilder $container)
         $container->registerForAutoconfiguration(CacheClearerInterface::class)
             ->addTag('kernel.cache_clearer');
         $container->registerForAutoconfiguration(CacheWarmerInterface::class)
-            ->addTag('kernel.cache_warmer')
-            ->addTag('container.no_preload');
+            ->addTag('kernel.cache_warmer');
         $container->registerForAutoconfiguration(EventSubscriberInterface::class)
             ->addTag('kernel.event_subscriber');
         $container->registerForAutoconfiguration(LocaleAwareInterface::class)
diff --git a/src/Symfony/Bundle/FrameworkBundle/FrameworkBundle.php b/src/Symfony/Bundle/FrameworkBundle/FrameworkBundle.php
@@ -34,6 +34,7 @@
 use Symfony\Component\Cache\DependencyInjection\CachePoolPass;
 use Symfony\Component\Cache\DependencyInjection\CachePoolPrunerPass;
 use Symfony\Component\Config\Resource\ClassExistenceResource;
+use Symfony\Component\Console\ConsoleEvents;
 use Symfony\Component\Console\DependencyInjection\AddConsoleCommandPass;
 use Symfony\Component\DependencyInjection\Compiler\PassConfig;
 use Symfony\Component\DependencyInjection\Compiler\RegisterReverseContainerPass;
@@ -103,13 +104,21 @@ public function build(ContainerBuilder $container)
     {
         parent::build($container);
 
-        $hotPathEvents = [
+        $registerListenersPass = new RegisterListenersPass();
+        $registerListenersPass->setHotPathEvents([
             KernelEvents::REQUEST,
             KernelEvents::CONTROLLER,
             KernelEvents::CONTROLLER_ARGUMENTS,
             KernelEvents::RESPONSE,
             KernelEvents::FINISH_REQUEST,
-        ];
+        ]);
+        if (class_exists(ConsoleEvents::class)) {
+            $registerListenersPass->setNoPreloadEvents([
+                ConsoleEvents::COMMAND,
+                ConsoleEvents::TERMINATE,
+                ConsoleEvents::ERROR,
+            ]);
+        }
 
         $container->addCompilerPass(new LoggerPass(), PassConfig::TYPE_BEFORE_OPTIMIZATION, -32);
         $container->addCompilerPass(new RegisterControllerArgumentLocatorsPass());
@@ -118,7 +127,7 @@ public function build(ContainerBuilder $container)
         $container->addCompilerPass(new ProfilerPass());
         // must be registered before removing private services as some might be listeners/subscribers
         // but as late as possible to get resolved parameters
-        $container->addCompilerPass((new RegisterListenersPass())->setHotPathEvents($hotPathEvents), PassConfig::TYPE_BEFORE_REMOVING);
+        $container->addCompilerPass($registerListenersPass, PassConfig::TYPE_BEFORE_REMOVING);
         $this->addCompilerPassIfExists($container, AddConstraintValidatorsPass::class);
         $container->addCompilerPass(new AddAnnotationsCachedReaderPass(), PassConfig::TYPE_AFTER_REMOVING, -255);
         $this->addCompilerPassIfExists($container, AddValidatorInitializersPass::class);
diff --git a/src/Symfony/Bundle/FrameworkBundle/Resources/config/annotations.xml b/src/Symfony/Bundle/FrameworkBundle/Resources/config/annotations.xml
@@ -34,7 +34,6 @@
         </service>
 
         <service id="annotations.cache_warmer" class="Symfony\Bundle\FrameworkBundle\CacheWarmer\AnnotationsCacheWarmer">
-            <tag name="container.no_preload" />
             <argument type="service" id="annotations.reader" />
             <argument>%kernel.cache_dir%/annotations.php</argument>
             <argument>#^Symfony\\(?:Component\\HttpKernel\\|Bundle\\FrameworkBundle\\Controller\\(?!.*Controller$))#</argument>
diff --git a/src/Symfony/Bundle/FrameworkBundle/Resources/config/cache_debug.xml b/src/Symfony/Bundle/FrameworkBundle/Resources/config/cache_debug.xml
@@ -20,7 +20,6 @@
                 <argument>cache.serializer</argument>
             </argument>
             <tag name="kernel.cache_warmer" priority="64" />
-            <tag name="container.no_preload" />
         </service>
     </services>
 </container>
diff --git a/src/Symfony/Bundle/FrameworkBundle/Resources/config/console.xml b/src/Symfony/Bundle/FrameworkBundle/Resources/config/console.xml
@@ -11,12 +11,10 @@
             <argument type="service" id="logger" on-invalid="null" />
             <tag name="kernel.event_subscriber" />
             <tag name="monolog.logger" channel="console" />
-            <tag name="container.no_preload" />
         </service>
 
         <service id="console.suggest_missing_package_subscriber" class="Symfony\Bundle\FrameworkBundle\EventListener\SuggestMissingPackageSubscriber">
             <tag name="kernel.event_subscriber" />
-            <tag name="container.no_preload" />
         </service>
 
         <service id="console.command.about" class="Symfony\Bundle\FrameworkBundle\Command\AboutCommand">
diff --git a/src/Symfony/Bundle/FrameworkBundle/Resources/config/routing.xml b/src/Symfony/Bundle/FrameworkBundle/Resources/config/routing.xml
@@ -101,7 +101,6 @@
         <service id="router.cache_warmer" class="Symfony\Bundle\FrameworkBundle\CacheWarmer\RouterCacheWarmer">
             <tag name="container.service_subscriber" id="router" />
             <tag name="kernel.cache_warmer" />
-            <tag name="container.no_preload" />
             <argument type="service" id="Psr\Container\ContainerInterface" />
         </service>
 
diff --git a/src/Symfony/Bundle/FrameworkBundle/Resources/config/serializer.xml b/src/Symfony/Bundle/FrameworkBundle/Resources/config/serializer.xml
@@ -113,7 +113,6 @@
             <argument type="collection" /><!-- Loaders injected by the extension -->
             <argument>%serializer.mapping.cache.file%</argument>
             <tag name="kernel.cache_warmer" />
-            <tag name="container.no_preload" />
         </service>
 
         <service id="serializer.mapping.cache.symfony" class="Psr\Cache\CacheItemPoolInterface">
diff --git a/src/Symfony/Bundle/FrameworkBundle/Resources/config/translation.xml b/src/Symfony/Bundle/FrameworkBundle/Resources/config/translation.xml
@@ -139,7 +139,6 @@
         <service id="translation.warmer" class="Symfony\Bundle\FrameworkBundle\CacheWarmer\TranslationsCacheWarmer">
             <tag name="container.service_subscriber" id="translator" />
             <tag name="kernel.cache_warmer" />
-            <tag name="container.no_preload" />
             <argument type="service" id="Psr\Container\ContainerInterface" />
         </service>
     </services>
diff --git a/src/Symfony/Bundle/FrameworkBundle/Resources/config/validator.xml b/src/Symfony/Bundle/FrameworkBundle/Resources/config/validator.xml
@@ -36,7 +36,6 @@
             <argument type="service" id="validator.builder" />
             <argument>%validator.mapping.cache.file%</argument>
             <tag name="kernel.cache_warmer" />
-            <tag name="container.no_preload" />
         </service>
 
         <service id="validator.mapping.cache.adapter" class="Symfony\Component\Cache\Adapter\PhpArrayAdapter">
diff --git a/src/Symfony/Bundle/FrameworkBundle/composer.json b/src/Symfony/Bundle/FrameworkBundle/composer.json
@@ -21,6 +21,7 @@
         "symfony/cache": "^4.4|^5.0",
         "symfony/config": "^5.0",
         "symfony/dependency-injection": "^5.1",
+        "symfony/event-dispatcher": "^5.1",
         "symfony/error-handler": "^4.4.1|^5.0.1",
         "symfony/http-foundation": "^4.4|^5.0",
         "symfony/http-kernel": "^5.0",
diff --git a/src/Symfony/Bundle/SecurityBundle/Resources/config/security.xml b/src/Symfony/Bundle/SecurityBundle/Resources/config/security.xml
@@ -213,7 +213,6 @@
         <!-- Cache Warmers -->
         <service id="security.cache_warmer.expression" class="Symfony\Bundle\SecurityBundle\CacheWarmer\ExpressionCacheWarmer">
             <tag name="kernel.cache_warmer" />
-            <tag name="container.no_preload" />
             <argument type="collection" /> <!-- expressions -->
             <argument type="service" id="security.expression_language" />
         </service>
diff --git a/src/Symfony/Bundle/TwigBundle/Resources/config/twig.xml b/src/Symfony/Bundle/TwigBundle/Resources/config/twig.xml
@@ -45,7 +45,6 @@
 
         <service id="twig.template_cache_warmer" class="Symfony\Bundle\TwigBundle\CacheWarmer\TemplateCacheWarmer">
             <tag name="kernel.cache_warmer" />
-            <tag name="container.no_preload" />
             <tag name="container.service_subscriber" id="twig" />
             <argument type="service" id="Psr\Container\ContainerInterface" />
             <argument type="service" id="twig.template_iterator" />
diff --git a/src/Symfony/Component/DependencyInjection/Compiler/PassConfig.php b/src/Symfony/Component/DependencyInjection/Compiler/PassConfig.php
@@ -92,6 +92,7 @@ public function __construct()
         $this->afterRemovingPasses = [[
             new CheckExceptionOnInvalidReferenceBehaviorPass(),
             new ResolveHotPathPass(),
+            new ResolveNoPreloadPass(),
         ]];
     }
 
diff --git a/src/Symfony/Component/DependencyInjection/Compiler/ResolveNoPreloadPass.php b/src/Symfony/Component/DependencyInjection/Compiler/ResolveNoPreloadPass.php
@@ -0,0 +1,100 @@
+<?php
+
+/*
+ * This file is part of the Symfony package.
+ *
+ * (c) Fabien Potencier <fabien@symfony.com>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Symfony\Component\DependencyInjection\Compiler;
+
+use Symfony\Component\DependencyInjection\ContainerBuilder;
+use Symfony\Component\DependencyInjection\Definition;
+use Symfony\Component\DependencyInjection\Reference;
+
+/**
+ * Propagate the "container.no_preload" tag.
+ *
+ * @author Nicolas Grekas <p@tchwork.com>
+ */
+class ResolveNoPreloadPass extends AbstractRecursivePass
+{
+    private const DO_PRELOAD_TAG = '.container.do_preload';
+
+    private $tagName;
+    private $resolvedIds = [];
+
+    public function __construct(string $tagName = 'container.no_preload')
+    {
+        $this->tagName = $tagName;
+    }
+
+    /**
+     * {@inheritdoc}
+     */
+    public function process(ContainerBuilder $container)
+    {
+        $this->container = $container;
+
+        try {
+            foreach ($container->getDefinitions() as $id => $definition) {
+                if ($definition->isPublic() && !$definition->isPrivate() && !isset($this->resolvedIds[$id])) {
+                    $this->resolvedIds[$id] = true;
+                    $this->processValue($definition, true);
+                }
+            }
+
+            foreach ($container->getAliases() as $alias) {
+                if ($alias->isPublic() && !$alias->isPrivate() && !isset($this->resolvedIds[$id = (string) $alias]) && $container->has($id)) {
+                    $this->resolvedIds[$id] = true;
+                    $this->processValue($container->getDefinition($id), true);
+                }
+            }
+        } finally {
+            $this->resolvedIds = [];
+            $this->container = null;
+        }
+
+        foreach ($container->getDefinitions() as $definition) {
+            if ($definition->hasTag(self::DO_PRELOAD_TAG)) {
+                $definition->clearTag(self::DO_PRELOAD_TAG);
+            } elseif (!$definition->isDeprecated() && !$definition->hasErrors()) {
+                $definition->addTag($this->tagName);
+            }
+        }
+    }
+
+    /**
+     * {@inheritdoc}
+     */
+    protected function processValue($value, bool $isRoot = false)
+    {
+        if ($value instanceof Reference && ContainerBuilder::IGNORE_ON_UNINITIALIZED_REFERENCE !== $value->getInvalidBehavior() && $this->container->has($id = (string) $value)) {
+            $definition = $this->container->findDefinition($id);
+
+            if (!isset($this->resolvedIds[$id])) {
+                $this->resolvedIds[$id] = true;
+                $this->processValue($definition, true);
+            }
+
+            return $value;
+        }
+
+        if (!$value instanceof Definition) {
+            return parent::processValue($value, $isRoot);
+        }
+
+        if ($value->hasTag($this->tagName) || $value->isDeprecated() || $value->hasErrors()) {
+            return $value;
+        }
+
+        if ($isRoot) {
+            $value->addTag(self::DO_PRELOAD_TAG);
+        }
+
+        return parent::processValue($value, $isRoot);
+    }
+}
diff --git a/src/Symfony/Component/DependencyInjection/Tests/Compiler/ResolveNoPreloadPassTest.php b/src/Symfony/Component/DependencyInjection/Tests/Compiler/ResolveNoPreloadPassTest.php
@@ -0,0 +1,53 @@
+<?php
+
+/*
+ * This file is part of the Symfony package.
+ *
+ * (c) Fabien Potencier <fabien@symfony.com>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Symfony\Component\DependencyInjection\Tests\Compiler;
+
+use PHPUnit\Framework\TestCase;
+use Symfony\Component\DependencyInjection\Compiler\ResolveNoPreloadPass;
+use Symfony\Component\DependencyInjection\ContainerBuilder;
+use Symfony\Component\DependencyInjection\Reference;
+
+class ResolveNoPreloadPassTest extends TestCase
+{
+    public function testProcess()
+    {
+        $container = new ContainerBuilder();
+
+        $container->register('entry_point')
+            ->setPublic(true)
+            ->addArgument(new Reference('preloaded'))
+            ->addArgument(new Reference('not_preloaded'));
+
+        $container->register('preloaded')
+            ->addArgument(new Reference('preloaded_dep'))
+            ->addArgument(new Reference('common_dep'));
+
+        $container->register('not_preloaded')
+            ->setPublic(true)
+            ->addTag('container.no_preload')
+            ->addArgument(new Reference('not_preloaded_dep'))
+            ->addArgument(new Reference('common_dep'));
+
+        $container->register('preloaded_dep');
+        $container->register('not_preloaded_dep');
+        $container->register('common_dep');
+
+        (new ResolveNoPreloadPass())->process($container);
+
+        $this->assertFalse($container->getDefinition('entry_point')->hasTag('container.no_preload'));
+        $this->assertFalse($container->getDefinition('preloaded')->hasTag('container.no_preload'));
+        $this->assertFalse($container->getDefinition('preloaded_dep')->hasTag('container.no_preload'));
+        $this->assertFalse($container->getDefinition('common_dep')->hasTag('container.no_preload'));
+        $this->assertTrue($container->getDefinition('not_preloaded')->hasTag('container.no_preload'));
+        $this->assertTrue($container->getDefinition('not_preloaded_dep')->hasTag('container.no_preload'));
+    }
+}
diff --git a/src/Symfony/Component/EventDispatcher/DependencyInjection/RegisterListenersPass.php b/src/Symfony/Component/EventDispatcher/DependencyInjection/RegisterListenersPass.php
@@ -32,6 +32,8 @@ class RegisterListenersPass implements CompilerPassInterface
 
     private $hotPathEvents = [];
     private $hotPathTagName;
+    private $noPreloadEvents = [];
+    private $noPreloadTagName;
 
     public function __construct(string $dispatcherService = 'event_dispatcher', string $listenerTag = 'kernel.event_listener', string $subscriberTag = 'kernel.event_subscriber', string $eventAliasesParameter = 'event_dispatcher.event_aliases')
     {
@@ -41,14 +43,28 @@ public function __construct(string $dispatcherService = 'event_dispatcher', stri
         $this->eventAliasesParameter = $eventAliasesParameter;
     }
 
-    public function setHotPathEvents(array $hotPathEvents, $tagName = 'container.hot_path')
+    /**
+     * @return $this
+     */
+    public function setHotPathEvents(array $hotPathEvents, string $tagName = 'container.hot_path')
     {
         $this->hotPathEvents = array_flip($hotPathEvents);
         $this->hotPathTagName = $tagName;
 
         return $this;
     }
 
+    /**
+     * @return $this
+     */
+    public function setNoPreloadEvents(array $noPreloadEvents, string $tagName = 'container.no_preload'): self
+    {
+        $this->noPreloadEvents = array_flip($noPreloadEvents);
+        $this->noPreloadTagName = $tagName;
+
+        return $this;
+    }
+
     public function process(ContainerBuilder $container)
     {
         if (!$container->hasDefinition($this->dispatcherService) && !$container->hasAlias($this->dispatcherService)) {
@@ -64,6 +80,8 @@ public function process(ContainerBuilder $container)
         $globalDispatcherDefinition = $container->findDefinition($this->dispatcherService);
 
         foreach ($container->findTaggedServiceIds($this->listenerTag, true) as $id => $events) {
+            $noPreload = 0;
+
             foreach ($events as $event) {
                 $priority = isset($event['priority']) ? $event['priority'] : 0;
 
@@ -99,8 +117,14 @@ public function process(ContainerBuilder $container)
 
                 if (isset($this->hotPathEvents[$event['event']])) {
                     $container->getDefinition($id)->addTag($this->hotPathTagName);
+                } elseif (isset($this->noPreloadEvents[$event['event']])) {
+                    ++$noPreload;
                 }
             }
+
+            if ($noPreload && \count($events) === $noPreload) {
+                $container->getDefinition($id)->addTag($this->noPreloadTagName);
+            }
         }
 
         $extractingDispatcher = new ExtractingEventDispatcher();
@@ -132,6 +156,7 @@ public function process(ContainerBuilder $container)
                 $dispatcherDefinitions = [$globalDispatcherDefinition];
             }
 
+            $noPreload = 0;
             ExtractingEventDispatcher::$aliases = $aliases;
             ExtractingEventDispatcher::$subscriber = $class;
             $extractingDispatcher->addSubscriber($extractingDispatcher);
@@ -143,8 +168,13 @@ public function process(ContainerBuilder $container)
 
                 if (isset($this->hotPathEvents[$args[0]])) {
                     $container->getDefinition($id)->addTag($this->hotPathTagName);
+                } elseif (isset($this->noPreloadEvents[$args[0]])) {
+                    ++$noPreload;
                 }
             }
+            if ($noPreload && \count($extractingDispatcher->listeners) === $noPreload) {
+                $container->getDefinition($id)->addTag($this->noPreloadTagName);
+            }
             $extractingDispatcher->listeners = [];
             ExtractingEventDispatcher::$aliases = [];
         }
diff --git a/src/Symfony/Component/EventDispatcher/Tests/DependencyInjection/RegisterListenersPassTest.php b/src/Symfony/Component/EventDispatcher/Tests/DependencyInjection/RegisterListenersPassTest.php
