-
Notifications
You must be signed in to change notification settings - Fork 11.5k
[9.x] Add command caching (deferred/lazy resolving of commands) for faster command loading, reduced memory #38598
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Is this a new Symfony Console feature? |
It's related to this feature: https://symfony.com/doc/current/console/lazy_commands.html @paras-malhotra added support for it in PR #34873 and this PR follows on from that by avoiding a |
7856fa2
to
433d49a
Compare
433d49a
to
39e5a44
Compare
@taylorotwell how do you feel about allowing artisan commands to be called by class name? This doesn't sound all that useful on the face of it - but it would allow being able to schedule commands without having to resolve them: php artisan App\Console\DeleteAccountsCommand This would allow you to do something like this in protected function schedule(Schedule $schedule)
{
$schedule->command(DeleteAccountsCommand::class)->hourly();
} Currently you CAN register your classes like this, but unfortunately it means they are all resolved through the container /**
* Add a new Artisan command event to the schedule.
*
* @param string $command
* @param array $parameters
* @return \Illuminate\Console\Scheduling\Event
*/
public function command($command, array $parameters = [])
{
if (class_exists($command)) {
$command = Container::getInstance()->make($command)->getName();
}
return $this->exec(
Application::formatCommandString($command), $parameters
);
} By allowing commands to be called by class names you could remove that resolving. Alternatively, need a way of caching the command map. |
@garygreen what was the reason for |
@taylorotwell I can totally understand that. It was due to not setting a default type here - so when attempting to loop over the list looking for a command it errored. src/Illuminate/Console/ContainerCommandLoader.php /**
* A list of class names.
*
* @var array
*/
protected $classes; Changed to: /**
* A list of class names.
*
* @var array
*/
protected $classes = []; |
Let me do some more testing because it's odd that |
Ok so I've done some more testing - it does make sense I've added some tests to check the operation of the command loader, as tests were missing for this. So it looks all good! |
4bda8a6
to
c3130ac
Compare
Co-authored-by: Julius Kiekbusch <jubeki99@gmail.com>
@garygreen so to clarify, based on my testing, this seems to only be for application defined commands, not for Laravel's own internal commands, right? |
Marking this as draft while you're working on it. Feel free to mark as active when it is ready. |
I've done some further tests on this - as it stands with this PR it will now defer resolving of most of Laravel core commands (aside from a few that don't have a In terms of numbers of what this PR does (without the config caching commit): Deferred Resolving: around 0.5MB memory reduction (-4%), 10ms decrease. This stat is quite disappointing. However, what I really wanted to do initially with this PR is to completely prevent loading of the command classes. To do this would require caching the command map/list. I've got an initial implementation working (pushed to this PR) which yields much more promising numbers: + Caching command list: around 2.5MB memory reduction (-17%), 33ms decrease This is based on core Laravel install without any user commands. The way this works is by caching the list of commands in And to generate the list you would use the From a technical perspective, if using I would be intrigued to see what effect this has on larger apps - @taylorotwell you have a Laravel service which is a UI library from what I remember and it's heavy on commands? Would you be willing to test on that? If you want me to carry on with this PR and the caching idea let me know Updated original post with checklist. |
@garygreen please remember that Taylor doesn't sees draft PR's. Feel free to mark this as ready for review if you want him to review again. |
I'm going to close this for now. Feel free to resubmit once you're ready to work on this again 👍 |
This PR fully defers resolving of commands (originally not working prior to this commit) + adds additional caching layer so that command files are not hit on disk at all.
Early testing yields around 17% memory reduction (2.5MB) on a base Laravel install without any user commands.
Results could be higher with larger apps with lots of commands.
This follows the same kind of principle behind config and route caching.
Related: #34873