Skip to content

improve plugins listing performance - #4151

Merged
thaJeztah merged 1 commit into
docker:masterfrom
crazy-max:improve-plugins-list
Apr 3, 2023
Merged

improve plugins listing performance#4151
thaJeztah merged 1 commit into
docker:masterfrom
crazy-max:improve-plugins-list

Conversation

@crazy-max

Copy link
Copy Markdown
Member

follow-up #4147 (comment)

- What I did

We can slightly improve plugins listing by spawning a goroutine for each iteration.

- How I did it

- How to verify it

$ hyperfine --warmup 2 --runs 5 '/tmp/docker-23.0.2 __complete ""'
Benchmark 1: /tmp/docker-23.0.2 __complete ""
Time (mean ± σ): 87.7 ms ± 2.4 ms [User: 66.2 ms, System: 54.2 ms]
Range (min … max): 83.8 ms … 90.3 ms 5 runs
$ make -f docker.Makefile binary
...
$ hyperfine --warmup 2 --runs 5 './build/docker __complete ""'
Benchmark 1: ./build/docker __complete ""
Time (mean ± σ): 34.6 ms ± 1.2 ms [User: 104.4 ms, System: 74.7 ms]
Range (min … max): 32.8 ms … 36.0 ms 5 runs

Also run this repro.

- Description for the changelog

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

We can slightly improve plugins listing by spawning a
goroutine for each iteration.
Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>
@crazy-max
crazy-maxforce-pushed the improve-plugins-list branch from 13070fa to 89583b9CompareApril 1, 2023 14:02
@codecov-commenter

codecov-commenter commented Apr 1, 2023

Copy link
Copy Markdown

Codecov Report

Merging #4151 (89583b9) into master (33961a7) will increase coverage by 0.00%.
The diff coverage is 71.05%.

Additional details and impacted files
@@ Coverage Diff @@## master #4151 +/- ##
=======================================
Coverage 59.03% 59.03% =======================================
Files 287 287 Lines 24767 24778 +11 =======================================
+ Hits 14620 14627 +7 - Misses 9264 9267 +3 - Partials 883 884 +1 

return p, nil
}
if cmd.HasAlias(p.Name) {
p.Err = NewPluginError("plugin %q duplicates an alias of builtin command %q", p.Name, cmd.Name())

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not related to this PR, but it looks like the only reason NewPluginError() is exported, is for it to be used in a test in docker info;

Err: pluginmanager.NewPluginError("something wrong"),

🤔

@thaJeztahthaJeztah left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@thaJeztah

Copy link
Copy Markdown
Member

@cpuguy83@laurazard ptal 🤗

@thaJeztah
thaJeztah merged commit 966e191 into docker:masterApr 3, 2023
@crazy-max
crazy-max deleted the improve-plugins-list branch April 3, 2023 13:21
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@crazy-max@codecov-commenter@thaJeztah@vvoland