Open gooof opened 5 years ago
@franzliedke any ideas how we would defer inclusion of some of those files by composer, or whether it's even worth doing?
Composer already only loads the files it needs. A few causes for this high amount:
As to lowering the amount. Laravel natively supports deferred service providers, causing the provider to be only loaded when any class in the provides
array of the provider is needed. I'm not sure we support that, but applying it to the MailServiceProvider - for instance - would make sense.
Solution for swiftmailer?
@gooof Thanks for raising this issue. I will have a look at this for the next beta release after the upcoming beta.8. Out of interest: a) how did you notice this, and b) what did you use to get the list of included files?
Re. service providers: At Laracon, I talked to Taylor about deferred service providers. He agreed they are mostly useless nowadays, and might be removed in a future release. (Memory savings - probably not, CPU cycle savings - meh.) They won't help much with this problem anyway: with well-written service providers, you will only avoid loading the service providers itself. Thanks to the IoC container, services that aren't needed for a request should not be instantiated, and thus not loaded.
What we see here is probably due to two things: 1.) some service providers instantiating services that aren't needed on every request, and 2.) the use of event subscribers such as this one - they need to be instantiated in order to register event listeners, which may instantiate other services (such as the mailer) that aren't needed very often.
Not very hard to fix, but we'll have to go through all service providers and extensions.
@franzliedke I notice that the API need 0.2sec to load my 3 groups on localhost and that every page has the same response time. While finding out from what the slowness comes, I'll use get_included_files() before the output happens. I also wrote a little debugger that show all sql queries, included files and times, so if a page is loading too long, I got a warning (to find (security) bugs). ~150 small included files normally = >0.05sec
A hack is using a own class that call a class only if its really called.
__construct(Mailer $mailer) -> $this->mailer = $mailer;
becomes: $this->mailer = $pseudo('mailer');
if someone called $this->mailer->send() the class check if "new mailer()" already loaded, if not he is include/loading it. Like this: https://github.com/marc1706/fast-image-size/pull/49/files
Extensions have a similar problem, in the bootstrap.php should be the event name defined where the class is required. Or the subscribe() function should be in bootstrap.php. But I wait for 0.8 to test this.
To add to the NotificationMailer
case, I noticed that NotificationSyncer
(which depends on the mailer) is included multiple times in listener constructors in core extensions (likes, suspend, ...) and are therefore unnecessarily loaded for every single request.
Most Laravel providers (including the mailer) already use singletons that should ideally never resolve if the feature isn't actually used. Right now it's not the case as email drivers are loaded for every request because of the above...
@gooof Can I ask you to share your setup that got you the list of included files in your original post? I am asking so that I can get something reproducible to test my progress. :smiley:
@gooof If you could provide the script that you used to test load time and included files, that would be great 🙂.
I tried to get the included files myself as well. This is the output of get_included_files
running after the route handler is executed. Running 98464a8a3314a22b9a6bb4ea127ad0a6b296396b, though should be the same output as with 9640dd6419f05a41c25b88992649f4f8fdbc6966.
I recently saw a tweet that mentioned adding to classes a trait that does something like this:
public static function make(...$arguments)
{
return Container::make(static::class, ...$arguments);
}
So instead of resolving classes from constructs or handle methods. We can call a class by calling it's make()
method. Eg:
use Illuminate\Event\Dispatcher;
$events = Dispatcher::make();
The Illuminate dispatcher is Macroable so the method could be added using that.
In the end this method isn't too different from using app(Dispatcher::class)
(or the future container(Dispatcher::class)
) 🤦
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. We do this to keep the amount of open issues to a manageable minimum. In any case, thanks for taking an interest in this software and contributing by opening the issue in the first place!
Bug Report
Current Behavior Currently flarum is loading over 450 files per page (with standard extensions enabled). That is a bit too much and costs much unnecessary time.
Example: swiftmailer are never need on the frontend, its only to send mails in cronjobs, after sending a post etc., no need to load these 17 files on every page.
Expected Behavior Make sure that only files are loading that used on the requested page.
Environment