[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 Support for Symfony's CommandLoaderInterface in Magento CLI #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]

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

❗ 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

ℹ️ 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.

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

🕙 You can find the schedule on the Magento Community Calendar page.

📞 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.

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

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

[lbajsarowicz]

Just sharing my thoughts seeing your work :)

[m2-assistant]

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]

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

❗ 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

ℹ️ 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

[gabrieldagama]

Hi @mattwellss, thanks for the pull request, it looks great! Please follow some small comments on it.

I can definitely see the benefit of using CommandLoader over CommandList, by deferring the creation of the command objects just when we actually call it. From the performance perspective, I'm not having that much of a difference, on my tests, I'm clearing the cache on both runs to make sure the performance is actually increased.

So when changing branches I'm actually running:
bin/magento cache:clean && time bin/magento setup:install --help

Running on 2.4-develop:
bin/magento setup:install --help 13.61s user 0.85s system 91% cpu 15.854 total

Running on implement-command-loader:
bin/magento setup:install --help 13.38s user 0.79s system 92% cpu 15.309 total

Could you confirm if the performance increase is similar to what I'm getting here?

Let me know your thoughts! Thanks!

[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

[engcom-Charlie]

@magento run Functional Tests B2B, Functional Tests CE, Functional Tests EE, Integration Tests, Semantic Version Checker, Static Tests, Unit Tests

[engcom-Charlie]

@magento run Functional Tests B2B, Functional Tests CE, Functional Tests EE, Integration Tests, Unit Tests

[engcom-Charlie]

@magento run Functional Tests B2B, Functional Tests CE, Functional Tests EE

[engcom-Charlie]

Hi @mattwellss and @ihor-sviziev,

Kindly look into this comment and please provide you input so that we can proceed the PR further.

Thank you!

[hostep]

OUTPUT on PR branch
./bin/magento cache:clean --help  3.81s user 0.21s system 99% cpu 4.050 total

There is no way this output is accurate. Probably your computer was working hard on something else while you tested it, which caused the result to take a lot longer then it should?

Best is probably to run your tests up to 5 or 10 different times, drop the highest and lowest value and make an average of the other runs.

[engcom-Charlie]

OUTPUT on PR branch
./bin/magento cache:clean --help  3.81s user 0.21s system 99% cpu 4.050 total

There is no way this output is accurate. Probably your computer was working hard on something else while you tested it, which caused the result to take a lot longer then it should?

Best is probably to run your tests up to 5 or 10 different times, drop the highest and lowest value and make an average of the other runs.

Hi @hostep,

As you see above comment threads, many times we have tested this and conveyed the same here. This above result also posted after many execution.

Thank you!

[engcom-Charlie]

We have tested the PR once again and the results are as below. On PR branch the initialisation time is reduced compared to Magento 2.4 develop hence moving this PR to Extended Testing now.

On Magento 2 without extension

On PR branch without extension

On Magento 2 with extension

On PR branch with extension

[engcom-Charlie]

@magento run Functional Tests B2B, Functional Tests CE, Functional Tests EE

[engcom-Charlie]

@magento run Functional Tests B2B, Functional Tests CE, Functional Tests EE

[engcom-Charlie]

Failures of Functional CE, B2B and EE are known open issues related to Page builder repo. Hence moving this to Merge in Progress.

Functional B2B:
Run 1:
https://public-results-storage-prod.magento-testing-service.engineering/reports/magento/magento2/pull/29355/70f9699d843952565c1e8c59d444ebfd/Functional/allure-report-b2b/index.html#categories

Run 2:
https://public-results-storage-prod.magento-testing-service.engineering/reports/magento/magento2/pull/29355/84de6348f4db860c0062029dce8d20b2/Functional/allure-report-b2b/index.html#categories

Functional CE:
Run 1:
https://public-results-storage-prod.magento-testing-service.engineering/reports/magento/magento2/pull/29355/b230b1032ac0fde0f2e1372620f8d865/Functional/allure-report-ce/index.html#categories

Run 2:
https://public-results-storage-prod.magento-testing-service.engineering/reports/magento/magento2/pull/29355/44542d5543464fba358b48be11675467/Functional/allure-report-ce/index.html#categories

Functional EE:
Run 1:
https://public-results-storage-prod.magento-testing-service.engineering/reports/magento/magento2/pull/29355/c74867cac5453927346efee29dde3ec5/Functional/allure-report-ee/index.html

Run 2:
https://public-results-storage-prod.magento-testing-service.engineering/reports/magento/magento2/pull/29355/4ecca2a37665531176a17f2adf83ba56/Functional/allure-report-ee/index.html#categories

Known JIRA:
ACQE-6695

[engcom-Dash]

Moving this PR to extended testing since the copyright tag has not been updated in the mentioned files.

[engcom-Dash]

@magento run Static Tests

[engcom-Dash]

@magento run all tests

[engcom-Dash]

Got a green build so moving it to merge in progress.