Avoid uselessly trying to migrate plugins #832
Conversation
marckhouzam
force-pushed
the
marck/fastStartupCatalog
branch
from
05e593e to
4317f5d
Compare
| @@ -141,222 +130,6 @@ var _ = Describe("GetInstalledPlugins (standalone plugins)", func() { | |||
| Expect(isInstalled).To(BeFalse()) | |||
| }) | |||
| }) | |||
|
|
|||
| Context("when a standalone and server plugin for k8s target installed", func() { | |||
There was a problem hiding this comment.
Since the MigrateContextPluginsAsStandaloneIfNeeded() function is no longer called by pluginsupplier.GetInstalledPlugins(), these tests were no longer applicable.
I however moved them to a new cleanup_test.go file to directly test MigrateContextPluginsAsStandaloneIfNeeded()
The only time we need to migrate context-scope plugins is after moving from a CLI < 1.3 to a newer one. This commit makes use of the existing global initializer to do this migration once, for all contexts. This avoids an unnecessary slow down for every CLI commands, especially shell completion which should be as fast as possible. Signed-off-by: Marc Khouzam <[email protected]>
marckhouzam
force-pushed
the
marck/fastStartupCatalog
branch
from
4317f5d to
92fa8ed
Compare
|
Thanks for the clear explanation. Looks like a good bit of optimization.
On point 3, I am not sure whether there was some nuance behind the way it was done before this change. |
| func MigrateContextPluginsAsStandaloneIfNeeded() { | ||
| activeContexts, err := configlib.GetAllActiveContextsList() | ||
| if err != nil || len(activeContexts) == 0 { | ||
| return | ||
| } | ||
|
|
||
| c, lockedFile, err := getCatalogCache(true) | ||
| if err != nil { | ||
| return | ||
| } | ||
| defer lockedFile.Close() | ||
|
|
||
| for _, ac := range activeContexts { | ||
| for pluginKey, installPath := range c.ServerPlugins[ac] { | ||
| for _, association := range c.ServerPlugins { | ||
| for pluginKey, installPath := range association { | ||
| c.StandAlonePlugins.Add(pluginKey, installPath) | ||
| } | ||
| delete(c.ServerPlugins, ac) | ||
| } | ||
| // Delete all entries by reassigning to a new empty map | ||
| c.ServerPlugins = make(map[string]PluginAssociation) | ||
|
|
||
| _ = saveCatalogCache(c, lockedFile) |
There was a problem hiding this comment.
What I understand is with this change now we are converting all the ServerPlugins as StandalonePlugins instead of just doing that for the ActiveContexts.
Is this change intentional?
There was a problem hiding this comment.
Correct. See point 3 in the PR description:
with this PR, since we only run the plugin migration once, we must migrate the context-scoped plugins for all contexts immediately. This is a slight change in behaviour because context-scoped plugins for an inactive (and possibly old context) will now appear as standalone plugins right away for the new CLI. Since this new behaviour is similar to point 2 above, I feel it is acceptable
Originally, I started by keeping the original solution of checking this migration at every command, but not blindly updating the catalog if there was nothing to migrate. However this still has a cost compared to running the migration only once, since we don't have to check the catalog for every command, just in case.
But I wanted your opinion if there may be side-effects I missed.
There was a problem hiding this comment.
instead of just doing that for the ActiveContexts
To be very clear, just in case, with the previous approach, when the active context would change, we would migrate the plugins for the new active context. So, assuming every context eventually became active, then all ServerPlugins would eventually be migrated as StandalonePlugins.
With this PR we do it all at once and be done with it.
Do your recall if "only doing it for the active contexts" was a kind of optimization? Maybe because it was run for every command?
There was a problem hiding this comment.
Do your recall if "only doing it for the active contexts" was a kind of optimization? Maybe because it was run for every command?
It was not for optimization but to reliably do the migration as different contexts can recommend different versions of a same plugin. So when we migrate the context-scoped plugins for all available contexts at once we can loose the data as the last migrated plugin version will win. I think we need to be careful and think this through.
Yes. This seems like the correct thing to do. I added a TODO to update the implementation this way because the globalinitializer approach was not available when I implemented this context-scoped plugin to standalone plugin migration. See the existing TODO below.
|
What this PR does / why we need it
The only time we need to migrate context-scope plugins is after moving from a CLI < 1.3 to a newer one. This PR makes use of the existing global initializer to do this migration once, for all contexts.
This avoids an unnecessary slow down for every CLI commands, especially shell completion which should be as fast as possible. We can see an improvement of 30% to 40% in speed for basic shell completion commands.
Note:
Which issue(s) this PR fixes
Fixes N/A
Describe testing done for PR
Test with a CLI 1.2 and a CLI using this PR. We install context-scope plugins and make sure they are migrated as expected.
Now, confirm that CLI is faster.
Before the PR we see a speed normally just above 0.20 seconds.
With the PR we see a time closer to 0.14 which is a 30% improvement
Release note
Additional information
Special notes for your reviewers
On my M1 machine I see these speed results:
I repeatedly run
time tz __complete '' > /dev/nullto test shell completion responsiveness.With 1 context:
v1.2.0 -> ~0.18 secs
v1.3.0 -> ~0.18 secs
v1.4.1 -> ~0.16 secs
v1.5.1 -> ~0.17 secs
main (92c98e8) -> ~0.17 secs
with this PR -> ~0.10 secs
So we see a significant improvement. It makes basic shell completion blazingly fast.