search

Gelio / go-global-update

A command to update globally installed go executables
MIT License
134 stars 4 forks source link

feat: bounded concurrency for binary inspection #19

Closed mroth closed 10 months ago

mroth commented 11 months ago

Uses a semaphore to bound concurrency to GOMAXPROCS value. Fixes #14.

On my system, a cached --dry-run goes from ~820ms to ~130ms, and an uncached one that needs to hit the network goes from ~2s to ~300ms.

Benchmark results from my laptop:

$ hyperfine --warmup 1 'go-global-update --dry-run' './go-global-update-fork --dry-run'
Benchmark 1: go-global-update --dry-run
  Time (mean ± σ):     821.7 ms ±   7.7 ms    [User: 171.8 ms, System: 139.0 ms]
  Range (min … max):   806.2 ms … 832.8 ms    10 runs

Benchmark 2: ./go-global-update-fork --dry-run
  Time (mean ± σ):     129.5 ms ±   4.2 ms    [User: 152.8 ms, System: 187.8 ms]
  Range (min … max):   120.3 ms … 137.3 ms    22 runs

Results with no modcache (cleared via go clean -modcache):

hyperfine --prepare 'go clean -modcache' 'go-global-update --dry-run' './go-global-update-fork --dry-run'
Benchmark 1: go-global-update --dry-run
  Time (mean ± σ):      1.936 s ±  0.072 s    [User: 0.236 s, System: 0.243 s]
  Range (min … max):    1.875 s …  2.120 s    10 runs

Benchmark 2: ./go-global-update-fork --dry-run
  Time (mean ± σ):     303.4 ms ±  22.4 ms    [User: 193.1 ms, System: 261.4 ms]
  Range (min … max):   284.7 ms … 361.6 ms    10 runs
Gelio commented 10 months ago

Hey, thanks for the contribution! Sorry for lack of activity on this PR. I am on an extended vacation.

The code looks good at first sight. I will do a more thorough review of it next week.

I see the CI failed for go 1.17. I presume it is a problem with shfmt being incompatible with go 1.17, and not anything related to this change. You do not need to worry about it

I appreciate the benchmarks in the PR description :tada:

Gelio commented 10 months ago

I have published this improvement in v0.2.3