Skip to content
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 plugins discovery performance #4120

Closed
wants to merge 1 commit into from

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

Instead of removing completion for plugins to fix the regression, we can slightly improve plugins discovery if we want to keep this feature.

We should also look if every plugin we currently ship doesn't introduce performance regressions when invoked through the plugin manager in

return exec.Command(c.path, MetadataSubcommandName).Output()

/usr/local/lib/docker/cli-plugins/docker-buildx docker-cli-plugin-metadata

I think a benchmark suite here that could be used by our plugins would be good at some point.

- How I did it

Spawn a goroutine for each iteration in the loop when listing plugins.

- 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-fix-perf-reg 32.1 ± 1.3 30.5 33.8 3.43 ± 0.62

- 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 #4120 (3ffbe4c) into master (f5d698a) will increase coverage by 0.00%.
The diff coverage is 61.90%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #4120   +/-   ##
=======================================
  Coverage   59.16%   59.16%           
=======================================
  Files         287      287           
  Lines       24716    24727   +11     
=======================================
+ Hits        14623    14630    +7     
- Misses       9209     9212    +3     
- Partials      884      885    +1     

Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>
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