Perf(reporthandling): uniqueStrings & trimunique (#81)

* introduced internal package for optimized versions of slice unique & trim * profiled & benchmarked internal/slices package * moved helpers call for listing to the internal package * adapted reporthandling to use internal/slices * removed erroneous comment about make map, corrected benchmark * improved slice alloc on trimmed resources * fixup listing failed to unique Signed-off-by: Frederic BIDON <[email protected]> * fix: renamed package for newly added test Signed-off-by: Frederic BIDON <[email protected]> --------- Signed-off-by: Frederic BIDON <[email protected]>
kubescape · Feb 7, 2023 · a32f9ca · a32f9ca
1 parent ff6ed2b
commit a32f9ca
Show file tree

Hide file tree

Showing 16 changed files with 791 additions and 157 deletions.
diff --git a/reporthandling/datastructuresmethodshelper.go b/reporthandling/datastructuresmethodshelper.go
@@ -1,5 +1,9 @@
 package reporthandling
 
+import (
+	"github.com/kubescape/opa-utils/reporthandling/internal/slices"
+)
+
 type ResourcesIDs struct {
 	passedResources  []string
 	failedResources  []string
@@ -31,30 +35,25 @@ func (r *ResourcesIDs) setFailedResources(a []string) {
 
 // setWarningResources - initialized after failed resources are set
 func (r *ResourcesIDs) setWarningResources(a []string) {
-	r.warningResources = TrimUniqueIDs(GetUniqueResourcesIDs(a), r.failedResources)
+	r.warningResources = slices.TrimStable(GetUniqueResourcesIDs(a), r.failedResources)
 }
 
 // setPassedResources - initialized after warning resources are set
 func (r *ResourcesIDs) setPassedResources(a []string) {
-	r.passedResources = TrimUniqueIDs(GetUniqueResourcesIDs(a), append(r.failedResources, r.warningResources...))
+	r.passedResources = slices.TrimStable(GetUniqueResourcesIDs(a), append(r.failedResources, r.warningResources...))
 }
 
 func deleteFromMap(m map[string]interface{}, keepFields []string) {
 	for k := range m {
-		if StringInSlice(keepFields, k) {
+		if slices.StringInSlice(keepFields, k) {
 			continue
 		}
 		delete(m, k)
 	}
 }
 
 func StringInSlice(strSlice []string, str string) bool {
-	for i := range strSlice {
-		if strSlice[i] == str {
-			return true
-		}
-	}
-	return false
+	return slices.StringInSlice(strSlice, str)
 }
 
 func RemoveResponse(slice []RuleResponse, index int) []RuleResponse {

diff --git a/reporthandling/helpers/v1/cloudmetadata.go b/reporthandling/helpers/v1/cloudmetadata.go
@@ -1,4 +1,4 @@
-package v1
+package helpers
 
 import (
 	"fmt"

diff --git a/reporthandling/helpers/v1/cloudmetadata_test.go b/reporthandling/helpers/v1/cloudmetadata_test.go
@@ -1,4 +1,4 @@
-package v1
+package helpers
 
 import (
 	"reflect"

diff --git a/reporthandling/helpers/v1/filters.go b/reporthandling/helpers/v1/filters.go
@@ -1,4 +1,4 @@
-package v1
+package helpers
 
 import (
 	"github.com/armosec/armoapi-go/armotypes"

diff --git a/reporthandling/helpers/v1/filters_test.go b/reporthandling/helpers/v1/filters_test.go
@@ -1,4 +1,4 @@
-package v1
+package helpers
 
 import (
 	"testing"

diff --git a/reporthandling/helpers/v1/listing.go b/reporthandling/helpers/v1/listing.go
@@ -1,8 +1,8 @@
-package v1
+package helpers
 
 import (
-	"github.com/armosec/utils-go/str"
 	"github.com/kubescape/opa-utils/reporthandling/apis"
+	"github.com/kubescape/opa-utils/reporthandling/internal/slices"
 )
 
 // ReportObject any report object must be compliment with a map[string]interface{} structures
@@ -104,10 +104,10 @@ func (all *AllLists) Update(all2 *AllLists) {
 // ToUnique - Call this function only when setting the List
 func (all *AllLists) toUniqueBase() {
 	// remove duplications from each resource list
-	all.failed = str.SliceStringToUnique(all.failed)
-	all.passed = str.SliceStringToUnique(all.passed)
-	all.skipped = str.SliceStringToUnique(all.skipped)
-	all.other = str.SliceStringToUnique(all.other)
+	all.failed = slices.UniqueStrings(all.failed)
+	all.passed = slices.UniqueStrings(all.passed)
+	all.skipped = slices.UniqueStrings(all.skipped)
+	all.other = slices.UniqueStrings(all.other)
 }
 
 // ToUnique - Call this function only when setting the List
@@ -117,44 +117,28 @@ func (all *AllLists) ToUniqueControls() {
 
 // ToUnique - Call this function only when setting the List
 func (all *AllLists) ToUniqueResources() {
-	all.toUniqueBase()
-	// remove failed from passed list
-	all.passed = trimUnique(all.passed, all.failed)
-	// remove failed, and passed from skipped list
-	trimmed := append(all.failed, all.passed...)
-	all.skipped = trimUnique(all.skipped, trimmed)
-	// remove failed, passed and skipped from other list
-	trimmed = append(trimmed, all.skipped...)
-	all.other = trimUnique(all.other, trimmed)
-}
+	all.failed = slices.UniqueStrings(all.failed)
 
-// trimUnique trim the list, return original list without the "trimFrom" list. the list is trimmed in place, so the original list is modified. Also, the list is not sorted
-func trimUnique(origin, trimFrom []string) []string {
-	if len(origin) == 0 || len(trimFrom) == 0 { // if there is nothing to trim
-		return origin
-	}
-	toRemove := make(map[string]bool, len(trimFrom))
+	const heuristicCapacity = 100 // alloc 100 slots to the stack. The rest would go to the heap - see https://github.com/golang/go/issues/58215
+	trimmed := append(make([]string, 0, heuristicCapacity), make([]string, 0, max(len(all.failed)+len(all.excluded)+len(all.passed)+len(all.skipped), heuristicCapacity)-heuristicCapacity)...)
 
-	for i := range trimFrom {
-		toRemove[trimFrom[i]] = true
-	}
+	// remove failed and excluded from passed list
+	trimmed = append(trimmed, all.failed...)
+	all.passed = slices.TrimStableUnique(all.passed, trimmed)
 
-	originLen := len(origin)
-	for i := 0; i < originLen; {
-		if _, ok := toRemove[origin[i]]; ok {
-			str.RemoveIndexFromStringSlice(&origin, i)
-			originLen--
-		} else {
-			i++
-		}
-	}
-	return origin
+	// remove failed, excluded and passed from skipped list
+	trimmed = append(trimmed, all.passed...)
+	all.skipped = slices.TrimStableUnique(all.skipped, trimmed)
+
+	// remove failed, excluded, passed and skipped from other list
+	trimmed = append(trimmed, all.skipped...)
+	all.other = slices.TrimStableUnique(all.other, trimmed)
 }
 
-// appendSlice append a slice to a slice the index indicates the position of the slice
-func appendSlice(origin, appendTo []string, index *int) {
-	for i := range appendTo {
-		origin[*index] = appendTo[i]
-		*index++
+func max(a, b int) int {
+	if a > b {
+		return a
 	}
+
+	return b
 }
diff --git a/reporthandling/helpers/v1/listing_benchmark_test.go b/reporthandling/helpers/v1/listing_benchmark_test.go
@@ -0,0 +1,20 @@
+package helpers
+
+import (
+	"testing"
+
+	"github.com/kubescape/opa-utils/reporthandling/apis"
+)
+
+func BenchmarkToUniqueResources(b *testing.B) {
+	listA := mockAllListsA()
+	listA.Append(apis.StatusExcluded, "b")
+	listA.Append(apis.StatusExcluded, "b")
+	listA.Append(apis.StatusExcluded, "b")
+	b.ResetTimer()
+	b.ReportAllocs()
+
+	for n := 0; n < b.N; n++ {
+		listA.ToUniqueResources()
+	}
+}
diff --git a/reporthandling/helpers/v1/listing_mocks.go b/reporthandling/helpers/v1/listing_mocks.go
@@ -1,4 +1,4 @@
-package v1
+package helpers
 
 func MockAllListsForIntegration() *AllLists {
 	return &AllLists{

diff --git a/reporthandling/helpers/v1/listing_test.go b/reporthandling/helpers/v1/listing_test.go
@@ -1,4 +1,4 @@
-package v1
+package helpers
 
 import (
 	"testing"
@@ -122,6 +122,7 @@ func TestAllListsUniqueControls(t *testing.T) {
 	assert.Equal(t, 1, len(listMock.Skipped()))
 }
 
+/* appendSlice and intToPointer are unused for now
 func TestAppendSlice(t *testing.T) {
 	type args struct {
 		origin   []string
@@ -160,81 +161,16 @@ func TestAppendSlice(t *testing.T) {
 		})
 	}
 }
+
 func intToPointer(i int) *int {
 	return &i
 }
 
-func TestTrimUnique(t *testing.T) {
-	type args struct {
-		origin   []string
-		trimFrom []string
-		expected []string
-	}
-	tests := []struct {
-		name string
-		args args
-	}{
-		{
-			name: "trim from begging of slice",
-			args: args{
-				origin:   []string{"a", "b", "c"},
-				trimFrom: []string{"a"},
-				expected: []string{"c", "b"},
-			},
-		},
-		{
-			name: "trim from middle of slice",
-			args: args{
-				origin:   []string{"a", "b", "c"},
-				trimFrom: []string{"b"},
-				expected: []string{"a", "c"},
-			},
-		},
-		{
-			name: "trim from end of slice",
-			args: args{
-				origin:   []string{"a", "b", "c"},
-				trimFrom: []string{"c"},
-				expected: []string{"a", "b"},
-			},
-		},
-		{
-			name: "do nothing",
-			args: args{
-				origin:   []string{"a", "b", "c"},
-				trimFrom: []string{"d"},
-				expected: []string{"a", "b", "c"},
-			},
-		},
-		{
-			name: "trim all",
-			args: args{
-				origin:   []string{"a", "b", "c"},
-				trimFrom: []string{"a", "b", "c"},
-				expected: []string{},
-			},
-		},
-		{
-			name: "trimFrom larger",
-			args: args{
-				origin:   []string{"a", "b", "c"},
-				trimFrom: []string{"a", "b", "e", "d"},
-				expected: []string{"c"},
-			},
-		},
-		{
-			name: "trim all not sorted",
-			args: args{
-				origin:   []string{"c", "a", "b"},
-				trimFrom: []string{"a", "b", "c"},
-				expected: []string{},
-			},
-		},
-	}
-	for _, tt := range tests {
-		t.Run(tt.name, func(t *testing.T) {
-			dd := trimUnique(tt.args.origin, tt.args.trimFrom)
-			assert.Equal(t, tt.args.expected, dd)
-		})
+// appendSlice append a slice to a slice the index indicates the position of the slice
+func appendSlice(origin, appendTo []string, index *int) {
+	for i := range appendTo {
+		origin[*index] = appendTo[i]
+		*index++
 	}
 }
+*/
diff --git a/reporthandling/helpers/v1/status.go b/reporthandling/helpers/v1/status.go
@@ -1,4 +1,4 @@
-package v1
+package helpers
 
 import "github.com/kubescape/opa-utils/reporthandling/apis"
 

diff --git a/reporthandling/helpers/v1/status_test.go b/reporthandling/helpers/v1/status_test.go
@@ -1,4 +1,4 @@
-package v1
+package helpers
 
 import (
 	"testing"

diff --git a/reporthandling/internal/slices/README.md b/reporthandling/internal/slices/README.md
@@ -0,0 +1,69 @@
+# slices
+
+Internal utility functions that manipulate slices of strings.
+
+All these methods alter their input slice and do not allocate extra memory.
+
+`UniqueStrings` is provided as a faster equivalent to `github.com/armosec/utils-go/str.SliceStringToUnique`.
+
+`Trim` is the same as the previously private function `trimUnique()` with similar performances. Code is just more straightforward.
+
+`TrimStable` has the same intent as `Trim` but does not alter the order of the input.
+
+`TrimUnique` and `TrimStableUnique` combine `UniqueStrings` and `Trim` (resp. `TrimStable`) in a single iteration of the input slice.
+
+## Benchmarks
+
+```
+go test -v -run Bench -bench . -benchtime 1s
+goos: linux
+goarch: amd64
+pkg: github.com/kubescape/opa-utils/reporthandling/internal/slices
+cpu: AMD Ryzen 7 5800X 8-Core Processor             
+BenchmarkUnique
+BenchmarkUnique/UniqueStrings_x_8
+BenchmarkUnique/UniqueStrings_x_8-16         	 6007258	       192.3 ns/op	       0 B/op	       0 allocs/op
+BenchmarkUnique/UniqueStrings_x_16
+BenchmarkUnique/UniqueStrings_x_16-16        	 3084657	       386.0 ns/op	       0 B/op	       0 allocs/op
+BenchmarkUnique/UniqueStrings_x_32
+BenchmarkUnique/UniqueStrings_x_32-16        	 3148867	       387.5 ns/op	       0 B/op	       0 allocs/op
+BenchmarkUnique/SliceStringToUnique_x_8
+BenchmarkUnique/SliceStringToUnique_x_8-16   	 2188087	       543.4 ns/op	      64 B/op	       1 allocs/op
+BenchmarkUnique/SliceStringToUnique_x_16
+BenchmarkUnique/SliceStringToUnique_x_16-16  	 2219115	       544.2 ns/op	      64 B/op	       1 allocs/op
+BenchmarkUnique/SliceStringToUnique_x_32
+BenchmarkUnique/SliceStringToUnique_x_32-16  	 2175615	       548.6 ns/op	      64 B/op	       1 allocs/op
+BenchmarkTrim
+BenchmarkTrim/Trim_x_8
+BenchmarkTrim/Trim_x_8-16                    	 5328590	       222.1 ns/op	       0 B/op	       0 allocs/op
+BenchmarkTrim/Trim_x_16
+BenchmarkTrim/Trim_x_16-16                   	 2748080	       422.4 ns/op	       0 B/op	       0 allocs/op
+BenchmarkTrim/Trim_x_32
+BenchmarkTrim/Trim_x_32-16                   	 2732326	       425.8 ns/op	       0 B/op	       0 allocs/op
+BenchmarkTrim/TrimStable_x_8
+BenchmarkTrim/TrimStable_x_8-16              	 5337442	       216.5 ns/op	       0 B/op	       0 allocs/op
+BenchmarkTrim/TrimStable_x_16
+BenchmarkTrim/TrimStable_x_16-16             	 2752138	       435.2 ns/op	       0 B/op	       0 allocs/op
+BenchmarkTrim/TrimStable_x_32
+BenchmarkTrim/TrimStable_x_32-16             	 2770256	       432.0 ns/op	       0 B/op	       0 allocs/op
+BenchmarkTrim/TrimSwap_x_8_(original_version)
+BenchmarkTrim/TrimSwap_x_8_(original_version)-16         	 5299237	       225.4 ns/op	       0 B/op	       0 allocs/op
+BenchmarkTrim/TrimSwap_x_16_(original_version)
+BenchmarkTrim/TrimSwap_x_16_(original_version)-16        	 2744328	       437.5 ns/op	       0 B/op	       0 allocs/op
+BenchmarkTrim/TrimSwap_x_32_(original_version)
+BenchmarkTrim/TrimSwap_x_32_(original_version)-16        	 2800281	       440.1 ns/op	       0 B/op	       0 allocs/op
+BenchmarkTrim/TrimUnique_x_8
+BenchmarkTrim/TrimUnique_x_8-16                          	 4220919	       284.7 ns/op	       0 B/op	       0 allocs/op
+BenchmarkTrim/TrimUnique_x_16
+BenchmarkTrim/TrimUnique_x_16-16                         	 2469658	       482.6 ns/op	       0 B/op	       0 allocs/op
+BenchmarkTrim/TrimUnique_x_32
+BenchmarkTrim/TrimUnique_x_32-16                         	 2408644	       494.1 ns/op	       0 B/op	       0 allocs/op
+BenchmarkTrim/TrimStableUnique_x_8
+BenchmarkTrim/TrimStableUnique_x_8-16                    	 4250454	       288.0 ns/op	       0 B/op	       0 allocs/op
+BenchmarkTrim/TrimStableUnique_x_16
+BenchmarkTrim/TrimStableUnique_x_16-16                   	 2390988	       486.5 ns/op	       0 B/op	       0 allocs/op
+BenchmarkTrim/TrimStableUnique_x_32
+BenchmarkTrim/TrimStableUnique_x_32-16                   	 2416376	       495.0 ns/op	       0 B/op	       0 allocs/op
+PASS
+ok 
+```