improve and load plugin command stubs when required

[crazy-max]

fixes #3621
related to #3429
closes #4119
closes #4120

- What I did

We are currently loading plugin commands stubs for every command invocation to add support for Cobra v2 completion. This causes a significant performance hit if there are a lot of plugins in the user space (7 atm in Docker Desktop):

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)

docker --version takes ~93ms currently:

Command	Mean [ms]	Min [ms]	Max [ms]	Relative
docker-20.10.12	19.2 ± 1.3	18.4	21.4	2.04 ± 0.47
docker-20.10.17	19.5 ± 0.8	18.5	20.5	2.07 ± 0.47
docker-20.10.23	15.3 ± 2.4	12.6	19.0	1.63 ± 0.44
docker-23.0.1	93.9 ± 1.8	91.0	95.8	9.96 ± 2.22
docker-dev-pr-3419-a4b6fe1	17.5 ± 1.0	16.8	19.1	1.85 ± 0.42
docker-dev-pr-3429-a09e61a	93.7 ± 0.8	92.5	94.5	9.94 ± 2.21

Instead of removing completion for plugins to fix the regression as suggested in #4119, we can slightly improve plugins discovery by spawning a goroutine for each iteration in the loop when listing plugins:

docker --version now takes ~38ms

But there is still a significant performance hit compared to 20.10.23. With second commit we are now loading plugins only if cobra completion arg request is found and also make sure plugin command stubs are loaded once.

With this change docker --version now takes ~9ms:

Command	Mean [ms]	Min [ms]	Max [ms]	Relative
docker-20.10.12	19.2 ± 1.3	18.4	21.4	2.04 ± 0.47
docker-20.10.17	19.5 ± 0.8	18.5	20.5	2.07 ± 0.47
docker-20.10.23	15.3 ± 2.4	12.6	19.0	1.63 ± 0.44
docker-23.0.1	93.9 ± 1.8	91.0	95.8	9.96 ± 2.22
docker-dev-pr-3419-a4b6fe1	17.5 ± 1.0	16.8	19.1	1.85 ± 0.42
docker-dev-pr-3429-a09e61a	93.7 ± 0.8	92.5	94.5	9.94 ± 2.21
docker-dev-fix-perf-reg-2	9.4 ± 2.1	7.6	12.6	1.00

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

cli/cli-plugins/manager/candidate.go

Line 22 in f5d698a

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

to load metadata:

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

Full benchmark can be seen at https://github.com/crazy-max/docker-cli-bench/blob/main/bench.md

- How to verify it

$ make -f docker.Makefile binary

Check completion still working for commands and plugins per #3429:

$ ./build/docker __completeNoDesc "b"
build
builder
buildx
:4
Completion ended with directive: ShellCompDirectiveNoFileComp

$ ./build/docker __completeNoDesc "c"
checkpoint
commit
compose
config
container
context
cp
create
:4
Completion ended with directive: ShellCompDirectiveNoFileComp

$ ./build/docker __complete --context "de"
./build/docker __complete --context "de"
desktop-linux
default
:4
Completion ended with directive: ShellCompDirectiveNoFileComp

Check a plugin help output:

$ ./build/docker buildx bake --help

Usage:  docker buildx bake [OPTIONS] [TARGET...]

Build from a file

Aliases:
  docker buildx bake, docker buildx f

Options:
      --builder string         Override the configured builder instance
  -f, --file stringArray       Build definition file
      --load                   Shorthand for "--set=*.output=type=docker"
      --metadata-file string   Write build result metadata to the file
      --no-cache               Do not use cache when building the image
      --print                  Print the options without building
      --progress string        Set type of progress output ("auto", "plain", "tty"). Use plain to show container output (default "auto")
      --provenance string      Shorthand for "--set=*.attest=type=provenance"
      --pull                   Always attempt to pull all referenced images
      --push                   Shorthand for "--set=*.output=type=registry"
      --sbom string            Shorthand for "--set=*.attest=type=sbom"
      --set stringArray        Override target value (e.g., "targetpattern.key=value")

- Description for the changelog

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

[codecov-commenter]

Codecov Report

Merging #4129 (c39c711) into master (f5d698a) will decrease coverage by 0.05%.
The diff coverage is 13.68%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4129      +/-   ##
==========================================
- Coverage   59.16%   59.12%   -0.05%     
==========================================
  Files         287      287              
  Lines       24716    24745      +29     
==========================================
+ Hits        14623    14630       +7     
- Misses       9209     9230      +21     
- Partials      884      885       +1     

[bsousaa]

cc @laurazard

[tonistiigi]

Even if there is a completion arg, why do we need to list all the plugins? Most likely the command is only for a specific plugin. Asking for follow-up, not a blocker.

[crazy-max]

Because we need to list completion values for plugins too so they have to be registered first in cobra root command.

[tonistiigi]

Only when the args is docker. Not when it is already docker buildx. This is not that important as only affects the completion path but ideally it shouldn't be that adding more plugins makes completion for another plugin slower.

[crazy-max]

Yes indeed, we could improve this.

[laurazard]

Tested and changes LGTM

[crazy-max]

Could we have a cherry pick label for this?

[thaJeztah]

THANKS! Let's bring this one in 👍