Skip to content

feat: introduces workerpool #306

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

feat: introduces workerpool #306

wants to merge 11 commits into from

Conversation

cx-rogerio-dalot
Copy link
Contributor

@cx-rogerio-dalot cx-rogerio-dalot commented Jul 7, 2025 •
edited
Loading

Closes #

Proposed Changes

  • It introduces a workerpool to optimize the detection.
    • Initializes with a default number of workers calculated by number of CPUs x2.
      • Reached that conclusion with the bench mark, see my comment below with the stats.
    • This workerpool can be used for other purposes in next steps of the pipeline (in future PRs)
  • On a future PR we won't wait for the detection to end in order to be able to start the next processing steps.

Checklist

  • I covered my changes with tests.
  • I Updated the documentation that is affected by my changes:
    • Change in the CLI arguments
    • Change in the configuration file

I submit this contribution under the Apache-2.0 license.

@cx-rogerio-dalot cx-rogerio-dalot requested a review from a team as a code owner July 7, 2025 16:36
@github-actions GitHub Actions
Copy link

github-actions bot commented Jul 7, 2025 •
edited
Loading

Logo
Checkmarx One – Scan Summary & Details – 18b2f625-b08f-406d-b26c-03f9581e55e1

Great job! No new security vulnerabilities introduced in this pull request

@cx-rogerio-dalot cx-rogerio-dalot force-pushed the AST-99998-architecture-refactor branch from b7f3ada to 3a71a7c Compare July 7, 2025 16:46
@cx-rogerio-dalot
Copy link
Contributor Author

cx-rogerio-dalot commented Jul 8, 2025 •
edited
Loading

Benchmark Analysis (10 runs each) on a CPU with 8 cores

Performance Summary

50 Items - Best Performers (by mean)

  1. 32 workers: 84.4ms (CoV: 19.5%)
  2. 16 workers: 96.3ms (CoV: 9.6%)
  3. 64 workers: 96.8ms (CoV: 25.6%)

100 Items - Best Performers (by mean)

  1. 128 workers: 143.8ms (CoV: 13.7%)
  2. 16 workers: 158.1ms (CoV: 16.4%)
  3. 32 workers: 163.0ms (CoV: 22.6%)

500 Items - Best Performers (by mean)

  1. 32 workers: 575.6ms (CoV: 8.1%)
  2. 128 workers: 603.9ms (CoV: 9.1%)
  3. 64 workers: 620.8ms (CoV: 9.8%)

1000 Items - Best Performers (by mean)

  1. 8 workers: 1040.7ms (CoV: 8.2%)
  2. 128 workers: 1041.6ms (CoV: 3.9%)
  3. 64 workers: 1055.6ms (CoV: 6.0%)

10000 Items - Best Performers (by mean)

  1. 16 workers: 8659.0ms (CoV: 5.6%)
  2. 64 workers: 8995.3ms (CoV: 0.4%)
  3. 8 workers: 9259.6ms (CoV: 6.5%)

Key Insights

Best Worker Counts (Overall Average)

  1. 16 workers: 2147.4ms average
  2. 64 workers: 2186.5ms average
  3. 8 workers: 2241.5ms average

@cx-rogerio-dalot cx-rogerio-dalot force-pushed the AST-99998-update-makefile-and-linters branch from 76c6ed0 to 615d684 Compare July 8, 2025 15:48
@cx-rogerio-dalot cx-rogerio-dalot force-pushed the AST-99998-architecture-refactor branch from 8c0ae9e to 71aedac Compare July 8, 2025 16:13
Base automatically changed from AST-99998-update-makefile-and-linters to master July 10, 2025 10:10
@cx-rogerio-dalot cx-rogerio-dalot force-pushed the AST-99998-architecture-refactor branch from 71aedac to 066f87c Compare July 10, 2025 10:22
@github-actions GitHub Actions
Copy link

kics-logo

KICS version: v1.7.13

Category Results
HIGH HIGH 0
MEDIUM MEDIUM 0
LOW LOW 0
INFO INFO 0
TRACE TRACE 0
TOTAL TOTAL 0
Metric Values
Files scanned placeholder 12
Files parsed placeholder 12
Files failed to scan placeholder 0
Total executed queries placeholder 53
Queries failed to execute placeholder 0
Execution time placeholder 1

cx-leonardo-fontes
cx-leonardo-fontes reviewed Jul 10, 2025
View reviewed changes
pkg/scan.go Outdated
@@ -88,6 +88,10 @@ func (s *scanner) Scan(scanItems []ScanItem, scanConfig ScanConfig) (*reporting.
return &reporting.Report{}, fmt.Errorf("error(s) processing scan items:\n%w", errors.Join(errs...))
}

if err := engineInstance.Shutdown(); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we add this to ScanDynamic function as well?

cx-leonardo-fontes
cx-leonardo-fontes reviewed Jul 10, 2025
View reviewed changes
cmd/workers_test.go Outdated
go ProcessItems(engineTest, "mockPlugin")
pluginName := "mockPlugin"
if rand.Intn(2) == 0 {
// 50% chance of filesystem, even though a scan eithers runs on one or the other, I just want to test both
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't we just do one test case for each? I don't get the non-deterministic part

Copy link
Contributor Author

Choose a reason for hiding this comment

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

because it takes twice as long and I just wanted to figure out the relationship of CPU vs IO. I can do it separate though to stay on the repo long term (I was not thinking on keeping this).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, this is a mistake, I thought I was on the benckmark

cx-leonardo-fontes
cx-leonardo-fontes reviewed Jul 10, 2025
View reviewed changes
cx-leonardo-fontes
cx-leonardo-fontes reviewed Jul 10, 2025
View reviewed changes
cmd/workers.go Outdated
for item := range Channels.Items {
Report.TotalItemsScanned++
item := item
item := item // capture loop variable
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is no longer required since go 1.22: https://tip.golang.org/doc/go1.22, "Previously, the variables declared by a “for” loop were created once and updated by each iteration. In Go 1.22, each iteration of the loop creates new variables, to avoid accidental sharing bugs..."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

old habits die hard

cx-leonardo-fontes
cx-leonardo-fontes reviewed Jul 22, 2025
View reviewed changes
pkg/scan.go
@@ -137,5 +137,9 @@ func (s *scanner) ScanDynamic(itemsIn <-chan ScanItem, scanConfig ScanConfig) (*
}
}

if err := engineInstance.Shutdown(); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you remove this call from the Scan function above? I think you had it earlier

@cx-rogerio-dalot cx-rogerio-dalot force-pushed the AST-99998-architecture-refactor branch from de0fcec to ca72934 Compare July 31, 2025 15:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cx-leonardo-fontes cx-leonardo-fontes cx-leonardo-fontes left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@cx-rogerio-dalot @cx-leonardo-fontes