-
Notifications
You must be signed in to change notification settings - Fork 3k
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
{Core} Improve autocomplete performance by preventing loading all modules #24765
Conversation
autocomplete performance improvement |
if not index_modules_extensions and is_autocomplete(): | ||
# If user type `az acco`, command begin with `acco` will be matched. | ||
# If the user just installed a new extension which contains a command named `acco`, he can't get the | ||
# expected suggestion. But it's a rare scenario, and it's okay to ignore. | ||
logger.debug("In autocomplete mode, load command begin with: '%s'", top_command) | ||
index_modules_extensions = [] | ||
for command in index: | ||
if command.startswith(top_command): | ||
index_modules_extensions += index[command] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe put these lines after L597 for simplicity?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I put it here as the following code convert index_modules_extensions
to index_builtin_modules
and index_extensions
️✔️AzureCLI-FullTest
|
if not index_modules_extensions and is_autocomplete(): | ||
# If user type `az acco`, command begin with `acco` will be matched. | ||
# If the user just installed a new extension which contains a command named `acco`, he can't get the | ||
# expected suggestion. But it's a rare scenario, and it's okay to ignore. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the user just installed a new extension which contains a command named
acco
, he can't get the
expected suggestion. But it's a rare scenario, and it's okay to ignore.
This does not happen as CommandIndex().invalidate()
is called after extension installation/remove/update.
# If the command is not complete in autocomplete mode, we should match shorter command. | ||
# For example, `account sho` should match `account`. | ||
logger.debug("Could not find a match in the command or command group table for '%s'", raw_cmd) | ||
raw_cmd = ' '.join(raw_cmd.split()[:-1]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please avoid reusing existing name. Instead, introduce a new name to make the code easier to read, such as raw_cmd_without_last_word
.
Co-authored-by: Jiashuo Li <[email protected]>
Description
Close #24762, related issue #9157, related pr #13294
If the command is not complete, for example:
az account sho
, press TAB, cli loads all modules, which takes 1s. It gets worse if user installs many extensions. This make autocomplete slow.This PR prevents module loading in autocomplete mode.
If command is
az acc
:acc
will be returned inCommandIndex.get()
.raw_cmd
is 'acc'. Trim last word, it becomes''
, returnself.command_table
.If command is
az account sh
:account
module is returned inCommandIndex.get()
.raw_cmd
isaccount sh
. Trim last word, it becomesaccount
and is inself.command_group_table
, returnself.command_table
.Testing Guide
time IFS=$'\013' _ARC_DEBUG=1 COMP_LINE='az account s' COMP_POINT=12 COMP_TYPE="" _ARGCOMPLETE=1 _ARGCOMPLETE_SUPPRESS_SPACE=0 az 8>&1 1>&1 2>&1 9>&1
before: 2.364s
after: 0.269s
This 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.