Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[ClassLoader] Add ClassCollectionLoader::inline() to generate inlined-classes files#19276
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
nicolas-grekas commentedJul 4, 2016 • 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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
What if someone extended this class?
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.
Nobody does that for such a strange and static method 😃
What would be the issue anyway?
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.
There will always be someone doing the not expected things. ;)
The issue is that before they could perform strict comparisons fortrue andfalse being passed. This now would fail when an array got passed.
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.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'm going to split the method in two
nicolas-grekas commentedJul 5, 2016 • 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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
What about settingdeclaredClasses private with only a getter and modifyinggetTempKernel 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 to3225370Comparenicolas-grekas commentedJul 6, 2016
I think I found the way to go: using |
5d2217b to965e8ebComparenicolas-grekas commentedJul 6, 2016
Just added |
| if ($autoReload) { | ||
| // save the resources | ||
| self::writeCacheFile($metadata,serialize(array($files,$classes))); |
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 broken, because$files is modified intoinline.
Savign resources should stay at the same place than saving the main file btw.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
but then, shouldinline really write the file if it does not write the meta file ?
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.
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 to980b101Comparea3356a4 tofc78bb7Comparenicolas-grekas commentedJul 10, 2016
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'll see what can do. Btw, would it make sense to move the composer script from distributionbundle to frameworkbundle?
nicolas-grekasJul 10, 2016 • 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.
Alternatively, what about moving this list in distributionbundle? => not possible, the bundle is not enabled inprod env
fabpot commentedJul 10, 2016
I like this approach. Being to have some tests somewhere about this one would be great. |
nicolas-grekas commentedJul 17, 2016
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. |
fabpot commentedJul 18, 2016
Thank you@nicolas-grekas. |
Uh oh!
There was an error while loading.Please reload this page.
Unfortunately, can't be tested because the method relies too much on side effects.
Coupled withsensiolabs/SensioDistributionBundle#272, allows inlining
ClassCollectionLoaderitself into thebootstrap.php.cachefile.