- Notifications
You must be signed in to change notification settings - Fork163
Added more classes to bootstrap#76
Uh oh!
There was an error while loading.Please reload this page.
Conversation
dlsniper commentedFeb 9, 2013
I can confirm that using this I have a speed increase of about 7% while using just Anyone else willing to confirm this please? |
dlsniper commentedFeb 9, 2013
Note: If anyone wants to check this, don't forget to rebuild the bootstrap file from |
fabpot commentedFeb 9, 2013
I can confirm as I created this boostrap file for this exact reason. And just before releasing a new version of Symfony, I always add some more classes. So, thanks for taking care of that. I will have a look at it in more details early next week. |
StanAngeloff commentedFeb 11, 2013
There should be a modest improvement after this, but it's not a good idea to include classes from bundles like JMS or Assetic, they may not be present or required in a project. However, it made we wonder if it's worth investigating a way for bundles to register important classes for the bootstrap file -- could be tough given the cache is built in a Composer task. On the other hand, Composer should have knowledge of all packages and their dependencies... I'd be happy to take a stab if that's something of interest. |
Composer/ScriptHandler.php Outdated
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 these are not needed to boot the kernel. So they should be added in the second level of bootstrap instead (handled by DI extensions of bundles and refreshed automatically on changes)
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.
Thus, you are currentmly introducing a hard dependency to all these bundles here (which should then be added in the composer.json file) whereas doing it in the cleaner way would not introduce this dependency in SensioDistributionBundle
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.
@stof I agree that this would make those bundles required but in this case maybe@StanAngeloff's idea of having a way for bundles/packages registering which classes can be added to bootstrap and having a second level bootstrap file that aggregates those files in a more meaningful way would be better.
Composer already supports theextra section which addssymfony-assets-install which is specific to Symfony2 so maybe we could have asymfony-aggregate-classes as well and then have all those classes built in the second stage bootstrap file which is loaded by the first one. Does this make sense?
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 isalready such a way, in the second level of bootstrap (loaded by the$kernel->loadClasses() call in the front controller)
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.
Yup, missed that inSymfony / Component / HttpKernel / DependencyInjection / AddClassesToCachePass.php.
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.
@stof right, I've missed that as well. So in this case, all the bundles included in the standard distribution should register such a compiler pass?
If so, maybe an article for this should be added in the documentation as well, describing how the bundles authors should use this in their bundles?
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.
@dlsniper no they don't. They should call$this->addClassesToCompile in their DI extension, as done in the core bundles for instance (where some classes may need to be added):https://github.com/symfony/FrameworkBundle/blob/master/DependencyInjection/FrameworkExtension.php#L105
dlsniper commentedFeb 12, 2013
@stof would it be safe to assume the current listed bundles/bridges from this list will always be present? |
stof commentedFeb 12, 2013
@dlsniper None of the classes you are adding here are needed toboot the kernel. So they should all be moved to the second level of bootstrap file managed by bundles (the list need to be splitted between FrameworkBundle, SecurityBundle and TwigBundle at least as the security classes are not used when SecurityBundle is not used for instance) And your list still introduces a hard dependency to Twig as it includes some classes extending from Twig classes. |
dlsniper commentedFeb 13, 2013
Applying these changes in the right bundles brings the performance of 2.2 on the same level as 2.1 but unfortunately it doesn't solve the underlaying issue, the Standard Framework distribution became more heavy, nor did does it do too much good for non-Twig responses, there's a speed increase of about 2 or 3 % at most. Should I keep this open and move the conversation back to the thread in symfony/symfony-standard#464 ? |
This PR was squashed before being merged into the master branch (closes#197).Commits-------23daf05 Added classes to cacheDiscussion----------Added classes to cacheHello,This commit adds some used classes to Symfony2 cache. See the talk insensiolabs/SensioDistributionBundle#76Let me know if it needs anything else/if I've did it against the wrong branch (it should go into 2.2 distro).Thanks.---------------------------------------------------------------------------by dlsniper at 2013-02-28T07:43:38ZWhen will this be merged? ping :)---------------------------------------------------------------------------by fabpot at 2013-02-28T07:56:06ZWe should add the classes only if we are sure that they will be used. So, they should be added only when the related configuration setting is enabled.---------------------------------------------------------------------------by dlsniper at 2013-02-28T12:43:05ZI think this should be a little bit better now.
I'm not sure if I understood correctly what's the purpose of the bootstrap file and if this is the right way of doing it.
I've noticed during the tests for symfony/symfony-standard#464 I've did that the autoloader can take quite a large number of files to load from, even with apc loader on, so I've added as many file as I've could without breaking the Welcome page or the console.
I'll do more tests about this later on today but if anyone else has a better idea/or I'm doing all sorts of wrong here then let me know.
Also, in my initial tests, this adds a nice speed bump but I'll need a better computer to confirm the initial tests.