[DI] Add ServiceLocatorArgument to generate array-based locators optimized for OPcache shared memory #27783

nicolas-grekas · Jun 30, 2018

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

Right now, to generate service locators, we use collections of closures described using ServiceClosureArgument. This works well, but it doesn't scale well when the number of services grows, because we have to load as many closures as there are services, even if we never call them.

This PR introduces ServiceLocatorArgument, which describes the same thing, but allows dumping optimized locators: instead of a collection of closures, this generates a static array that OPcache can put in shared memory (see fixtures for example.)

Once this PR is merged, we'll be able to update ServiceLocatorPass::register() to leverage it and generate these optimized locators everywhere. One particular I have in mind in the locator used by ServiceArgumentResolver, which can grow fast (it has as many entries as there are actions.)

nicolas-grekas

PR is now fully fledged and ready. Service locators defined as collections of closures are dumped as optimized locators and locators defined using the new ServiceLocatorArgument are still deduplicated as the previous.

nicolas-grekas · Jul 1, 2018

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

-                </service>
+            <argument type="service_locator">
+                <argument key="session" type="service" id="session" on-invalid="ignore" />
+                <argument key="initialized_session" type="service" id="session" on-invalid="ignore_uninitialized" />


Here is an example showing easier definition of service locators.

nicolas-grekas · Jul 1, 2018

src/Symfony/Component/DependencyInjection/Tests/Fixtures/php/services_subscriber.php

-        }, 'baz' => function (): ?\Symfony\Component\DependencyInjection\Tests\Fixtures\CustomDefinition {
-            return ($this->privates['Symfony\Component\DependencyInjection\Tests\Fixtures\CustomDefinition'] ?? $this->privates['Symfony\Component\DependencyInjection\Tests\Fixtures\CustomDefinition'] = new \Symfony\Component\DependencyInjection\Tests\Fixtures\CustomDefinition());
-        })))->withContext('foo_service', $this));
+        return $this->services['foo_service'] = new \Symfony\Component\DependencyInjection\Tests\Fixtures\TestServiceSubscriber((new \Symfony\Component\DependencyInjection\Argument\ServiceLocator(\Closure::fromCallable(array($this, 'getService')), array(


Here is an example of an optimized service locator: we now dump a static array instead of an array of closures.

chalasr · Jul 1, 2018

src/Symfony/Component/DependencyInjection/Argument/ReferenceSetArgumentTrait.php

+    }
+
+    /**
+     * @return array The values to lazily iterate over


description doesn't seem relevant anymore, @return Reference[]?

fixed thanks!

chalasr · Jul 1, 2018

src/Symfony/Component/DependencyInjection/Argument/ReferenceSetArgumentTrait.php

+    }
+
+    /**
+     * @param Reference[] $values The service references to lazily iterate over


ogizanagi · Jul 3, 2018

src/Symfony/Component/DependencyInjection/Argument/ServiceLocator.php

+ * file that was distributed with this source code.
+ */
+
+namespace Symfony\Component\DependencyInjection\Argument;


Just asking, should it really be in this namespace? Seems a bit confusing to me. Can't we just keep it in Symfony\Component\DependencyInjection and find an appropriate name?

like RewindableIterator: this is internal class, I don't think it should be at the root of the component

Fair enough. But this isn't an argument in the sense of the ArgumentInterface, i.e a representation used before resolution/compilation. This is the actual resolved item used at runtime, indeed like the RewindableGenerator. As both are internal, we could move them to a Resolved/Runtime sub-namespace, just in order to not bring confusion with actual ArgumentInterface implementations which are "descriptors". Those ones can be used by anyone.

Still not convinced this deserves more. Not all classes in a sub-namespace have to implement the same interface. Here, if there is a rule, it's that all classes that have the Argument suffix should implement ArgumentInterface.
But I wouldn't bug on some others like this one...

No problem, just random thoughts.

stof · Jul 3, 2018

src/Symfony/Component/DependencyInjection/Compiler/DefinitionErrorExceptionPass.php

@@ -33,6 +40,10 @@ protected function processValue($value, $isRoot = false)
        }

        if ($isRoot && !$value->isPublic()) {
+            if ($this->analyzingPass) {
+                $this->analyzingPass->process($this->container);
+                $this->analyzingPass = null;


this means we analyze only once. So does it actually make sense to have it in processValue ?

reverted actually, that's another topic for another PR

stof · Jul 3, 2018

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

+                            continue;
+                        }
+                        $definition = $this->container->findDefinition($id = (string) $v);
+                        $load = !($e = $definition->getErrors()) ? $this->asFiles && !$this->isHotPath($definition) : reset($e);


why would $load contain an error in some cases ? This looks like a confusing API

It's not an API but an internal data structure, I'd prefer making it compact.

stof · Jul 3, 2018

src/Symfony/Component/DependencyInjection/Container.php

@@ -401,6 +402,35 @@ protected function getEnv($name)
        }
    }

+    protected function throw($message)


does it actually make sense to have it in the parent class ?

reverted that part :)

stof · Jul 3, 2018

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

@@ -1645,7 +1617,7 @@ private function dumpValue($value, bool $interpolate = true): string
    private function dumpLiteralClass(string $class): string
    {
        if (false !== strpos($class, '$')) {
-            return sprintf('${($_ = %s) && false ?: "_"}', $class);
+            return "\${(\$_ = {$class}) ? '_' : '_'}";


what is this about actually ?

the exact same written in a nicer way, but unrelated, now reverted

stof · Jul 3, 2018

src/Symfony/Component/DependencyInjection/Loader/XmlFileLoader.php

                case 'iterator':
                    $arg = $this->getArgumentsAsPhp($arg, $name, $file, false);
                    try {
-                        $arguments[$key] = new IteratorArgument($arg);
+                        $arguments[$key] = 'iterator' === $type ? new IteratorArgument($arg) : new ServiceLocatorArgument($arg);


given that all the parsing of referenced is already factored inside getArgumentsAsPhp, I don't think it makes sense to group the 2 cases together and then performed if checks in all other statements

chalasr · Jul 3, 2018

src/Symfony/Component/DependencyInjection/Loader/YamlFileLoader.php

                if (!is_array($argument)) {
-                    throw new InvalidArgumentException(sprintf('"!iterator" tag only accepts sequences in "%s".', $file));
+                    throw new InvalidArgumentException(sprintf('"!%s" tag only accepts sequences in "%s".', $value->getTag(), $file));


In xml each service is keyed, can't they be in yaml? If yes, service_locator also accepts mappings (maybe only mappings, not sequences?). c.f. #21553 (review) (glad to see the argument back btw :))

indeed, a service locator accepts maps, not sequences.

…mized for OPcache shared memory

nicolas-grekas · Jul 4, 2018

Comments addressed, thanks for the review.

…d locators optimized for OPcache shared memory (nicolas-grekas) This PR was merged into the 4.2-dev branch. Discussion ---------- [DI] Add ServiceLocatorArgument to generate array-based locators optimized for OPcache shared memory | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | - | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - Right now, to generate service locators, we use collections of closures described using `ServiceClosureArgument`. This works well, but it doesn't scale well when the number of services grows, because we have to load as many closures as there are services, even if we never call them. This PR introduces `ServiceLocatorArgument`, which describes the same thing, but allows dumping optimized locators: instead of a collection of closures, this generates a static array that OPcache can put in shared memory (see fixtures for example.) Once this PR is merged, we'll be able to update `ServiceLocatorPass::register()` to leverage it and generate these optimized locators everywhere. One particular I have in mind in the locator used by `ServiceArgumentResolver`, which can grow fast (it has as many entries as there are actions.) Commits ------- 6c8e957 [DI] Add ServiceLocatorArgument to generate array-based locators optimized for OPcache shared memory

nicolas-grekas added the Performance label Jun 30, 2018

nicolas-grekas added this to the next milestone Jun 30, 2018

carsonbot added Status: Needs Review DependencyInjection Feature labels Jun 30, 2018

nicolas-grekas force-pushed the di-closure-locator branch 6 times, most recently from d7ae279 to a5b4efb Compare July 1, 2018 06:32

nicolas-grekas commented Jul 1, 2018

View reviewed changes

nicolas-grekas force-pushed the di-closure-locator branch from a5b4efb to 934299f Compare July 1, 2018 06:42

chalasr reviewed Jul 1, 2018

View reviewed changes

nicolas-grekas force-pushed the di-closure-locator branch 2 times, most recently from bc38cdd to b274613 Compare July 1, 2018 19:40

ogizanagi reviewed Jul 3, 2018

View reviewed changes

stof reviewed Jul 3, 2018

View reviewed changes

chalasr reviewed Jul 3, 2018

View reviewed changes

nicolas-grekas force-pushed the di-closure-locator branch 2 times, most recently from 0ae2098 to 9e7d579 Compare July 4, 2018 13:57

[DI] Add ServiceLocatorArgument to generate array-based locators opti…

6c8e957

…mized for OPcache shared memory

nicolas-grekas force-pushed the di-closure-locator branch from 9e7d579 to 6c8e957 Compare July 4, 2018 14:01

carsonbot removed the Status: Needs Review label Jul 4, 2018

ogizanagi approved these changes Jul 4, 2018

View reviewed changes

carsonbot added the Status: Reviewed label Jul 4, 2018

chalasr approved these changes Jul 6, 2018

View reviewed changes

nicolas-grekas merged commit 6c8e957 into symfony:master Jul 7, 2018

nicolas-grekas deleted the di-closure-locator branch July 7, 2018 16:05

dmaicher mentioned this pull request Aug 9, 2018

Failed opening required container cache file, when using form system #28180

Closed

nicolas-grekas mentioned this pull request Sep 7, 2018

Make it possible to omit keys for service locaters #26892

Closed

javiereguiluz mentioned this pull request Sep 13, 2018

[DI] Add ServiceLocatorArgument to generate array-based locators symfony/symfony-docs#10335

Closed

nicolas-grekas modified the milestones: next, 4.2 Nov 1, 2018

This was referenced Nov 3, 2018

Release v4.2.0-BETA1 #29072

Closed

Release v4.2.0-BETA1 #29073

Merged

Search code, repositories, users, issues, pull requests...

Uh oh!

[DI] Add ServiceLocatorArgument to generate array-based locators optimized for OPcache shared memory #27783

[DI] Add ServiceLocatorArgument to generate array-based locators optimized for OPcache shared memory #27783

Uh oh!

Conversation

nicolas-grekas commented Jun 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nicolas-grekas left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ogizanagi Jul 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicolas-grekas Jul 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicolas-grekas commented Jul 4, 2018

Uh oh!

Uh oh!

nicolas-grekas commented Jun 30, 2018 •

edited

Loading

ogizanagi Jul 3, 2018 •

edited

Loading

nicolas-grekas Jul 4, 2018 •

edited

Loading