[DI] Generate one file per service factory #23741

nicolas-grekas · 2017-08-01T12:54:47Z

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

See #23678 for background on this proposal.

javiereguiluz

I guess there are technical reasons to have LazyLoadingValueHolderFactoryV1 and LazyLoadingValueHolderFactoryV2. However, using V1 and V2 as part of the class names feels wrong.

Unless in the future you plan to add V3, V4, etc. could we remove V2 and use Legacy instead of V1?

nicolas-grekas · 2017-08-01T13:18:59Z

@javiereguiluz your comment is for #23729. And yes, I've no better name idea and the intent is to reflect the major version of ocramius/proxy-manager, so yes also, there will be as many versions of this file as there will be BC breaks on the parent class.

nicolas-grekas

PR is ready. Discussion started in #23678 so please read it to have benchs and rationales esp.

nicolas-grekas · 2017-08-03T15:54:34Z

src/Symfony/Bridge/ProxyManager/Tests/LazyProxy/PhpDumper/ProxyDumperTest.php

@@ -70,19 +70,15 @@ public function testGetProxyFactoryCodeWithCustomMethod()
        $code = $this->dumper->getProxyFactoryCode($definition, 'foo', '$this->getFoo2Service(false)');

        $this->assertStringMatchesFormat(
-            '%wif ($lazyLoad) {%wreturn $this->services[\'foo\'] =%s'


the format is already tested elsewhere, no need to duplicate that effort, it just makes updating tests more complex (same below)

nicolas-grekas · 2017-08-03T15:55:54Z

src/Symfony/Bundle/FrameworkBundle/Command/CacheClearCommand.php

@@ -92,6 +93,9 @@ protected function execute(InputInterface $input, OutputInterface $output)

        $filesystem->remove($oldCacheDir);

+        // The current event dispatcher is stale, let's not use it anymore
+        $this->getApplication()->setDispatcher(new EventDispatcher());


required because the current legacy container is unusable (its service factory files have been deleted.)
anyway, looks legit to me, as conveyed by the comment

nicolas-grekas · 2017-08-03T15:56:55Z

src/Symfony/Component/DependencyInjection/Container.php

@@ -299,49 +300,48 @@ public function get($id, $invalidBehavior = self::EXCEPTION_ON_INVALID_REFERENCE
                throw new ServiceCircularReferenceException($id, array_keys($this->loading));
            }

-            if (isset($this->methodMap[$id])) {


the diff in this method is better viewed ignoring whitespaces

nicolas-grekas · 2017-08-03T15:59:34Z

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

+            $code .= "    }\n";
+        }
+        if ($this->asFiles) {
+            $code = str_replace("\$this->load(__DIR__.''.'", "\$this->load(__DIR__.'".($asFile ? '' : '/'.$this->class), $code);


injecting the $this->class directory prefix is the reason why we generate code like __DIR__.''.'...': these quotes cannot happen in var_export'ed strings, thus can be "parsed" very easily with a simple str_replace

nicolas-grekas · 2017-08-03T16:01:50Z

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

+    /*{$this->docStar}
+     * {@inheritdoc}
+     */
+    protected function load(\$file, \$lazyLoad = true)


even if the implementation in the parent class is exactly the same, this is needed to have access to private properties of $this class in required files.

nicolas-grekas · 2017-08-03T16:07:18Z

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

-        // with proxies, for 5.3.3 compatibility, the getter must be public to be accessible to the initializer
-        $isProxyCandidate = $this->getProxyDumper()->isProxyCandidate($definition);
-        $visibility = $isProxyCandidate ? 'public' : 'protected';
+        $asFile = $this->asFiles && $definition->isShared();


non-shared services are not split because they can be created several times (that's their purpose) and that would mean paying for "require" several times (for shared services, the code needs to be loaded anyway, so there is no extra "tax")

nicolas-grekas · 2017-08-03T16:11:11Z

Failures are false positives :)

robfrawley · 2017-08-03T19:55:24Z

Here are some benchmarks and cache comparisons for an older, legacy Symfony app that is halfway through being rewritten with some modern principals in mind. It has an overly bloated service container that only recently has gone public: false by default.

Beyond the shown comparisons below, I generally found slight performance increases, sometimes alongside slight memory increases, with only a few situations that resulted in minor performance regressions (often within a margin of error). The cache size is significantly larger, but it is still a negligible size, anyway, so this doesn't particularly bother me.

Benchmarks

Controller	Symfony Vers	Difference	Blackfire
DefaultController::index	3.3 -> 3.4	`+2.71%`	Graph
	3.4 -> PR#23741	`-4.39%`	Graph
CouponController::list	3.3 -> 3.4	`+0.59%`	Graph
	3.4 -> PR#23741	`-2.14%`	Graph
CategoryController::list	3.3 -> 3.4	`-2.62%`	Graph
	3.4 -> PR#23741	`-2.43%`	Graph

Cache Size

Symfony 3.3

Size	File
188K	var/cache/dev/appDevDebugProjectContainerCompiler.log
4.0K	var/cache/dev/appDevDebugProjectContainerDeprecations.log
580K	var/cache/dev/appDevDebugProjectContainer.php
72K	var/cache/dev/appDevDebugProjectContainer.php.meta
28K	var/cache/dev/appDevDebugProjectContainerUrlGenerator.php
92K	var/cache/dev/appDevDebugProjectContainerUrlGenerator.php.meta
28K	var/cache/dev/appDevDebugProjectContainerUrlMatcher.php
92K	var/cache/dev/appDevDebugProjectContainerUrlMatcher.php.meta
536K	var/cache/dev/appDevDebugProjectContainer.xml
72K	var/cache/dev/appDevDebugProjectContainer.xml.meta
1692K	Total

Symfony 3.4

Size	File
188K	var/cache/dev/appDevDebugProjectContainerCompiler.log
4.0K	var/cache/dev/appDevDebugProjectContainerDeprecations.log
580K	var/cache/dev/appDevDebugProjectContainer.php
72K	var/cache/dev/appDevDebugProjectContainer.php.meta
28K	var/cache/dev/appDevDebugProjectContainerUrlGenerator.php
92K	var/cache/dev/appDevDebugProjectContainerUrlGenerator.php.meta
28K	var/cache/dev/appDevDebugProjectContainerUrlMatcher.php
92K	var/cache/dev/appDevDebugProjectContainerUrlMatcher.php.meta
512K	var/cache/dev/appDevDebugProjectContainer.xml
72K	var/cache/dev/appDevDebugProjectContainer.xml.meta
1668K	Total

Symfony 3.4 (with PR #23741)

Size	File
1843K	var/cache/dev/appDevDebugProjectContainer
188K	var/cache/dev/appDevDebugProjectContainerCompiler.log
4.0K	var/cache/dev/appDevDebugProjectContainerDeprecations.log
188K	var/cache/dev/appDevDebugProjectContainer.php
72K	var/cache/dev/appDevDebugProjectContainer.php.meta
28K	var/cache/dev/appDevDebugProjectContainerUrlGenerator.php
92K	var/cache/dev/appDevDebugProjectContainerUrlGenerator.php.meta
28K	var/cache/dev/appDevDebugProjectContainerUrlMatcher.php
92K	var/cache/dev/appDevDebugProjectContainerUrlMatcher.php.meta
512K	var/cache/dev/appDevDebugProjectContainer.xml
72K	var/cache/dev/appDevDebugProjectContainer.xml.meta
3119K	Total

nicolas-grekas · 2017-08-03T20:09:44Z

Thanks for the numbers. The size of the folder doesn't matter at all to me btw.

nicolas-grekas · 2017-08-04T14:39:14Z

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

+    }
+}
+
+return new Container{$hash}();


This is a very important piece added just now, permitted by this PR. It fixes an architectural issue with the dumped class. Here, instead of defining the container class straight away, we turn the file into a container factory. For BC, a class is created with the expected name. But under the hood, we generated a container class whose name contains a hash of the dumped code. This class, and all service factory files are put into a directory that is named after this same hash.

This way, there can be no collision between two container's cache. This makes it easy to deal with atomicity when writting files to disk, and makes it easy also to generate two different containers side-by-side (this last aspect will be leveraged in a later PR to fix the cache:clear command.)

So, now on, this PR is not only a small but nice perf improvement. It's also a architectural enhancement.

fabpot · 2017-08-07T16:49:19Z

Thank you @nicolas-grekas.

…ekas) This PR was merged into the 3.4 branch. Discussion ---------- [DI] Generate one file per service factory | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #23601 | License | MIT | Doc PR | - See #23678 for background on this proposal. Commits ------- 4037009 [DI] Generate one file per service factory

sstok · 2017-08-08T10:10:15Z

FYI some services generate a rather long file-name,
service park_manager.domain_event_listener.update_auth_token_when_password_was_changed.administrator generates getParkManager_DomainEventListener_UpdateAuthTokenWhenPasswordWasChanged_AdministratorService.php (99 characters).

I'm not sure it's worth a "fix", maybe start a poll to find-out what's the average maximum length developers use for there service-id's, and another concern is the amount of files in a single directory (Cache pools use (whatever this is called) to bypass this problem).

nicolas-grekas · 2017-08-08T12:11:33Z

generate a rather long file-name

that may be an issue on Windows - not on other OSes. FYI, FQCN ids have their namespace removed when generating the file name.

amount of files in a single directory

not sure it matters at all with OPcache (and modern filesystems)

nicolas-grekas added this to the 3.4 milestone Aug 1, 2017

carsonbot added Status: Needs Review DependencyInjection Feature labels Aug 1, 2017

javiereguiluz reviewed Aug 1, 2017

View reviewed changes

nicolas-grekas force-pushed the di-split34 branch 11 times, most recently from 3717997 to e99b9a1 Compare August 3, 2017 15:45

nicolas-grekas mentioned this pull request Aug 3, 2017

[DI] Generate one file per service factory #23678

Closed

nicolas-grekas force-pushed the di-split34 branch from e99b9a1 to a5d9d90 Compare August 3, 2017 16:05

nicolas-grekas commented Aug 3, 2017

View reviewed changes

nicolas-grekas changed the title ~~[3.4][DI] Generate one file per service factory~~ [DI] Generate one file per service factory Aug 3, 2017

nicolas-grekas force-pushed the di-split34 branch 5 times, most recently from 1b89e95 to f4e6c9b Compare August 4, 2017 14:30

nicolas-grekas commented Aug 4, 2017

View reviewed changes

nicolas-grekas force-pushed the di-split34 branch 4 times, most recently from fec513e to 719e8c1 Compare August 4, 2017 20:21

nicolas-grekas mentioned this pull request Aug 4, 2017

[HttpKernel][FrameworkBundle] Add RebootableInterface, fix and un-deprecate cache:clear with warmup #23792

Merged

nicolas-grekas force-pushed the di-split34 branch from 719e8c1 to ef22c0a Compare August 4, 2017 22:07

[DI] Generate one file per service factory

4037009

nicolas-grekas force-pushed the di-split34 branch from ef22c0a to 4037009 Compare August 6, 2017 18:32

nicolas-grekas mentioned this pull request Aug 7, 2017

[Console] Symfony Console memory usage increased after upgrade to Symfony 3.3 #23601

Closed

fabpot approved these changes Aug 7, 2017

View reviewed changes

fabpot merged commit 4037009 into symfony:3.4 Aug 7, 2017

nicolas-grekas deleted the di-split34 branch August 7, 2017 18:06

mvrhov mentioned this pull request Aug 12, 2017

[DI] disk filling feature :) #23872

Closed

TomasVotruba mentioned this pull request Sep 19, 2017

[EasyCodingStandard] More informational errors. deprecated-packages/symplify#304

Closed

chalasr mentioned this pull request Oct 6, 2017

[DI] Throw useful error msg when trying to access not-found but private service #24444

Closed

This was referenced Oct 18, 2017

Release v3.4.0-BETA1 #24609

Merged

Release v4.0.0-BETA1 #24612

Merged

mimol91 mentioned this pull request Nov 3, 2017

[Dependency Injection] No Circular reference warning for Symfony 3.4 & Doctrine #24775

Closed

kbond mentioned this pull request Nov 9, 2017

[WebServerBundle] Compile error when stopping the webserver after cache clear #24885

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DI] Generate one file per service factory #23741

[DI] Generate one file per service factory #23741

nicolas-grekas commented Aug 1, 2017 •

edited

Loading

javiereguiluz left a comment

nicolas-grekas commented Aug 1, 2017

nicolas-grekas left a comment

nicolas-grekas Aug 3, 2017

nicolas-grekas Aug 3, 2017 •

edited

Loading

nicolas-grekas Aug 3, 2017

nicolas-grekas Aug 3, 2017

nicolas-grekas Aug 3, 2017

nicolas-grekas Aug 3, 2017 •

edited by stof

Loading

nicolas-grekas commented Aug 3, 2017

robfrawley commented Aug 3, 2017 •

edited

Loading

nicolas-grekas commented Aug 3, 2017

nicolas-grekas Aug 4, 2017 •

edited

Loading

fabpot commented Aug 7, 2017

sstok commented Aug 8, 2017 •

edited

Loading

nicolas-grekas commented Aug 8, 2017 •

edited

Loading

[DI] Generate one file per service factory #23741

[DI] Generate one file per service factory #23741

Conversation

nicolas-grekas commented Aug 1, 2017 • edited Loading

javiereguiluz left a comment

Choose a reason for hiding this comment

nicolas-grekas commented Aug 1, 2017

nicolas-grekas left a comment

Choose a reason for hiding this comment

nicolas-grekas Aug 3, 2017

Choose a reason for hiding this comment

nicolas-grekas Aug 3, 2017 • edited Loading

Choose a reason for hiding this comment

nicolas-grekas Aug 3, 2017

Choose a reason for hiding this comment

nicolas-grekas Aug 3, 2017

Choose a reason for hiding this comment

nicolas-grekas Aug 3, 2017

Choose a reason for hiding this comment

nicolas-grekas Aug 3, 2017 • edited by stof Loading

Choose a reason for hiding this comment

nicolas-grekas commented Aug 3, 2017

robfrawley commented Aug 3, 2017 • edited Loading

Benchmarks

Cache Size

Symfony 3.3

Symfony 3.4

Symfony 3.4 (with PR #23741)

nicolas-grekas commented Aug 3, 2017

nicolas-grekas Aug 4, 2017 • edited Loading

Choose a reason for hiding this comment

fabpot commented Aug 7, 2017

sstok commented Aug 8, 2017 • edited Loading

nicolas-grekas commented Aug 8, 2017 • edited Loading

nicolas-grekas commented Aug 1, 2017 •

edited

Loading

nicolas-grekas Aug 3, 2017 •

edited

Loading

nicolas-grekas Aug 3, 2017 •

edited by stof

Loading

robfrawley commented Aug 3, 2017 •

edited

Loading

nicolas-grekas Aug 4, 2017 •

edited

Loading

sstok commented Aug 8, 2017 •

edited

Loading

nicolas-grekas commented Aug 8, 2017 •

edited

Loading