[Performance] Implement command loader to reduce initialization time of Magento CLI

[mattwellss]

Description (*)

This change reduces initialization time of the Magento CLI app by allowing deferred initialization of commands until they're needed.

• Implements the Symfony CommandLoaderInterface three times:
  1. \Magento\Setup\Console\CommandLoader, a replacement for Magento_Setup's CommandList
  2. \Magento\Framework\Console\CommandLoader, an alternative means of adding commands outside the Magento\Setup namespace
  3. \Magento\Framework\Console\CommandLoader\Aggregate, an aggregate command loader, necessary because a Symfony CLI app can only have one Command Loader.

Related Pull Requests

None

Fixed Issues (if relevant)

1. Fixes magento/magento2#29266

Manual testing scenarios (*)

1. Check out the 2.4-develop branch
2. Run a console command through time; for exmaple: time ./bin/magento setup:install --help and note how long it takes
3. Check out this branch locally (mattwellss:implement-command-loader)
4. Run the same console command again, and note that it is significantly faster than before

Questions or comments

• Should the "Aggregate" command loader be renamed or implemented a different way? (Extending \Magento\Framework\ObjectManager\TMap seems feasible)
• I identified setup:install and setup:config:set as the two commands causing the most slowdown, so I focused first on migrating the commands in Magento_setup. I'm happy to migrate more commands to use the Command Loader, but it seems unnecessary from a performance perspective.
• Should Magento\Setup\Console\CommandList be removed entirely, or just have its getCommandsClasses() method updated to return an empty array? The latter would preserve BC, in the case that people have extended the class with their own commands.

Contribution checklist (*)

• [ ] Pull request has a meaningful description of its purpose
• [ ] All commits are accompanied by meaningful commit messages
• [ ] All new or changed code is covered with unit/integration tests (if applicable)
• [ ] All automated tests passed successfully (all builds are green)

[m2-assistant[bot]]

Hi @mattwellss. Thank you for your contribution Here is some useful tips how you can test your changes using Magento test environment. Add the comment under your pull request to deploy test or vanilla Magento instance:

• @magento give me test instance - deploy test instance based on PR changes
• @magento give me 2.4-develop instance - deploy vanilla Magento instance

:exclamation: Automated tests can be triggered manually with an appropriate comment:

• @magento run all tests - run or re-run all required tests against the PR changes
• @magento run <test-build(s)> - run or re-run specific test build(s) For example: @magento run Unit Tests

<test-build(s)> is a comma-separated list of build names. Allowed build names are:

1. Database Compare
2. Functional Tests CE
3. Functional Tests EE,
4. Functional Tests B2B
5. Integration Tests
6. Magento Health Index
7. Sample Data Tests CE
8. Sample Data Tests EE
9. Sample Data Tests B2B
10. Static Tests
11. Unit Tests
12. WebAPI Tests

You can find more information about the builds here

:information_source: Please run only needed test builds instead of all when developing. Please run all test builds before sending your PR for review.

For more details, please, review the Magento Contributor Guide documentation.

:warning: According to the Magento Contribution requirements, all Pull Requests must go through the Community Contributions Triage process. Community Contributions Triage is a public meeting.

:clock10: You can find the schedule on the Magento Community Calendar page.

:telephone_receiver: The triage of Pull Requests happens in the queue order. If you want to speed up the delivery of your contribution, please join the Community Contributions Triage session to discuss the appropriate ticket.

:movie_camera: You can find the recording of the previous Community Contributions Triage on the Magento Youtube Channel

:pencil2: Feel free to post questions/proposals/feedback related to the Community Contributions Triage process to the corresponding Slack Channel

[m2-assistant[bot]]

Hi @mattwellss, thank you for your contribution! Please, complete Contribution Survey, it will take less than a minute. Your feedback will help us to improve contribution process.

[m2-assistant[bot]]

Hi @mattwellss. Thank you for your contribution Here is some useful tips how you can test your changes using Magento test environment. Add the comment under your pull request to deploy test or vanilla Magento instance:

• @magento give me test instance - deploy test instance based on PR changes
• @magento give me 2.4-develop instance - deploy vanilla Magento instance

:exclamation: Automated tests can be triggered manually with an appropriate comment:

• @magento run all tests - run or re-run all required tests against the PR changes
• @magento run <test-build(s)> - run or re-run specific test build(s) For example: @magento run Unit Tests

<test-build(s)> is a comma-separated list of build names. Allowed build names are:

1. Database Compare
2. Functional Tests CE
3. Functional Tests EE,
4. Functional Tests B2B
5. Integration Tests
6. Magento Health Index
7. Sample Data Tests CE
8. Sample Data Tests EE
9. Sample Data Tests B2B
10. Static Tests
11. Unit Tests
12. WebAPI Tests

You can find more information about the builds here

:information_source: Please run only needed test builds instead of all when developing. Please run all test builds before sending your PR for review.

For more details, please, review the Magento Contributor Guide documentation.

[ihor-sviziev]

@magento run all tests

[mattwellss]

@gabrieldagama, thanks for the review. I appreciate it!

Regarding performance:

1. The biggest improvement will be noticed when running a common command rather than the two which cause the biggest performance drain, cache:clean is a good example This is because my intent is to improve performance when commands such as setup:install don't need to be initialized. If the setup:install command is initialized, the performance benefit of the command loader is partially lost
2. The improvements made here are largely overshadowed by DI initialization (and all the other uncached stuff that has to happen when the cache is empty), so my goal is to improve performance while the cache is warm

Some results:

On 2.4-develop:

./bin/magento cache:clean  0.70s user 0.16s system 98% cpu 0.881 total

On implement-command-loader:

./bin/magento cache:clean  0.58s user 0.15s system 97% cpu 0.752 total

Let me know if you see similar percentage improvement (the command loader version finishes in about 85% of the time of the base branch).

On my installation of Magento, there are 57 commands added via the Magento\Framework\Console\CommandList. All of these can be migrated to use the CommandLoader if we want to squeeze a bit more performance benefit out of the change. I'm not sure if we'll see much improvement, though... when I profiled a ./bin/magento list command, only the setup:install and setup:config:set commands stood out as particularly slow to initialize.

[gabrieldagama]

Thanks for the detailed reply @mattwellss! I understand your point, I will do a further investigation here, but on my environment, with warmed cache, there is not much difference in performance.

Regards the other using CommandLoader for the other commands, it may be backward incompatible, currently, developers rely on CommandListInterface to add new commands, so any change there will be backward-incompatible (https://devdocs.magento.com/guides/v2.4/extension-dev-guide/cli-cmds/cli-howto.html). That doesn't mean it is not possible, but we may need to be a little bit more careful if we do it, but as you said, if there are no real performance improvements, it may not be necessary.

Thanks!

[gabrieldagama]

@magento run all tests

[mattwellss]

@gabrieldagama regarding:

Regards the other using CommandLoader for the other commands, it may be backward incompatible, currently, developers rely on CommandListInterface to add new commands, so any change there will be backward-incompatible (https://devdocs.magento.com/guides/v2.4/extension-dev-guide/cli-cmds/cli-howto.html). That doesn't mean it is not possible, but we may need to be a little bit more careful if we do it, but as you said, if there are no real performance improvements, it may not be necessary.

That's a great point about BC and also documentation. Here's how I'd work around it:

• @deprecate the CommandListInterface and CommandList in framework code
• Migrate commands which rely on the command list to use command loader
• Leave the command list initialization in place (\Magento\Framework\Console\Cli::getApplicationCommands, \Magento\Framework\Console\Cli::getVendorCommands) for custom and extension code that relies on it
• Update documentation to recommend use of CommandLoader

That approach a) won't break any existing code, b) helps to "document by example" (in addition to regular documentation), c) gains whatever performance improvement is available.

Thanks again for reviewing. Let me know what you think.

[gabrieldagama]

Hi @mattwellss, agree with you on steps there to introduce CommandLoader to general commands (outside Setup module). I guess we can create a separate issue for it and work this out on separate PR (once this gets merged), what do you think?

There are some small fixes that are required from the Static Tests, are you able to address them? The functional failures aren't related to this PR.

Thanks for your contribution!

[mattwellss]

I guess we can create a separate issue for it and work this out on separate PR (once this gets merged), what do you think?

Good plan

Regarding static test failures, I'm not sure what to do about this error:

Data set: lib/internal/Magento/Framework/Console/Cli.php Cli.php depends for non-library classes Magento\Setup\Console\CommandLoader Magento\Setup\Console\CommandLoader Magento\Setup\Console\CommandLoader

It appears to be repeated a large number of times in testCheckDependencies. I believe the error should've already existed for Magento\Setup\ConsoleCommandList's usage inside Cli as well. I made sure to check class_exists before creating a new instance, so even if Magento Setup code is removed, the Cli should still run.

[mattwellss]

@magento run all tests

[engcom-Echo]

Hi, @mattwellss are you going to continue progress on that? Thanks!

[mattwellss]

Hey, sorry about the delay. I got busy with other work. I fixed the errors found by Magento\Test\Php\LiveCodeTest, aside from those in *Command classes (which got caught up in the validation because I modified them). The errors in those classes, such as phpcs direct throw warnings in the AdminUserCreateCommand, predated these changes, so I didn't attempt to fix them. Otherwise, the code quality checks should be fine. If this is fine, please request a test run and review the code.

[mattwellss]

@engcom-Echo, please note my comment from a couple weeks ago.

[mattwellss]

@ihor-sviziev, thanks for giving this some attention! I'll take a look at your feedback in the next few days and change the target to 2.5-dev.

[engcom-Charlie]

@magento run all tests

[engcom-Charlie]

Hello @gabrieldagama could you please help to understand why test builds fall without reports? Thank you.

[ihor-sviziev]

@magento run all tests

[ihor-sviziev]

@gabrieldagama @sidolov @sivaschenko could you help to understand why tests are failing with not build reports?

[ihor-sviziev]

@magento run all tests

[mattwellss]

@ihor-sviziev, looks like that command was moved from Magento_Setup to Magento_Backend.

https://github.com/magento/magento2/commit/4e40bc152a530d0bf549adb36817b1d5fced9e8b#diff-8c9bf1dd971391daf0211e3b5c134f73ab4a0bf76a78eda7c7e6e0afe2190d7e

Because (at the moment) this PR's scope is limited to updating Magento_Setup to use command loader, I simply removed the references to the "maintenance" commands from \Magento\Setup\Console\CommandLoader::$commands

edit: Also, sorry for being absent for the past few weeks. Work has been pretty busy!

[ihor-sviziev]

@magento run all tests

[ihor-sviziev]

@magento run all tests

[ihor-sviziev]

@magento run Functional Tests B2B

[magento-engcom-team]

Hi @ihor-sviziev, thank you for the review. ENGCOM-8747 has been created to process this Pull Request :eight_spoked_asterisk: @ihor-sviziev, could you please add one of the following labels to the Pull Request?

Label	Description
Auto-Tests: Covered	All changes in Pull Request is covered by auto-tests
Auto-Tests: Not Covered	Changes in Pull Request requires coverage by auto-tests
Auto-Tests: Not Required	Changes in Pull Request does not require coverage by auto-tests

[magento-engcom-team]

@mattwellss thank you for contributing. Please accept Community Contributors team invitation here to gain extended permissions for this repository.

[ihor-sviziev]

@magento run Functional Tests B2B

[engcom-Lima]

@magento run all tests

[magento-automated-testing[bot]]

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please message the #magento-devops slack channel if they don't show in a reasonable amount of time and a representative will look into any issues.

[ihor-sviziev]

@engcom-Lima, maybe we can move the changes on top of 2.4-develop branch and move it forward?