Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[DI] Generate one file per service factory#23741
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.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
javiereguiluz left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I guess there are technical reasons to haveLazyLoadingValueHolderFactoryV1 andLazyLoadingValueHolderFactoryV2. However, usingV1 andV2 as part of the class namesfeels wrong.
Unless in the future you plan to addV3,V4, etc. could we removeV2 and useLegacy instead ofV1?
nicolas-grekas commentedAug 1, 2017
@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. |
3717997 toe99b9a1Compare
nicolas-grekas left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
PR is ready. Discussion started in#23678 so please read it to have benchs and rationales esp.
| $code =$this->dumper->getProxyFactoryCode($definition,'foo','$this->getFoo2Service(false)'); | ||
| $this->assertStringMatchesFormat( | ||
| '%wif ($lazyLoad) {%wreturn $this->services[\'foo\'] =%s' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
the format is already tested elsewhere, no need to duplicate that effort, it just makes updating tests more complex (same below)
| $filesystem->remove($oldCacheDir); | ||
| // The current event dispatcher is stale, let's not use it anymore | ||
| $this->getApplication()->setDispatcher(newEventDispatcher()); |
nicolas-grekasAug 3, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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
| thrownewServiceCircularReferenceException($id,array_keys($this->loading)); | ||
| } | ||
| if (isset($this->methodMap[$id])) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
the diff in this method is better viewed ignoring whitespaces
| $code .=" }\n"; | ||
| } | ||
| if ($this->asFiles) { | ||
| $code =str_replace("\$this->load(__DIR__.''.'","\$this->load(__DIR__.'".($asFile ?'' :'/'.$this->class),$code); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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
| /*{$this->docStar} | ||
| * {@inheritdoc} | ||
| */ | ||
| protected function load(\$file,\$lazyLoad = true) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
| // 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(); |
nicolas-grekasAug 3, 2017 • edited by stof
Loading Uh oh!
There was an error while loading.Please reload this page.
edited by stof
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 commentedAug 3, 2017
Failures are false positives :) |
robfrawley commentedAug 3, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Here are some benchmarks and cache comparisons for anolder, 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 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
Cache SizeSymfony 3.3
Symfony 3.4
Symfony 3.4 (with PR#23741)
|
nicolas-grekas commentedAug 3, 2017
Thanks for the numbers. The size of the folder doesn't matter at all to me btw. |
1b89e95 tof4e6c9bCompare| } | ||
| } | ||
| return new Container{$hash}(); |
nicolas-grekasAug 4, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
fec513e to719e8c1Comparefabpot commentedAug 7, 2017
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 commentedAug 8, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
FYI some services generate a rather long file-name, 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 commentedAug 8, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
that may be an issue on Windows - not on other OSes. FYI, FQCN ids have their namespace removed when generating the file name.
not sure it matters at all with OPcache (and modern filesystems) |
Uh oh!
There was an error while loading.Please reload this page.
See#23678 for background on this proposal.