{Core} using threads to build command table for performance #32518
Conversation
️✔️AzureCLI-FullTest
|
️✔️AzureCLI-BreakingChangeTest
|
|
The git hooks are available for azure-cli and azure-cli-extensions repos. They could help you run required checks before creating the PR. Please sync the latest code with latest dev branch (for azure-cli) or main branch (for azure-cli-extensions). pip install azdev --upgrade
azdev setup -c <your azure-cli repo path> -r <your azure-cli-extensions repo path>
|
|
Thank you for your contribution! We will review the pull request and get back to you soon. |
microsoft-github-policy-service
bot
requested review from
jiasli and
yonzhan
microsoft-github-policy-service
bot
added
Auto-Assign
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
DanielMicrosoft
requested review from
bebound,
calvinhzy,
evelyn-ys,
jsntcy,
kairu-ms,
necusjz and
zhoxing-ms
as code owners
There was a problem hiding this comment.
Pull request overview
This PR introduces concurrent module loading using ThreadPoolExecutor to improve Azure CLI command table build performance. The changes parallelize the expensive operation of loading command modules, which is currently a major bottleneck.
Key changes:
- Command modules are now loaded concurrently using threads with a configurable timeout and worker count
- Module loading logic has been refactored into smaller methods for better organization
- A new test ensures command table integrity after the threading changes
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| src/azure-cli-core/azure/cli/core/init.py | Implements ThreadPoolExecutor-based concurrent module loading with timeout protection, refactors module loading into separate methods (_load_modules, _load_single_module, _process_successful_load, etc.), and adds configuration constants for timeout and thread count |
| src/azure-cli-core/azure/cli/core/tests/test_command_table_integrity.py | Adds integration test to verify command table integrity, checking for duplicates, core command groups, structural integrity, and proper command source attribution |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| import traceback | ||
| try: | ||
| start_time = timeit.default_timer() | ||
| module_command_table, module_group_table = _load_module_command_loader(self, args, mod) |
There was a problem hiding this comment.
Is there a risk of race conditions when loading command_loader with multiple threads? How can we ensure that all loaders are properly registered and that each command is correctly mapped to its corresponding loader?
There was a problem hiding this comment.
great catch @MoChilia , I believe I have addressed those concerns by changing the setup to have the threads only return a result, and the parent thread only will do the object mutation.
Interested in your feedback 👍
|
Can you show some example BEFORE/AFTER for commands such as |
|
My understanding is that the 6.517 seconds execution time of |
DanielMicrosoft
force-pushed
the
35856522-improve-command-index-perf
branch
from
de7a573 to
8b414bd
Compare
| cmd_tbl = loader.load_command_table(["extra", "extra", "positional_argument"]) | ||
| self.assertDictEqual(INDEX[CommandIndex._COMMAND_INDEX], self.expected_command_index) | ||
| self.assertEqual(list(cmd_tbl), ['hello mod-only', 'hello overridden', 'extra final', 'hello ext-only']) | ||
| self.assertSetEqual(set(cmd_tbl), {'hello mod-only', 'hello overridden', 'extra final', 'hello ext-only'}) |
There was a problem hiding this comment.
As commands are loaded non-deterministically now, the order of them when converting a dict to a list may change, so changing to a set here, afaik this is not functionally important (as commands are invoked by key, not by order of registation). So changing to a set here is fine.
There are various scenarios that will trigger the rebuild:
We have added a telemetry property to track when a rebuild happens: Around 30% of |
4 participants
Related command
Description
ADO task: https://msazure.visualstudio.com/One/_workitems/edit/35856522
This PR aims to speed up the time it takes to re-build the commandIndex. This is currently a major bottleneck to performance.
The most expensive operations in that code path have had concurrency applied using threads (where possible).
Testing scenarios:
az help,version,upgrade, etc).vm create, etc).commandIndex.json(when it doesnt exist)commandIndex.json(PATCH)az helpcorrectly generates command tree._update_command_table_from_modulesis unchangedTesting Guide
History Notes
[Component Name 1] BREAKING CHANGE:
az command a: Make some customer-facing breaking change[Component Name 2]
az command b: Add some customer-facing featureThis checklist is used to make sure that common guidelines for a pull request are followed.
The PR title and description has followed the guideline in Submitting Pull Requests.
I adhere to the Command Guidelines.
I adhere to the Error Handling Guidelines.