[ClassLoader] Add ClassCollectionLoader::inline() to generate inlined-classes files #19276
Conversation
|
Status: needs work |
| * @param string $cacheDir A cache directory | ||
| * @param string $name The cache name prefix | ||
| * @param bool $autoReload Whether to flush the cache when the cache is stale or not | ||
| * @param bool|array $adaptive Whether to remove already declared classes or not, or array of already declared classes |
There was a problem hiding this comment.
What if someone extended this class?
There was a problem hiding this comment.
Nobody does that for such a strange and static method 😃
What would be the issue anyway?
There was a problem hiding this comment.
There will always be someone doing the not expected things. ;)
The issue is that before they could perform strict comparisons for true and false being passed. This now would fail when an array got passed.
There was a problem hiding this comment.
@xabbuh please suggest alternatives, or surrender :-) The whole benefit of the PR is worth it, don't you think?
There was a problem hiding this comment.
I'm going to split the method in two
|
Status: needs review. This is not finished, yet it's a complex topic. The Without the cache warmer, the file is generated very early in the bootstrapping process, with more classes being inlined. The outcome of this issue is "only" lower performance of course. This PR adds mechanisms to compute a very early declared classes list, and inject it later the cache warmer. I'm not sure the implementation is the right one, thus, I'm calling for ideas on the best way to fix this issue (reverting is not possible because that would break read-only containers based on 3.1). FYI, the list of classes that are re-inlined after using this PR on a SE is the following:
|
| $realKernelClass = substr($realKernelClass, $pos + 1); | ||
| } | ||
| $tempKernel = $this->getTempKernel($realKernel, $namespace, $realKernelClass, $warmupDir); | ||
| $tempKernel->declaredClasses = $realKernel->declaredClasses; |
There was a problem hiding this comment.
What about setting declaredClasses private with only a getter and modifying getTempKernel to set it's value in the code instead of relying on a public argument? It seems better for "backwards compatibiltiy" in the future (even if we are using the internal comment).
885f4d3 to
3225370
Compare
|
I think I found the way to go: using |
5d2217b to
965e8eb
Compare
|
Just added |
|
|
||
| if ($autoReload) { | ||
| // save the resources | ||
| self::writeCacheFile($metadata, serialize(array($files, $classes))); |
There was a problem hiding this comment.
this is broken, because $files is modified into inline.
Savign resources should stay at the same place than saving the main file btw.
There was a problem hiding this comment.
Good catch, fixed by making inline() return $files. The cache invalidation logic should remain in load() so I kept the general logic as is.
There was a problem hiding this comment.
but then, should inline really write the file if it does not write the meta file ?
There was a problem hiding this comment.
All use cases I saw so far need to write the cache. The metadata-related logic looks fine to me as a decorator thing, handled by ::load().
7340dea to
980b101
Compare
a3356a4 to
fc78bb7
Compare
|
Much simpler now, magic removed, let's be plain explicit. |
| <argument>Symfony\Component\HttpKernel\Kernel</argument> | ||
| <argument>Symfony\Component\ClassLoader\ClassCollectionLoader</argument> | ||
| <argument>Symfony\Component\ClassLoader\ApcClassLoader</argument> | ||
| </argument> |
There was a problem hiding this comment.
This is fragile. We should have tests to ensure that we don't break this at some point.
There was a problem hiding this comment.
I'll see what can do. Btw, would it make sense to move the composer script from distributionbundle to frameworkbundle?
There was a problem hiding this comment.
Alternatively, what about moving this list in distributionbundle? => not possible, the bundle is not enabled in prod env
|
I like this approach. Being to have some tests somewhere about this one would be great. |
fc78bb7 to
66cc465
Compare
|
I just added unit tests. But for the list of classes given to the cache warmer, this would require an integration test, to be done on the standard edition. Not possible here. |
66cc465 to
558ea3b
Compare
558ea3b to
99f7b5f
Compare
|
Thank you @nicolas-grekas. |
Unfortunately, can't be tested because the method relies too much on side effects.
Coupled with sensiolabs/SensioDistributionBundle#272, allows inlining
ClassCollectionLoaderitself into thebootstrap.php.cachefile.