Skip to content

Conversation

@crazy-max
Copy link
Member

@crazy-max crazy-max commented Mar 25, 2023

fixes #3621
related to #3429

#3429 adds Cobra completion v2 support for commands and plugins. Since this change every invocation takes ~+60ms.

For docker --version:

Command Mean [ms] Min [ms] Max [ms] Relative
docker-19.03.15 61.4 ± 2.6 57.5 64.4 6.56 ± 1.19
docker-20.10.0 18.0 ± 2.9 14.6 21.8 1.93 ± 0.46
docker-20.10.12 17.6 ± 0.9 16.3 18.9 1.88 ± 0.35
docker-20.10.17 16.5 ± 1.5 15.4 19.1 1.77 ± 0.35
docker-20.10.23 13.9 ± 1.0 12.7 15.3 1.49 ± 0.28
docker-23.0.1 68.2 ± 2.2 64.7 70.3 7.29 ± 1.31
docker-dev-pr-3419-a4b6fe1 15.7 ± 0.7 14.9 16.5 1.68 ± 0.30
docker-dev-pr-3429-a09e61a 69.3 ± 2.5 66.7 73.1 7.41 ± 1.33

See the diff between docker-dev-pr-3419-a4b6fe1 and docker-dev-pr-3429-a09e61a.

Looking at the changes, we are now loading plugins for every invocation:

cli/cmd/docker/docker.go

Lines 230 to 233 in f5d698a

err = pluginmanager.AddPluginCommandStubs(dockerCli, cmd)
if err != nil {
return err
}

So the more plugins are in place in the user space, the worst it would be. And we have a lot of them in Docker Desktop atm:

Client:
 Context:    default
 Debug Mode: false
 Plugins:
  buildx: Docker Buildx (Docker Inc., v0.10.3)
  compose: Docker Compose (Docker Inc., v2.15.1)
  dev: Docker Dev Environments (Docker Inc., v0.1.0)
  extension: Manages Docker extensions (Docker Inc., v0.2.18)
  sbom: View the packaged-based Software Bill Of Materials (SBOM) for an image (Anchore Inc., 0.6.0)
  scan: Docker Scan (Docker Inc., v0.25.0)
  scout: Command line tool for Docker Scout (Docker Inc., v0.6.0)

- What I did

Remove completion for plugins to fix the regression.

Also wants to note that Cobra v2 completion is not implemented in our completion scripts atm. #3429 is just a preliminary work for Cobra v2 completion support afaik. If we want full support we need to revisit the completion scripts and remove hand-written funcs. We could rely on a single function and let Cobra do the completion. Pretty much like #4094 for plugins. All this to say that with this change it does not break the current workflow.

- How I did it

- How to verify it

Here is the benchmark result (see last row):

Command Mean [ms] Min [ms] Max [ms] Relative
docker-19.03.15 61.4 ± 2.6 57.5 64.4 6.56 ± 1.19
docker-20.10.0 18.0 ± 2.9 14.6 21.8 1.93 ± 0.46
docker-20.10.12 17.6 ± 0.9 16.3 18.9 1.88 ± 0.35
docker-20.10.17 16.5 ± 1.5 15.4 19.1 1.77 ± 0.35
docker-20.10.23 13.9 ± 1.0 12.7 15.3 1.49 ± 0.28
docker-23.0.1 68.2 ± 2.2 64.7 70.3 7.29 ± 1.31
docker-dev-pr-3419-a4b6fe1 15.7 ± 0.7 14.9 16.5 1.68 ± 0.30
docker-dev-pr-3429-a09e61a 69.3 ± 2.5 66.7 73.1 7.41 ± 1.33
docker-dev-rm-plugins-completion 10.1 ± 0.5 9.4 10.8 1.08 ± 0.20

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

@codecov-commenter
Copy link

codecov-commenter commented Mar 25, 2023

Codecov Report

Merging #4119 (e2b5dde) into master (f5d698a) will decrease coverage by 0.01%.
The diff coverage is 0.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4119      +/-   ##
==========================================
- Coverage   59.16%   59.16%   -0.01%     
==========================================
  Files         287      287              
  Lines       24716    24717       +1     
==========================================
  Hits        14623    14623              
- Misses       9209     9210       +1     
  Partials      884      884              

@crazy-max crazy-max force-pushed the rm-plugins-completion branch 2 times, most recently from dd3483a to 6d4d341 Compare March 25, 2023 12:15
Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>
@crazy-max crazy-max force-pushed the rm-plugins-completion branch from 6d4d341 to e2b5dde Compare March 25, 2023 12:25
@crazy-max crazy-max deleted the rm-plugins-completion branch March 30, 2023 15:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Performance regression on every command invocation

2 participants