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] Lazy load Laravel commands #34925

Merged
merged 2 commits into from
Oct 22, 2020
Merged

[9.x] Lazy load Laravel commands #34925

merged 2 commits into from
Oct 22, 2020

Conversation

paras-malhotra
Copy link
Contributor

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

As a follow-up to #34873, this PR lazy loads all commands shipped with Laravel.

Breaking Changes
All commands were previously bound with the command.* binding names and are now bound by class name. Why? Because lazy loading implementation requires the container to resolve by class name. The command.* binding keys were unused elsewhere in the framework but if we need to support those keys as well, I can modify the PR to add aliases for those keys.

Memory Reduction
I confirmed that after this PR, the constructors of the Laravel commands are no longer invoked. With memory_get_usage, I confirmed that memory usage is now reduced by ~130KB (by lazy loading the commands shipped with Laravel). I'm not quite sure if GC is playing a role here and the actual reduction is higher. Of course, coupled with package and app commands, the memory reduction would be higher.

@taylorotwell
Copy link
Member

What is the total memory usage before and after? 130kb out of what total?

@paras-malhotra
Copy link
Contributor Author

paras-malhotra commented Oct 21, 2020

@taylorotwell, the total memory of the test command shows as 21.2 MB. The documentation on memory_get_usage reads that it triggers GC, so I'm not 100% sure that this is the only gain.

Here's the code for TestCommand@handle:

public function handle()
{
    $memory = round(memory_get_usage() / 1024 / 1024, 2);

    $this->info("Memory used: {$memory} MB");
    
    return 0;
}

@taylorotwell
Copy link
Member

So I'm curious if there is a way in ArtisanServiceProvider to feed Artisan a list of the default names directly as a large array so that the class files themselves aren't even included from disk. I think this would give quite a bit more savings.

@GrahamCampbell
Copy link
Member

Once they have been loaded once, production opcache settings prevents them being loaded again.

@paras-malhotra
Copy link
Contributor Author

paras-malhotra commented Oct 21, 2020

All we're giving the command loader is a mapping array of ['command:name' => Command::class]. We need to provide the class names so that if the command is indeed called, the loader can resolve/instantiate the command class from the container

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.

None yet

3 participants