[Config][DI] Add ComposerResource to track the runtime engine + deps #21505

nicolas-grekas · 2017-02-02T09:26:44Z

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

So that the container is invalidated whenever a new PHP runtime is used, a PHP-extension is added, or vendors are changed.

stof · 2017-02-02T09:36:26Z

src/Symfony/Component/Config/Resource/ComposerResource.php

+        self::$globalVersions = array();
+
+        foreach (get_loaded_extensions() as $ext) {
+            self::$globalVersions[$ext] = phpversion($ext);


should we also add the PHP version ?

it's already there, because among the extensions, there is always "Core", which holds the php version

stof · 2017-02-02T09:38:25Z

src/Symfony/Component/Config/Resource/ComposerResource.php

+                $r = new \ReflectionClass($class);
+                $v = dirname(dirname($r->getFileName())).DIRECTORY_SEPARATOR;
+                if (file_exists($v.'composer/installed.json')) {
+                    self::$globalVendors[$v] = @filemtime($v);


IMO, we should rather hash the file. If you rerun composer install, the file will be touched by composer even when there is no change

When the file is touched, we'd better recompute the container once rather than compute the hash again and again IMHO

javiereguiluz

👍

javiereguiluz · 2017-02-02T09:46:13Z

src/Symfony/Component/Config/Resource/ComposerResource.php

+namespace Symfony\Component\Config\Resource;
+
+/**
+ * ComposerResource tracks the PHP version and composer dependencies.


composer -> Composer

javiereguiluz · 2017-02-02T09:47:40Z

src/Symfony/Component/Config/Resource/ComposerResource.php

+    private $versions;
+    private $vendors;
+
+    private static $globalVersions;


I don't like the name of these variables. What about $existingVersions or $installedVersions ? (same for the $globalVendors var).

change to $runtimeVersions & $runtimeVendors

nicolas-grekas · 2017-02-03T08:21:52Z

Any files in vendors are now excluded from tracking, transferring tracking responsibility to the new ComposerResource. This should save a bunch of checks in practice, thus enhance DX.

stof · 2017-02-03T18:45:22Z

src/Symfony/Bridge/Doctrine/DependencyInjection/AbstractDoctrineExtension.php

+            $driver = 'php';
+        } else {
+            // add the directory itself as a resource
+            $container->fileExists($dir);


I would also rather add $dir.'/'.$configPath as a resource, as creating the directory and adding files in it could change the config (adding the whole bundle as a resource when <bundle>/Resources does not exist was a mistake IMO, hurting performance for some people using annotation, and not working fine anyway as it was not detecting the creation of the folder which would change the type)

I guess the code is as it is because $configPath can be a glob/missing subdirs, so the loop just removes "glob+sub-dirs" until a real one is found.

stof · 2017-02-03T18:49:01Z

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

@@ -67,7 +66,7 @@ private function updateDefinition(ContainerBuilder $container, $id, Definition $
            try {
                $m = new \ReflectionFunction($factory);
                if (false !== $m->getFileName() && file_exists($m->getFileName())) {
-                    $container->addResource(new FileResource($m->getFileName()));
+                    $container->fileExists($m->getFileName());


shouldn't you replace the file_exists check above instead ?

no need: if the file does not exist, we should not track its non-existence

stof · 2017-02-03T18:52:21Z

src/Symfony/Bridge/Doctrine/DependencyInjection/AbstractDoctrineExtension.php

-            return 'annotation';
+        // add the closest existing directory as a resource
+        $resource = $dir.'/'.$configPath;
+        while (!is_dir($resource)) {


the only way to reach this code now is that one of the glob calls found something, so we are sure that $dir.'/'.$configPath exists (and we need to track both its existence and the addition/removal of files in it)

To me, the code change in this file only ensures that only one resource is added. The remaining logic is untouched. I wouldn't change anything else than the double resource issue here.

stof · 2017-02-03T18:53:25Z

This should save a bunch of checks in practice, thus enhance DX.

have you tried profiling the dev environment of Symfony to see the impact this PR has ?

nicolas-grekas · 2017-02-03T19:53:46Z

have you tried profiling the dev environment of Symfony to see the impact this PR has ?

no, but it's not hard to say that if you happen to have some bundles, everything config-related in them won't be tracked anymore, thus the gain can be huge (esp since this involves i/o). Add autowiring to the mix, which creates resources for every single service when looking for filling type hints - it's even bigger :)

stof · 2017-02-07T08:58:56Z

@nicolas-grekas could you try profiling it to give numbers ? (blame the mail I receive from the Sensiolabs marketing telling me I need to measure things to be able to optimize them... 😄 )

nicolas-grekas · 2017-02-07T14:32:46Z

@stof and you were right to ask, because that needed some polishing :)
So, end result on Symfony Demo:

var/cache/dev/appDevDebugProjectContainer.php.meta deflates from 239K to 15K
performance increases by 15% on the home page (which does almost nothing itself), see Blackfire comparison.

fabpot · 2017-02-12T11:59:34Z

Thank you @nicolas-grekas.

… engine + deps (nicolas-grekas) This PR was merged into the 3.3-dev branch. Discussion ---------- [Config][DI] Add ComposerResource to track the runtime engine + deps | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - So that the container is invalidated whenever a new PHP runtime is used, a PHP-extension is added, or vendors are changed. Commits ------- a960c76 [Config][DI] Add ComposerResource to track runtime + vendors

This PR was merged into the 3.3 branch. Discussion ---------- [DI] Use GlobResource for non-tracked directories | Q | A | ------------- | --- | Branch? | 3.3 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | n/a | License | MIT | Doc PR | n/a I noticed that some of my excluded directories are still tracked by the container and changes to files inside them make it recompile. This brought me to the `$trackContents || is_dir($path)` line introduced in #21505. Commits ------- 048eb18 [DI] Use GlobResource for non-tracked directories

nicolas-grekas added this to the 3.3 milestone Feb 2, 2017

carsonbot added Status: Needs Review Config DependencyInjection Feature labels Feb 2, 2017

nicolas-grekas force-pushed the di-vendor-res branch 2 times, most recently from af806a8 to c71d9ee Compare February 2, 2017 09:29

stof reviewed Feb 2, 2017

View reviewed changes

javiereguiluz reviewed Feb 2, 2017

View reviewed changes

nicolas-grekas force-pushed the di-vendor-res branch 2 times, most recently from 6f67de1 to 7368e32 Compare February 2, 2017 09:52

This was referenced Feb 2, 2017

[DI] : Fix bad generation of proxy class when use overriden getter on class with constructor #21508

Merged

[DI][Config] Add ContainerBuilder::classExists() for checking and tracking class, interface or trait existence #21452

Closed

nicolas-grekas mentioned this pull request Feb 2, 2017

[DI][Config] Add & use ReflectionClassResource #21419

Merged

nicolas-grekas force-pushed the di-vendor-res branch 4 times, most recently from 6d7bedd to c3e7022 Compare February 3, 2017 08:20

nicolas-grekas force-pushed the di-vendor-res branch 2 times, most recently from b7adaad to e930f40 Compare February 3, 2017 12:04

stof reviewed Feb 3, 2017

View reviewed changes

greg0ire mentioned this pull request Feb 5, 2017

Introduce weak vendors mode #21539

Merged

7 tasks

nicolas-grekas force-pushed the di-vendor-res branch from e930f40 to 2cf2170 Compare February 7, 2017 14:30

nicolas-grekas force-pushed the di-vendor-res branch 2 times, most recently from 0a834df to 53985cc Compare February 7, 2017 14:36

nicolas-grekas force-pushed the di-vendor-res branch 2 times, most recently from 1738293 to 4e58b34 Compare February 7, 2017 14:40

[Config][DI] Add ComposerResource to track runtime + vendors

a960c76

nicolas-grekas force-pushed the di-vendor-res branch from 4e58b34 to a960c76 Compare February 7, 2017 14:43

nicolas-grekas added the Ready label Feb 11, 2017

fabpot merged commit a960c76 into symfony:master Feb 12, 2017

javiereguiluz mentioned this pull request Feb 12, 2017

Document the new ComposerResource symfony/symfony-docs#7463

Closed

nicolas-grekas deleted the di-vendor-res branch February 14, 2017 15:20

fabpot mentioned this pull request May 1, 2017

Release v3.3.0-BETA1 #22603

Merged

vudaltsov mentioned this pull request Aug 11, 2017

[DI] Use GlobResource for non-tracked directories #23870

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Config][DI] Add ComposerResource to track the runtime engine + deps #21505

[Config][DI] Add ComposerResource to track the runtime engine + deps #21505

nicolas-grekas commented Feb 2, 2017 •

edited

Loading

stof Feb 2, 2017

nicolas-grekas Feb 2, 2017

stof Feb 2, 2017

stof Feb 2, 2017

nicolas-grekas Feb 2, 2017

javiereguiluz left a comment

javiereguiluz Feb 2, 2017

javiereguiluz Feb 2, 2017

nicolas-grekas Feb 2, 2017

nicolas-grekas commented Feb 3, 2017

stof Feb 3, 2017

nicolas-grekas Feb 3, 2017 •

edited

Loading

stof Feb 3, 2017

nicolas-grekas Feb 3, 2017

stof Feb 3, 2017

nicolas-grekas Feb 3, 2017 •

edited

Loading

stof commented Feb 3, 2017

nicolas-grekas commented Feb 3, 2017

stof commented Feb 7, 2017 •

edited

Loading

nicolas-grekas commented Feb 7, 2017 •

edited

Loading

fabpot commented Feb 12, 2017

[Config][DI] Add ComposerResource to track the runtime engine + deps #21505

[Config][DI] Add ComposerResource to track the runtime engine + deps #21505

Conversation

nicolas-grekas commented Feb 2, 2017 • edited Loading

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

javiereguiluz left a 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

nicolas-grekas commented Feb 3, 2017

Choose a reason for hiding this comment

nicolas-grekas Feb 3, 2017 • edited Loading

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

nicolas-grekas Feb 3, 2017 • edited Loading

Choose a reason for hiding this comment

stof commented Feb 3, 2017

nicolas-grekas commented Feb 3, 2017

stof commented Feb 7, 2017 • edited Loading

nicolas-grekas commented Feb 7, 2017 • edited Loading

fabpot commented Feb 12, 2017

nicolas-grekas commented Feb 2, 2017 •

edited

Loading

nicolas-grekas Feb 3, 2017 •

edited

Loading

nicolas-grekas Feb 3, 2017 •

edited

Loading

stof commented Feb 7, 2017 •

edited

Loading

nicolas-grekas commented Feb 7, 2017 •

edited

Loading