-
Notifications
You must be signed in to change notification settings - Fork 22
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
Improve startup speed #821
Conversation
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.
Overall optimization is quite clean and much appreciated, thanks for looking into this.
I have a very tiny concern if the skipping of the SetContext() has any unintended consequences, and if we want to cover our bases.
40995ae
to
989159a
Compare
Before this commit, the CLI would always perform a two-way sync of the Contexts of the config-ng.yaml file and the Servers of the config.yaml file, even if there was no reason to sync, i.e., if the Contexts and Servers were already in sync. This sync was being done for every single command of the CLI.. This caused a write to the config files for each contexts configured and one more for the current context. With many contexts configured, this unnecessary sync would make the CLI startup noticeably slower. This commit verifies if the sync is required by computing and storing a SHA for the "context" section and one for the "server" section, and checking if the SHAs have changed on each CLI command and if so, only then doing the sync. Note that Tanzu contexts are kept out of the SHA computation since they are not synced. Signed-off-by: Marc Khouzam <[email protected]>
The logic to store the last execute CLI would always write to the datastore and to the config files, which should be avoided. This commit first checks if the update is required or not. Signed-off-by: Marc Khouzam <[email protected]>
989159a
to
9eab763
Compare
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.
LGTM. Very nice way to test the change as well. I just have one question.
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.
Very nice approach of comparing the server and context contents!
lgtm!
What this PR does / why we need it
This PR avoids writing to the config files and the
.data-store.yaml
on startup if not necessary.Writing to the config file to update the server list has a cost for each context. I ended up with 27 contexts configured, which triggered writing to file over 50 times which added 0.65 seconds on my M1 Mac, making the startup jump from around 0.35 seconds to a full second. Having the CLI take a full second to execute any command implies that each call to shell completion takes at least 1 second, which is slow for the user.
Updating the server list does not actually need to be done all the time.
Furthermore, the CLI currently writes to the
.data-store.yaml
and to the config files on every command when it stores the LastCLIVersionUsed. This file writes are also unnecessary most of the time.This PR has two commits:
features.global.context-target-v2
feature if it is currently enabled (to avoid writing to the config files)This reduces the execution time for commands that don't access the backend; this means that shell completion for commands and flags becomes noticeably more responsive when a user has multiple contexts configured.
Before this PR, running
tanzu version
with 9 contexts configures takes around 0.5 seconds.With this PR is becomes closer to 0.3 seconds.
Which issue(s) this PR fixes
Fixes #823
Describe testing done for PR
I use this little utility called
entr
which will execute a command whenever a specific file is changed.Before the PR, if I run any command such as
tanzu -h >/dev/null
I see the config files change for every command multiple times (one for each context).And for commands that trigger the lastTanzuVersion check (most commands), such as
tanzu plugin list >/dev/null
, I see the datastore file updated and the config files also.With this PR, running those same commands, I don't see any changes done to the config or datastore files.
I also ran some normal shell completion down the command tree, and the responsiveness is noticeably better.
Release note
Additional information
Special notes for your reviewer