[Config][DependencyInjection] Optimize dumped resources for tracking #57948
Conversation
|
The failures look related to the changes. |
9fa19e3 to
a29fb1e
Compare
Yep, fixed |
4b3dc48 to
07d646d
Compare
src/Symfony/Component/Config/Resource/ReflectionClassResource.php
Outdated
Show resolved
Hide resolved
07d646d to
53da807
Compare
53da807 to
60f0fca
Compare
… on cache clear, impossible to install new project (eltharin) This PR was merged into the 7.2 branch. Discussion ---------- [DependencyInjection] Fix bug on windows, memory exhausted on cache clear, impossible to install new project | Q | A | ------------- | --- | Branch? | 7.2 | Bug fix? | yes | New feature? | no | Deprecations? | no | Issues | Fix | License | MIT Since #57948 on windows, I can't make a cache clear with a fatal error form memory exhausted or install a new symfony app. Xdebug saw an infinite loop. Correction with DIRECTORY_SEPARATOR constant instead / Commits ------- 89b4c87 bug correction
| continue; | ||
| } | ||
|
|
||
| if (!$inVendor = $this->inVendors($dir)) { |
There was a problem hiding this comment.
This logic looks wrong to me. Configuring a namespace prefixed to match a given directory does not mean that all classes matching that prefix must be defined in that folder.
For instance, all our mailer bridges have a namespace prefix that can also be matched by the prefix of the symfony/mailer package. And nothing prevents the project-level code to also define classes with that namespace (best practices is to keep your project-level code in a different namespace than your vendors, but that's not a requirement, and I know some projects do weird things).
For existing classes, we can optimize the class existence resource if the class is defined in the vendor folder (by checking the actual class definition, not folders in which it is maybe defined). For non-existent classes, we should not optimize the resource (or we should optimize it only based on all possible autoloading locations)
After reading the code, I don't understand which new classes are excluded. |
…peMC) This PR was merged into the 7.3 branch. Discussion ---------- [Config] Fix `ReflectionClassResource` hash validation | Q | A | ------------- | --- | Branch? | 7.3 | Bug fix? | yes | New feature? | no | Deprecations? | no | Issues | - | License | MIT It looks like #57948 broke hash validation. The problem is that `$excludedVendors` is now used to generate the hash but is not serialized. After unserialization, the property is empty, so the generated hash never matches the dumped one. Commits ------- cec2f00 [Config] Fix `ReflectionClassResource` hash validation
|
@GromNaN this is about skipping FileResource, DirectoryResource, FileExistenceResource and ClassExistenceResource for resources that are in the This is all based on the assumption that you don't edit manually files in the |
…e (nicolas-grekas) This PR was merged into the 7.3 branch. Discussion ---------- [DependencyInjection] Fix optimizing ClassExistenceResource | Q | A | ------------- | --- | Branch? | 7.3 | Bug fix? | yes | New feature? | no | Deprecations? | no | Issues | - | License | MIT Per `@stof` review at #57948 (review) Best reviewed [ignoring whitespaces](https://github.com/symfony/symfony/pull/61615/files?w=1). Commits ------- b7ca948 [DependencyInjection] Fix optimizing ClassExistenceResource
Looking at the resource tracking cache, I figured out we could remove tracking some more resources.
On a webapp skeleton, this goes from 85 to 39 resources to track.
On my Ubuntu, I couldn't measure a difference, but on slow filesystems (Windows, shared mounts), I hope this might improve the DX a bit during dev. Anyone able to confirm?