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

feat: add concurrency option to parallelize package loading #778

Merged
merged 4 commits into from Feb 16, 2022
Merged

feat: add concurrency option to parallelize package loading #778

merged 4 commits into from Feb 16, 2022

Conversation

kruskall
Copy link
Contributor

This PR adds support for running package loading in parallel due to gosec.load being a major bottleneck for big projects.

The concurrency option defaults to the number of available CPUs and tests are using a concurrency value of 1.

Benchmarks:

Benchmarks of a couple of GitLab projects using hyperfine:


Gitlab Pages:

before:

Benchmark 1: /path/to/gosec -exclude-generated ./...
Time (mean ± σ):     11.384 s ±  0.227 s    [User: 28.443 s, System: 8.731 s]
Range (min … max):   10.974 s … 11.793 s    10 runs

Warning: Ignoring non-zero exit code.

after:

Benchmark 1: /path/to/gosec -exclude-generated ./...
Time (mean ± σ):      2.292 s ±  0.078 s    [User: 23.965 s, System: 8.732 s]
Range (min … max):    2.230 s …  2.469 s    10 runs

Warning: Ignoring non-zero exit code.

Gitaly

before:

Benchmark 1: /path/to/gosec -exclude-generated ./...
Time (mean ± σ):     27.442 s ±  0.710 s    [User: 75.289 s, System: 20.270 s]
Range (min … max):   26.601 s … 28.497 s    10 runs

Warning: Ignoring non-zero exit code.

after:

Benchmark 1: /path/to/gosec -exclude-generated ./...
Time (mean ± σ):      5.953 s ±  1.047 s    [User: 67.921 s, System: 20.703 s]
Range (min … max):    5.237 s …  8.658 s    10 runs

Warning: Ignoring non-zero exit code.

@ccojocar
Copy link
Member

@kruskall Thanks for this improvement. I'll have a closer look at it soon. Please could you add some unit tests where the concurrency is more than 1. I'll like to see that the concurrent case is covered by test.

analyzer.go Outdated Show resolved Hide resolved
analyzer.go Outdated Show resolved Hide resolved
analyzer.go Outdated Show resolved Hide resolved
analyzer.go Show resolved Hide resolved
@ccojocar
Copy link
Member

ccojocar commented Feb 16, 2022

Check cannot be made concurrent due to the shared state in the analyser. Sorry for my recommendation to make it concurrent.

analyzer.go Outdated Show resolved Hide resolved
analyzer.go Show resolved Hide resolved
@kruskall
Copy link
Contributor Author

@kruskall Thanks for this improvement. I'll have a closer look at it soon. Please could you add some unit tests where the concurrency is more than 1. I'll like to see that the concurrent case is covered by test.

@ccojocar Thank you for the feedback! I've added a test with a concurrency value of 32, a few more comments and made sure a goroutine would stop if there are no jobs left. Back to you 🏓

Copy link
Member

@ccojocar ccojocar left a comment

Choose a reason for hiding this comment

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

Thanks for fixing the review comments. It looks good now!

@ccojocar ccojocar merged commit 7d539ed into securego:master Feb 16, 2022
@kruskall kruskall deleted the feat/parallel-load branch February 16, 2022 17:24
@hansgylling hansgylling mentioned this pull request Oct 17, 2023
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.

None yet

2 participants