Skip to content
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

[9.x] Add support for lazy loading commands #34873

Merged
merged 2 commits into from
Oct 21, 2020
Merged

[9.x] Add support for lazy loading commands #34873

merged 2 commits into from
Oct 21, 2020

Conversation

paras-malhotra
Copy link
Contributor

@paras-malhotra paras-malhotra commented Oct 17, 2020

Currently, all commands in Laravel have to be instantiated to be registered in the console application. That could be a problem for commands with big dependency graphs as all the dependencies are loaded in memory. See laravel/ideas#1399.

Symfony has had lazy loading support for commands since version 3.4. This PR extends Symfony's lazy loading to Laravel. To allow for lazy loading, Laravel package console commands and/or app console commands just need to set the static $defaultName property like so:

/**
 * The default command name for lazy loading.
 *
 * @var string|null
*/
protected static $defaultName = 'command:name';

Since the PR changes the return type of Application::resolve (adds a null return type), I'm sending this to master instead of 8x. If this PR is accepted, I can send across another PR to lazy load the commands shipped with Laravel.

@taylorotwell
Copy link
Member

taylorotwell commented Oct 21, 2020

@paras-malhotra it seems like we could make the getDefaultName() method have a default implementation in our console command that gets $signature before the first white-space. That way it wouldn't typically need to be defined in commands? Is that a bad idea?

@paras-malhotra
Copy link
Contributor Author

@taylorotwell the signature is a non-static variable and that's the real issue here (cannot be accessed from static methods). The getDefaultName is a static method and it's structured that way in Symfony so that there's no need to instantiate the commands to access the default name.

@taylorotwell taylorotwell merged commit 958d02b into laravel:master Oct 21, 2020
@taylorotwell
Copy link
Member

Thanks. Does it make sense to add a defaultName to some of Laravel's own heavier commands?

@paras-malhotra paras-malhotra deleted the lazy_load_commands branch October 21, 2020 13:39
@paras-malhotra
Copy link
Contributor Author

Yes @taylorotwell, I'll send across a follow-up PR to add default names to the commands shipped with Laravel. That should be a nice performance boost.

@garygreen
Copy link
Contributor

garygreen commented Aug 30, 2021

@paras-malhotra does this actually work? It still has to resolve all of the console commands in order to get the name:

            $this->artisan = (new Artisan($this->app, $this->events, $this->app->version()))
                                    ->resolveCommands($this->commands)
                                    ->setContainerCommandLoader();

... then when resolvedCommands is called:

    /**
     * Add a command, resolving through the application.
     *
     * @param  string  $command
     * @return \Symfony\Component\Console\Command\Command|null
     */
    public function resolve($command)
    {
        if (class_exists($command) && ($commandName = $command::getDefaultName())) {
            $this->commandMap[$commandName] = $command;

            return null;
        }

        return $this->add($this->laravel->make($command));
    }

It calls $command::getDefaultName() which then LOADS the class in memory. It doesn't matter that it's a static function or not, it still has to load up the class which then consumes memory?

@garygreen
Copy link
Contributor

garygreen commented Aug 30, 2021

I just tested with a large class based class file and when making static calls to a class it does indeed load it in memory as can be observed with get_declared_classes and memory_get_usage shows it's using memory.

It's likely still much lower than actually resolving the classes from the container, but still room for optimization so the classes aren't loaded from disk at all. Though I'm not sure how this plays with OpCache as it may have those classes in memory already.

bennothommo added a commit to wintercms/storm that referenced this pull request Dec 14, 2021
LukeTowers pushed a commit to wintercms/storm that referenced this pull request Dec 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants