Use concurrency for path scanning #405

egibs · 2024-08-15T21:14:10Z

As opposed to #402 which implemented concurrency at the scanPath level, this PR implements configurable concurrency at the path level, that is, all of the recursively discovered paths within a given scanPath.

By default, the concurrency is limited to runtime.NumCPU() and will automatically be set to 1 if the configured concurrency is less than 1. Similarly, the concurrency can not be set higher than the number of paths being scanned.

I converted map[any]any usages over to go-ordered-map to ensure that all of our dictionaries maintain their insertion order when iterated over.

Anecdotally, this change has provided a decent increase in speed depending on the concurrency value, number of files, and the size of each file.

Benchmarking with the default concurrency of runtime.NumCPU() takes ~50 seconds:

goos: darwin
goarch: arm64
pkg: github.com/chainguard-dev/bincapz/samples
cpu: Apple M3 Pro
BenchmarkRun-12    	       1	48120070292 ns/op	773853464 B/op	 4895873 allocs/op
PASS
ok  	github.com/chainguard-dev/bincapz/samples	49.446s

as opposed to ~80 seconds without concurrency (-j 1):

goos: darwin
goarch: arm64
pkg: github.com/chainguard-dev/bincapz/samples
cpu: Apple M3 Pro
BenchmarkRun-12    	       1	78739845000 ns/op	774942256 B/op	 4896912 allocs/op
PASS
ok  	github.com/chainguard-dev/bincapz/samples	80.366s

Anecdotally, scanning /usr/bin on my Framework Laptop takes ~60 seconds with concurrency and ~220 seconds without (-j 1).

egibs · 2024-08-15T22:01:07Z

pkg/action/programkind.go

@@ -80,7 +80,7 @@ func programKind(ctx context.Context, path string) string {
 	headerString := ""
 	n, err := io.ReadFull(f, header[:])
 	if err == nil || errors.Is(err, io.ErrUnexpectedEOF) {
-		kind, err := magic.Lookup(header[:n])
+		kind, err := magic.LookupSync(header[:n])


I doubt we were seeing much of a benefit from this.

LookupSync lookups up the file type based on the provided magic bytes without spawning any additional goroutines. You should provide at least the first 1024 bytes of the file in this slice. A magic.ErrUnknown will be returned if the file type is not known.

Signed-off-by: egibs <[email protected]>

…overed paths Signed-off-by: egibs <[email protected]>

Signed-off-by: egibs <[email protected]>

pkg/action/diff.go

egibs · 2024-08-15T22:08:27Z

pkg/action/diff.go

@@ -127,7 +128,7 @@ func processDest(ctx context.Context, c bincapz.Config, from, to map[string]*bin
 	for relPath, tr := range to {
 		fr, exists := from[relPath]
 		if !exists {
-			d.Added[relPath] = tr
+			d.Added.Set(relPath, tr)


This is the pattern for adding a KV pair to an orderedmap dictionary.

egibs · 2024-08-15T22:08:52Z

pkg/action/diff.go

-		delete(d.Removed, rpath)
-		delete(d.Added, apath)
+		d.Modified.Set(apath, abs)
+		d.Modified.Delete(rpath)


This is the pattern for deleting a key from an orderedmap.

egibs · 2024-08-15T22:09:07Z

pkg/action/scan.go

@@ -152,7 +159,7 @@ func recursiveScan(ctx context.Context, c bincapz.Config) (*bincapz.Report, erro
 	logger := clog.FromContext(ctx)
 	logger.Debug("recursive scan", slog.Any("config", c))
 	r := &bincapz.Report{
-		Files: map[string]*bincapz.FileReport{},
+		Files: orderedmap.New[string, *bincapz.FileReport](),


This is the pattern for initializing a new orderedmap.

egibs · 2024-08-15T22:09:42Z

pkg/action/util.go

@@ -0,0 +1,24 @@
+// Copyright 2024 Chainguard, Inc.


These are helper functions for inserting sorted keys into a map.

egibs · 2024-08-15T22:10:33Z

pkg/action/diff.go

-		d.Modified[relPath].Behaviors = append(d.Modified[relPath].Behaviors, abs.Behaviors...)
+		rel, _ := d.Modified.Get(relPath)
+		rel.Behaviors = append(rel.Behaviors, abs.Behaviors...)
+		d.Modified.Set(relPath, rel)


This was the most difficult portion of migrating to orderedmap. I don't think a one-liner was doable but this is more readable.

Signed-off-by: egibs <[email protected]>

pkg/action/scan.go

Signed-off-by: egibs <[email protected]>

…tion loop Signed-off-by: egibs <[email protected]>

Signed-off-by: egibs <[email protected]>

pkg/action/scan.go

tstromberg

Solid improvement, I just have some readability comments regarding data structures.

pkg/action/scan.go

tstromberg · 2024-08-16T13:58:30Z

pkg/action/scan.go

+				fr, err := processFile(ctx, c, yrs, path, scanPath, trimPath, logger)
+				if err != nil {
+					mu.Lock()
+					result = append(result, findings{path: path, fr: &bincapz.FileReport{}})


I think it would improve readability if a sync.Map was used to store the findings, so you wouldn't have to worry about manually managing the mutex. The slice ends up being converted to a map anyways later via scanPathFindings.

I tested this out and got some weird results when running tests where the file reports were merged for two different tests. Managing the mutex(es) isn't ideal but the outcome is deterministic. I'll see if I was doing something wrong with the sync.Map, though.

Yep, I was doing something wrong. I'll push the working code in a bit.

Updated in 81fca83 (#405).

Follow-up removals/fixes in c03d232 (#405) and c4888b3 (#405).

Signed-off-by: egibs <[email protected]>

Signed-off-by: Evan Gibler <[email protected]>

Signed-off-by: egibs <[email protected]>

tstromberg · 2024-08-16T18:24:09Z

Love it. Thank you!

* Use file descriptors Signed-off-by: egibs <[email protected]> * Relocate file descriptor code; implement working concurrency for discovered paths Signed-off-by: egibs <[email protected]> * Move to ordered map; relocate File rendering Signed-off-by: egibs <[email protected]> * Limit concurrency to runtime.NumCPU() Signed-off-by: egibs <[email protected]> * Rename map variables Signed-off-by: egibs <[email protected]> * Directly initialize diff maps Signed-off-by: egibs <[email protected]> * Appease the linter Signed-off-by: egibs <[email protected]> * Configurable concurrency Signed-off-by: egibs <[email protected]> * Update samples_test.go Signed-off-by: egibs <[email protected]> * More consolidation Signed-off-by: egibs <[email protected]> * Add exists check Signed-off-by: egibs <[email protected]> * Use -j instead of -c for concurrency Signed-off-by: egibs <[email protected]> * Use an errgroup instead of a waitgroup + semaphore Signed-off-by: egibs <[email protected]> * Use SetLimit; add concurrency config to all tests; linting fixes Signed-off-by: egibs <[email protected]> * Move sorting code into scan.go; move rendering inside of map construction loop Signed-off-by: egibs <[email protected]> * Use 8-core runner for tests Signed-off-by: egibs <[email protected]> * Go back to ubuntu-latest Signed-off-by: egibs <[email protected]> * Run core tests in parallel Signed-off-by: egibs <[email protected]> * Better alias naming, consolidate structs Signed-off-by: egibs <[email protected]> * Run make fix Signed-off-by: egibs <[email protected]> * Replace mutexes with sync.Map Signed-off-by: egibs <[email protected]> * Remove struct/type Signed-off-by: egibs <[email protected]> * Appease the linter Signed-off-by: egibs <[email protected]> --------- Signed-off-by: egibs <[email protected]> Signed-off-by: Evan Gibler <[email protected]>

egibs commented Aug 15, 2024

View reviewed changes

egibs added 10 commits August 15, 2024 17:05

Use file descriptors

d8fb97b

Signed-off-by: egibs <[email protected]>

Relocate file descriptor code; implement working concurrency for disc…

567f530

…overed paths Signed-off-by: egibs <[email protected]>

Move to ordered map; relocate File rendering

83bdce2

Signed-off-by: egibs <[email protected]>

Limit concurrency to runtime.NumCPU()

ad58188

Signed-off-by: egibs <[email protected]>

Rename map variables

1d9e1a1

Signed-off-by: egibs <[email protected]>

Directly initialize diff maps

7d6a70c

Signed-off-by: egibs <[email protected]>

Appease the linter

e62d12f

Signed-off-by: egibs <[email protected]>

Configurable concurrency

57396b4

Signed-off-by: egibs <[email protected]>

Update samples_test.go

6ebbf4c

Signed-off-by: egibs <[email protected]>

More consolidation

42d2cda

Signed-off-by: egibs <[email protected]>

egibs force-pushed the performance-improvements branch from d7e0729 to 42d2cda Compare August 15, 2024 22:06

egibs commented Aug 15, 2024

View reviewed changes

pkg/action/diff.go Show resolved Hide resolved

egibs commented Aug 15, 2024

View reviewed changes

Add exists check

9182f88

Signed-off-by: egibs <[email protected]>

egibs commented Aug 15, 2024

View reviewed changes

pkg/action/scan.go Outdated Show resolved Hide resolved

egibs added 3 commits August 15, 2024 17:47

Use -j instead of -c for concurrency

d18239d

Signed-off-by: egibs <[email protected]>

Use an errgroup instead of a waitgroup + semaphore

26a57dc

Signed-off-by: egibs <[email protected]>

Use SetLimit; add concurrency config to all tests; linting fixes

bf0fb13

Signed-off-by: egibs <[email protected]>

egibs force-pushed the performance-improvements branch from 3b10a15 to aa6bed2 Compare August 16, 2024 12:47

Move sorting code into scan.go; move rendering inside of map construc…

8b05ed4

…tion loop Signed-off-by: egibs <[email protected]>

egibs force-pushed the performance-improvements branch from aa6bed2 to 8b05ed4 Compare August 16, 2024 12:48

egibs added 3 commits August 16, 2024 08:02

Use 8-core runner for tests

28d23e4

Signed-off-by: egibs <[email protected]>

Go back to ubuntu-latest

580052b

Signed-off-by: egibs <[email protected]>

Run core tests in parallel

8b97499

Signed-off-by: egibs <[email protected]>

egibs requested review from tstromberg and hectorj2f August 16, 2024 13:21

egibs marked this pull request as ready for review August 16, 2024 13:21

tstromberg reviewed Aug 16, 2024

View reviewed changes

pkg/action/scan.go Outdated Show resolved Hide resolved

tstromberg reviewed Aug 16, 2024

View reviewed changes

pkg/action/scan.go Outdated Show resolved Hide resolved

tstromberg requested changes Aug 16, 2024

View reviewed changes

egibs added 6 commits August 16, 2024 11:26

Better alias naming, consolidate structs

c42f418

Signed-off-by: egibs <[email protected]>

Merge branch 'main' into performance-improvements

a84cc1a

Signed-off-by: Evan Gibler <[email protected]>

Run make fix

c7bdc4f

Signed-off-by: egibs <[email protected]>

Replace mutexes with sync.Map

81fca83

Signed-off-by: egibs <[email protected]>

Remove struct/type

c03d232

Signed-off-by: egibs <[email protected]>

Appease the linter

c4888b3

Signed-off-by: egibs <[email protected]>

egibs requested a review from tstromberg August 16, 2024 17:35

egibs changed the title ~~Use concurrency for path scanning; improve scan performance via ScanFileDescriptor~~ Use concurrency for path scanning Aug 16, 2024

tstromberg approved these changes Aug 16, 2024

View reviewed changes

egibs merged commit 24c0a3b into chainguard-dev:main Aug 16, 2024
6 checks passed

egibs deleted the performance-improvements branch August 16, 2024 18:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use concurrency for path scanning #405

Use concurrency for path scanning #405

egibs commented Aug 15, 2024 •

edited

Loading

egibs Aug 15, 2024

egibs Aug 15, 2024

egibs Aug 15, 2024

egibs Aug 15, 2024

egibs Aug 15, 2024

egibs Aug 15, 2024

tstromberg left a comment

tstromberg Aug 16, 2024

egibs Aug 16, 2024

egibs Aug 16, 2024

egibs Aug 16, 2024

egibs Aug 16, 2024

tstromberg commented Aug 16, 2024

Use concurrency for path scanning #405

Use concurrency for path scanning #405

Conversation

egibs commented Aug 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tstromberg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tstromberg commented Aug 16, 2024

egibs commented Aug 15, 2024 •

edited

Loading