[ClassLoader] Add ClassCollectionLoader::inline() to generate inlined-classes files #19276

nicolas-grekas · 2016-07-03T09:34:54Z

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

Unfortunately, can't be tested because the method relies too much on side effects.
Coupled with sensiolabs/SensioDistributionBundle#272, allows inlining ClassCollectionLoader itself into the bootstrap.php.cache file.

nicolas-grekas · 2016-07-04T09:29:12Z

Status: needs work
The warm up of the classes.php file is broken: it should inline many more classes but because it excludes currently declared ones, many are excluded.
I'd like to inspect this behavior and it's related to this PR.

xabbuh · 2016-07-04T10:10:36Z

src/Symfony/Component/ClassLoader/ClassCollectionLoader.php

+     * @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


What if someone extended this class?

Nobody does that for such a strange and static method 😃
What would be the issue anyway?

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.

@xabbuh please suggest alternatives, or surrender :-) The whole benefit of the PR is worth it, don't you think?

I'm going to split the method in two

nicolas-grekas · 2016-07-05T18:58:50Z

Status: needs review.

This is not finished, yet it's a complex topic. The classes.php file is "broken" on 3.1.
Since #16263, this file is created by a cache warmer, but its generation is affected by the current list of declared classes. This list happens to be quite big when the cache warmer is executed, which means many classes are excluded from inlining.

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:

SessionListener, NativeSessionStorage, NativeSessionHandler, NativeFileSessionHandler, SessionHandlerProxy, Session, TemplateNameParser, TemplateNameParser, TemplateLocator, UrlGenerator, RequestContext, Router, UrlMatcher, RedirectableUrlMatcher, Router, FileLocator, Event, EventDispatcher, ContainerAwareEventDispatcher, ResponseListener, RouterListener, ControllerResolver, ArgumentMetadata, KernelEvent, FilterControllerEvent, FilterResponseEvent, GetResponseEvent, GetResponseForControllerResultEvent, GetResponseForExceptionEvent, FileLocator, ControllerNameParser, ControllerResolver, Firewall, AuthenticationProviderManager, TokenStorage, AccessDecisionManager, AuthorizationChecker, FirewallMap, FirewallContext, RequestMatcher, Twig_Environment, Twig_Extension_Core, Twig_Extension_Escaper, Twig_Extension_Optimizer, NormalizerFormatter, LineFormatter, StreamHandler, FilterHandler, TestHandler, Logger, Logger, DebugHandler, ClassUtils, Registry, ControllerListener, ParamConverterListener, TemplateListener, HttpCacheListener, SecurityListener.

tgalopin · 2016-07-05T19:12:08Z

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

@@ -131,6 +131,7 @@ protected function warmup($warmupDir, $realCacheDir, $enableOptionalWarmers = tr
            $realKernelClass = substr($realKernelClass, $pos + 1);
        }
        $tempKernel = $this->getTempKernel($realKernel, $namespace, $realKernelClass, $warmupDir);
+        $tempKernel->declaredClasses = $realKernel->declaredClasses;


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).

nicolas-grekas · 2016-07-06T06:53:12Z

I think I found the way to go: using get_included_files() combined with get_declared_classes() allows filtering out early-declared classes without "snapshoting" these all the time even when not need, and without changing anything in HttpKernel.

nicolas-grekas · 2016-07-06T08:56:12Z

Just added ClassCollectionLoader::inline(), RFR

stof · 2016-07-06T09:30:57Z

src/Symfony/Component/ClassLoader/ClassCollectionLoader.php

+
+        if ($autoReload) {
+            // save the resources
+            self::writeCacheFile($metadata, serialize(array($files, $classes)));


this is broken, because $files is modified into inline.

Savign resources should stay at the same place than saving the main file btw.

Good catch, fixed by making inline() return $files. The cache invalidation logic should remain in load() so I kept the general logic as is.

but then, should inline really write the file if it does not write the meta file ?

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().

nicolas-grekas · 2016-07-10T06:55:34Z

Much simpler now, magic removed, let's be plain explicit.
Requires sensiolabs/SensioDistributionBundle#272
Status: needs review

fabpot · 2016-07-10T07:50:25Z

src/Symfony/Bundle/FrameworkBundle/Resources/config/services.xml

+                <argument>Symfony\Component\HttpKernel\Kernel</argument>
+                <argument>Symfony\Component\ClassLoader\ClassCollectionLoader</argument>
+                <argument>Symfony\Component\ClassLoader\ApcClassLoader</argument>
+            </argument>


This is fragile. We should have tests to ensure that we don't break this at some point.

I'll see what can do. Btw, would it make sense to move the composer script from distributionbundle to frameworkbundle?

~~Alternatively, what about moving this list in distributionbundle?~~ => not possible, the bundle is not enabled in prod env

fabpot · 2016-07-10T07:51:35Z

I like this approach. Being to have some tests somewhere about this one would be great.

nicolas-grekas · 2016-07-17T19:16:29Z

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.

…-classes files

…eCacheWarmer

fabpot · 2016-07-18T12:56:13Z

Thank you @nicolas-grekas.

carsonbot added Status: Needs Review ClassLoader Feature labels Jul 3, 2016

nicolas-grekas mentioned this pull request Jul 3, 2016

Optimize bootstrap.php.cache sensiolabs/SensioDistributionBundle#272

Merged

carsonbot added Status: Needs Work and removed Status: Needs Review labels Jul 4, 2016

xabbuh reviewed Jul 4, 2016
View reviewed changes

nicolas-grekas force-pushed the classcoll branch from 5e3145f to ef960d5 Compare July 5, 2016 18:49

carsonbot added Status: Needs Review and removed Status: Needs Work labels Jul 5, 2016

nicolas-grekas force-pushed the classcoll branch from ef960d5 to c552185 Compare July 5, 2016 19:03

tgalopin reviewed Jul 5, 2016
View reviewed changes

nicolas-grekas force-pushed the classcoll branch 3 times, most recently from 885f4d3 to 3225370 Compare July 6, 2016 06:46

nicolas-grekas force-pushed the classcoll branch 4 times, most recently from 5d2217b to 965e8eb Compare July 6, 2016 08:44

nicolas-grekas changed the title ~~[ClassLoader] Allow injecting declared classes to ClassCollectionLoader::load()~~ [ClassLoader] Add ClassCollectionLoader::inline() to generate inlined-classes files Jul 6, 2016

nicolas-grekas force-pushed the classcoll branch from 965e8eb to 5b07bc8 Compare July 6, 2016 09:02

stof reviewed Jul 6, 2016
View reviewed changes

nicolas-grekas force-pushed the classcoll branch 2 times, most recently from 7340dea to 980b101 Compare July 6, 2016 12:18

nicolas-grekas added Status: Needs Work and removed Status: Needs Review labels Jul 6, 2016

nicolas-grekas force-pushed the classcoll branch 2 times, most recently from a3356a4 to fc78bb7 Compare July 10, 2016 06:51

carsonbot added Status: Needs Review and removed Status: Needs Work labels Jul 10, 2016

fabpot reviewed Jul 10, 2016
View reviewed changes

nicolas-grekas force-pushed the classcoll branch from fc78bb7 to 66cc465 Compare July 17, 2016 19:14

nicolas-grekas force-pushed the classcoll branch from 66cc465 to 558ea3b Compare July 17, 2016 19:28

nicolas-grekas added 2 commits July 17, 2016 21:40

[ClassLoader] Add ClassCollectionLoader::inline() to generate inlined…

5ad1f0a

…-classes files

[FrameworkBundle] Compute list of early-declared classes in ClassCach…

99f7b5f

…eCacheWarmer

nicolas-grekas force-pushed the classcoll branch from 558ea3b to 99f7b5f Compare July 17, 2016 19:40

fabpot closed this in 583a45d Jul 18, 2016

nicolas-grekas deleted the classcoll branch July 24, 2016 16:48

fabpot mentioned this pull request Oct 27, 2016

Release v3.2.0-BETA1 #20317

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ClassLoader] Add ClassCollectionLoader::inline() to generate inlined-classes files #19276

[ClassLoader] Add ClassCollectionLoader::inline() to generate inlined-classes files #19276

[ClassLoader] Add ClassCollectionLoader::inline() to generate inlined-classes files #19276

[ClassLoader] Add ClassCollectionLoader::inline() to generate inlined-classes files #19276

Conversation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment