Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Nov 9, 2017
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Compiler/UnusedTagsPass.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ class UnusedTagsPass implements CompilerPassInterface
private $whitelist = array(
'cache.pool.clearer',
'console.command',
'container.hot_path',
'container.service_locator',
'container.service_subscriber',
'controller.service_arguments',
Expand Down
11 changes: 10 additions & 1 deletion src/Symfony/Bundle/FrameworkBundle/FrameworkBundle.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@
use Symfony\Component\Form\DependencyInjection\FormPass;
use Symfony\Component\HttpFoundation\Request;
use Symfony\Component\HttpKernel\Bundle\Bundle;
use Symfony\Component\HttpKernel\KernelEvents;
use Symfony\Component\Config\Resource\ClassExistenceResource;
use Symfony\Component\Translation\DependencyInjection\TranslationDumperPass;
use Symfony\Component\Translation\DependencyInjection\TranslationExtractorPass;
Expand Down Expand Up @@ -83,14 +84,22 @@ public function build(ContainerBuilder $container)
{
parent::build($container);

$hotPathEvents = array(
KernelEvents::REQUEST,
KernelEvents::CONTROLLER,
KernelEvents::CONTROLLER_ARGUMENTS,
KernelEvents::RESPONSE,
KernelEvents::FINISH_REQUEST,
);

$container->addCompilerPass(new LoggerPass(), PassConfig::TYPE_BEFORE_OPTIMIZATION, -32);
$container->addCompilerPass(new RegisterControllerArgumentLocatorsPass());
$container->addCompilerPass(new RemoveEmptyControllerArgumentLocatorsPass(), PassConfig::TYPE_BEFORE_REMOVING);
$container->addCompilerPass(new RoutingResolverPass());
$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(), PassConfig::TYPE_BEFORE_REMOVING);
$container->addCompilerPass((new RegisterListenersPass())->setHotPathEvents($hotPathEvents), PassConfig::TYPE_BEFORE_REMOVING);
$container->addCompilerPass(new TemplatingPass());
$this->addCompilerPassIfExists($container, AddConstraintValidatorsPass::class, PassConfig::TYPE_BEFORE_REMOVING);
$container->addCompilerPass(new AddAnnotationsCachedReaderPass(), PassConfig::TYPE_BEFORE_REMOVING);
Expand Down
2 changes: 2 additions & 0 deletions src/Symfony/Bundle/FrameworkBundle/Resources/config/services.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@

<service id="event_dispatcher" class="Symfony\Component\EventDispatcher\ContainerAwareEventDispatcher" public="true">
<argument type="service" id="service_container" />
<tag name="container.hot_path" />
</service>
<service id="Symfony\Component\EventDispatcher\EventDispatcherInterface" alias="event_dispatcher" />

Expand All @@ -17,6 +18,7 @@
<argument type="service" id="controller_resolver" />
<argument type="service" id="request_stack" />
<argument type="service" id="argument_resolver" />
<tag name="container.hot_path" />
</service>
<service id="Symfony\Component\HttpKernel\HttpKernelInterface" alias="http_kernel" />

Expand Down
2 changes: 1 addition & 1 deletion src/Symfony/Bundle/FrameworkBundle/composer.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@
"symfony/class-loader": "~3.2",
"symfony/dependency-injection": "~3.4|~4.0",
"symfony/config": "~3.4|~4.0",
"symfony/event-dispatcher": "^3.3.1|~4.0",
"symfony/event-dispatcher": "^3.4-beta4|~4.0-beta4",
"symfony/http-foundation": "^3.3.11|~4.0",
"symfony/http-kernel": "~3.4|~4.0",
"symfony/polyfill-mbstring": "~1.0",
Expand Down
1 change: 1 addition & 0 deletions src/Symfony/Component/DependencyInjection/Compiler/PassConfig.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,7 @@ public function __construct()
)),
new DefinitionErrorExceptionPass(),
new CheckExceptionOnInvalidReferenceBehaviorPass(),
new ResolveHotPathPass(),
));
}

Expand Down
71 changes: 71 additions & 0 deletions src/Symfony/Component/DependencyInjection/Compiler/ResolveHotPathPass.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
<?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\Argument\ArgumentInterface;
use Symfony\Component\DependencyInjection\ContainerBuilder;
use Symfony\Component\DependencyInjection\Definition;
use Symfony\Component\DependencyInjection\Reference;

/**
* Propagate "container.hot_path" tags to referenced services.
*
* @author Nicolas Grekas <p@tchwork.com>
*/
class ResolveHotPathPass extends AbstractRecursivePass
{
private $tagName;
private $resolvedIds = array();

public function __construct($tagName = 'container.hot_path')
{
$this->tagName = $tagName;
}

/**
* {@inheritdoc}
*/
public function process(ContainerBuilder $container)
{
try {
parent::process($container);
$container->getDefinition('service_container')->clearTag($this->tagName);
} finally {
$this->resolvedIds = array();
}
}

/**
* {@inheritdoc}
*/
protected function processValue($value, $isRoot = false)
{
if ($value instanceof ArgumentInterface) {
return $value;
}
if ($value instanceof Definition && $isRoot && (isset($this->resolvedIds[$this->currentId]) || !$value->hasTag($this->tagName))) {
return $value;
}
if ($value instanceof Reference && ContainerBuilder::IGNORE_ON_UNINITIALIZED_REFERENCE !== $value->getInvalidBehavior() && $this->container->has($id = (string) $value)) {
$definition = $this->container->findDefinition($id);
if (!$definition->hasTag($this->tagName)) {
$this->resolvedIds[$id] = true;
$definition->addTag($this->tagName);
parent::processValue($definition, false);
}

return $value;
}

return parent::processValue($value, $isRoot);
}
}
117 changes: 106 additions & 11 deletions src/Symfony/Component/DependencyInjection/Dumper/PhpDumper.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,9 @@ class PhpDumper extends Dumper
private $usedMethodNames;
private $namespace;
private $asFiles;
private $hotPathTag;
private $inlineRequires;
private $inlinedRequires = array();

/**
* @var ProxyDumper
Expand Down Expand Up @@ -108,16 +111,21 @@ public function setProxyDumper(ProxyDumper $proxyDumper)
public function dump(array $options = array())
{
$this->targetDirRegex = null;
$this->inlinedRequires = array();
$options = array_merge(array(
'class' => 'ProjectServiceContainer',
'base_class' => 'Container',
'namespace' => '',
'as_files' => false,
'debug' => true,
'hot_path_tag' => null,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does it not have a default?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

'inline_class_loader_parameter' => 'container.dumper.inline_class_loader',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

), $options);

$this->namespace = $options['namespace'];
$this->asFiles = $options['as_files'];
$this->hotPathTag = $options['hot_path_tag'];
$this->inlineRequires = $this->container->hasParameter($options['inline_class_loader_parameter']) && $this->container->getParameter($options['inline_class_loader_parameter']);
$this->initializeMethodNamesMap($options['base_class']);

$this->docStar = $options['debug'] ? '*' : '';
Expand Down Expand Up @@ -214,6 +222,7 @@ class_alias(Container{$hash}::class, {$options['class']}::class, false);
}

$this->targetDirRegex = null;
$this->inlinedRequires = array();

$unusedEnvs = array();
foreach ($this->container->getEnvCounters() as $env => $use) {
Expand Down Expand Up @@ -257,9 +266,13 @@ private function addServiceLocalTempVariables($cId, Definition $definition, arra

array_unshift($inlinedDefinitions, $definition);

$collectLineage = $this->inlineRequires && !($this->hotPathTag && $definition->hasTag($this->hotPathTag));
$isNonLazyShared = !$this->getProxyDumper()->isProxyCandidate($definition) && $definition->isShared();
$calls = $behavior = array();
$lineage = $calls = $behavior = array();
foreach ($inlinedDefinitions as $iDefinition) {
if ($collectLineage && $class = is_array($factory = $iDefinition->getFactory()) && is_string($factory[0]) ? $factory[0] : $iDefinition->getClass()) {
$this->collectLineage($class, $lineage);
}
$this->getServiceCallsFromArguments($iDefinition->getArguments(), $calls, $behavior, $isNonLazyShared);
$isPreInstantiation = $isNonLazyShared && $iDefinition !== $definition && !$this->hasReference($cId, $iDefinition->getMethodCalls(), true) && !$this->hasReference($cId, $iDefinition->getProperties(), true);
$this->getServiceCallsFromArguments($iDefinition->getMethodCalls(), $calls, $behavior, $isPreInstantiation);
Expand All @@ -274,6 +287,13 @@ private function addServiceLocalTempVariables($cId, Definition $definition, arra
continue;
}

if ($collectLineage && ContainerInterface::IGNORE_ON_UNINITIALIZED_REFERENCE !== $behavior[$id] && $this->container->has($id)
&& $this->isTrivialInstance($iDefinition = $this->container->findDefinition($id))
&& $class = is_array($factory = $iDefinition->getFactory()) && is_string($factory[0]) ? $factory[0] : $iDefinition->getClass()
) {
$this->collectLineage($class, $lineage);
}

if ($callCount > 1) {
$name = $this->getNextVariableName();
$this->referenceVariables[$id] = new Variable($name);
Expand All @@ -300,9 +320,48 @@ private function addServiceLocalTempVariables($cId, Definition $definition, arra
$code .= "\n";
}

if ($lineage && $lineage = array_diff_key(array_flip($lineage), $this->inlinedRequires)) {
$code = "\n".$code;

foreach (array_reverse($lineage) as $file => $class) {
$code = sprintf(" require_once %s;\n", $file).$code;
}
}

return $code;
}

private function collectLineage($class, array &$lineage)
{
if (isset($lineage[$class])) {
return;
}
if (!$r = $this->container->getReflectionClass($class)) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return;
}
if ($this->container instanceof $class) {
return;
}
$file = $r->getFileName();
if (!$file || $this->doExport($file) === $exportedFile = $this->export($file)) {
return;
}

if ($parent = $r->getParentClass()) {
$this->collectLineage($parent->name, $lineage);
}

foreach ($r->getInterfaces() as $parent) {
$this->collectLineage($parent->name, $lineage);
}

foreach ($r->getTraits() as $parent) {
$this->collectLineage($parent->name, $lineage);
}

$lineage[$class] = substr($exportedFile, 1, -1);
}

private function generateProxyClasses()
{
$definitions = $this->container->getDefinitions();
Expand Down Expand Up @@ -509,10 +568,15 @@ private function isTrivialInstance(Definition $definition)
if (!$v || ($v instanceof Reference && 'service_container' === (string) $v)) {
continue;
}
if ($v instanceof Reference && $this->container->has($id = (string) $v) && $this->container->findDefinition($id)->isSynthetic()) {
continue;
}
if (!is_scalar($v) || $this->dumpValue($v) !== $this->dumpValue($v, false)) {
return false;
}
}
} elseif ($arg instanceof Reference && $this->container->has($id = (string) $arg) && $this->container->findDefinition($id)->isSynthetic()) {
continue;
} elseif (!is_scalar($arg) || $this->dumpValue($arg) !== $this->dumpValue($arg, false)) {
return false;
}
Expand Down Expand Up @@ -694,7 +758,7 @@ private function addService($id, Definition $definition, &$file = null)
$lazyInitialization = '';
}

$asFile = $this->asFiles && $definition->isShared();
$asFile = $this->asFiles && $definition->isShared() && !($this->hotPathTag && $definition->hasTag($this->hotPathTag));
$methodName = $this->generateMethodName($id);
if ($asFile) {
$file = $methodName.'.php';
Expand Down Expand Up @@ -760,7 +824,7 @@ private function addServices()
$definitions = $this->container->getDefinitions();
ksort($definitions);
foreach ($definitions as $id => $definition) {
if ($definition->isSynthetic() || ($this->asFiles && $definition->isShared())) {
if ($definition->isSynthetic() || ($this->asFiles && $definition->isShared() && !($this->hotPathTag && $definition->hasTag($this->hotPathTag)))) {
continue;
}
if ($definition->isPublic()) {
Expand All @@ -778,7 +842,7 @@ private function generateServiceFiles()
$definitions = $this->container->getDefinitions();
ksort($definitions);
foreach ($definitions as $id => $definition) {
if (!$definition->isSynthetic() && $definition->isShared()) {
if (!$definition->isSynthetic() && $definition->isShared() && !($this->hotPathTag && $definition->hasTag($this->hotPathTag))) {
$code = $this->addService($id, $definition, $file);
yield $file => $code;
}
Expand Down Expand Up @@ -899,6 +963,7 @@ public function __construct()
$code .= $this->asFiles ? $this->addFileMap() : '';
$code .= $this->addPrivateServices();
$code .= $this->addAliases();
$code .= $this->addInlineRequires();
$code .= <<<'EOF'
}

Expand Down Expand Up @@ -1050,7 +1115,7 @@ private function addMethodMap()
$definitions = $this->container->getDefinitions();
ksort($definitions);
foreach ($definitions as $id => $definition) {
if (!$definition->isSynthetic() && (!$this->asFiles || !$definition->isShared())) {
if (!$definition->isSynthetic() && (!$this->asFiles || !$definition->isShared() || ($this->hotPathTag && $definition->hasTag($this->hotPathTag)))) {
$code .= ' '.$this->export($id).' => '.$this->export($this->generateMethodName($id)).",\n";
}
}
Expand All @@ -1069,7 +1134,7 @@ private function addFileMap()
$definitions = $this->container->getDefinitions();
ksort($definitions);
foreach ($definitions as $id => $definition) {
if (!$definition->isSynthetic() && $definition->isShared()) {
if (!$definition->isSynthetic() && $definition->isShared() && !($this->hotPathTag && $definition->hasTag($this->hotPathTag))) {
$code .= sprintf(" %s => __DIR__.'/%s.php',\n", $this->export($id), $this->generateMethodName($id));
}
}
Expand Down Expand Up @@ -1137,6 +1202,38 @@ private function addAliases()
return $code." );\n";
}

private function addInlineRequires()
{
if (!$this->hotPathTag || !$this->inlineRequires) {
Copy link
Contributor

@Tobion Tobion Nov 8, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return '';
}

$lineage = array();

foreach ($this->container->findTaggedServiceIds($this->hotPathTag) as $id => $tags) {
$definition = $this->container->getDefinition($id);
$inlinedDefinitions = $this->getInlinedDefinitions($definition);
array_unshift($inlinedDefinitions, $definition);

foreach ($inlinedDefinitions as $iDefinition) {
if ($class = is_array($factory = $iDefinition->getFactory()) && is_string($factory[0]) ? $factory[0] : $iDefinition->getClass()) {
$this->collectLineage($class, $lineage);
}
}
}

$code = "\n";

foreach ($lineage as $file) {
if (!isset($this->inlinedRequires[$file])) {
$this->inlinedRequires[$file] = true;
$code .= sprintf(" require_once %s;\n", $file);
}
}

return "\n" === $code ? '' : $code;
}

/**
* Adds default parameters method.
*
Expand Down Expand Up @@ -1408,7 +1505,7 @@ private function getServiceCallsFromArguments(array $arguments, array &$calls, a
$id = (string) $argument;

if (!isset($calls[$id])) {
$calls[$id] = (int) $isPreInstantiation;
$calls[$id] = (int) ($isPreInstantiation && $this->container->has($id) && !$this->container->findDefinition($id)->isSynthetic());
}
if (!isset($behavior[$id])) {
$behavior[$id] = $argument->getInvalidBehavior();
Expand Down Expand Up @@ -1746,17 +1843,15 @@ private function getServiceCall($id, Reference $reference = null)
return '$this';
}

if ($this->container->hasDefinition($id)) {
$definition = $this->container->getDefinition($id);

if ($this->container->hasDefinition($id) && ($definition = $this->container->getDefinition($id)) && !$definition->isSynthetic()) {
if (null !== $reference && ContainerInterface::IGNORE_ON_UNINITIALIZED_REFERENCE === $reference->getInvalidBehavior()) {
$code = 'null';
} elseif ($this->isTrivialInstance($definition)) {
$code = substr($this->addNewInstance($definition, '', '', $id), 8, -2);
if ($definition->isShared()) {
$code = sprintf('$this->services[\'%s\'] = %s', $id, $code);
}
} elseif ($this->asFiles && $definition->isShared()) {
} elseif ($this->asFiles && $definition->isShared() && !($this->hotPathTag && $definition->hasTag($this->hotPathTag))) {
$code = sprintf("\$this->load(__DIR__.'/%s.php')", $this->generateMethodName($id));
} else {
$code = sprintf('$this->%s()', $this->generateMethodName($id));
Expand Down
Loading