From 48016d5a2f3d0a518de795e4eba6d899628a86bb Mon Sep 17 00:00:00 2001 From: Colin Cross Date: Tue, 27 Jun 2023 09:45:26 -0700 Subject: [PATCH] Fix FirstUniqueStrings after conversion to generics The conversion of FirstUniqueStrings to be implemented on top of the generic firstUnique accidentally used a different threshold to switch from the list implementation to the map implementation. Modify the threshold of firstUnique to match the old value from FirstUniqueStrings now that it doesn't have the reflection overhead. While we're at it, also make firstUnique make a copy of the list, and make FirstUniqueStrings a pure wrapper around firstUnique. Test: BenchmarkFirstUniqueStrings Change-Id: Icc2febea663142c508ff2e4be65a8a68121631d5 --- android/depset_generic.go | 12 +----------- android/util.go | 27 +++++++++++++++------------ android/util_test.go | 2 +- dexpreopt/dexpreopt.go | 2 -- java/dexpreopt_config.go | 2 -- java/robolectric.go | 2 +- 6 files changed, 18 insertions(+), 29 deletions(-) diff --git a/android/depset_generic.go b/android/depset_generic.go index ae14d3271..4f31b8697 100644 --- a/android/depset_generic.go +++ b/android/depset_generic.go @@ -175,16 +175,6 @@ func (d *DepSet[T]) walk(visit func([]T)) { // its transitive dependencies, in which case the ordering of the duplicated element is not // guaranteed). func (d *DepSet[T]) ToList() []T { - return d.toList(firstUnique[T]) -} - -// toList returns the DepSet flattened to a list. The order in the list is based on the order -// of the DepSet. POSTORDER and PREORDER orders return a postordered or preordered left to right -// flattened list. TOPOLOGICAL returns a list that guarantees that elements of children are listed -// after all of their parents (unless there are duplicate direct elements in the DepSet or any of -// its transitive dependencies, in which case the ordering of the duplicated element is not -// guaranteed). The firstUniqueFunc is used to remove duplicates from the list. -func (d *DepSet[T]) toList(firstUniqueFunc func([]T) []T) []T { if d == nil { return nil } @@ -192,7 +182,7 @@ func (d *DepSet[T]) toList(firstUniqueFunc func([]T) []T) []T { d.walk(func(paths []T) { list = append(list, paths...) }) - list = firstUniqueFunc(list) + list = firstUniqueInPlace(list) if d.reverse { reverseSliceInPlace(list) } diff --git a/android/util.go b/android/util.go index c4ce71a5c..5dbf3cf75 100644 --- a/android/util.go +++ b/android/util.go @@ -25,12 +25,12 @@ import ( ) // CopyOf returns a new slice that has the same contents as s. -func CopyOf(s []string) []string { +func CopyOf[T any](s []T) []T { // If the input is nil, return nil and not an empty list if s == nil { return s } - return append([]string{}, s...) + return append([]T{}, s...) } // Concat returns a new slice concatenated from the two input slices. It does not change the input @@ -278,22 +278,25 @@ func RemoveFromList(s string, list []string) (bool, []string) { } // FirstUniqueStrings returns all unique elements of a slice of strings, keeping the first copy of -// each. It modifies the slice contents in place, and returns a subslice of the original slice. +// each. It does not modify the input slice. func FirstUniqueStrings(list []string) []string { - // Do not moodify the input in-place, operate on a copy instead. - list = CopyOf(list) - // 128 was chosen based on BenchmarkFirstUniqueStrings results. - if len(list) > 128 { - return firstUnique(list) - } return firstUnique(list) } // firstUnique returns all unique elements of a slice, keeping the first copy of each. It -// modifies the slice contents in place, and returns a subslice of the original slice. +// does not modify the input slice. func firstUnique[T comparable](slice []T) []T { - // 4 was chosen based on Benchmark_firstUnique results. - if len(slice) > 4 { + // Do not modify the input in-place, operate on a copy instead. + slice = CopyOf(slice) + return firstUniqueInPlace(slice) +} + +// firstUniqueInPlace returns all unique elements of a slice, keeping the first copy of +// each. It modifies the slice contents in place, and returns a subslice of the original +// slice. +func firstUniqueInPlace[T comparable](slice []T) []T { + // 128 was chosen based on BenchmarkFirstUniqueStrings results. + if len(slice) > 128 { return firstUniqueMap(slice) } return firstUniqueList(slice) diff --git a/android/util_test.go b/android/util_test.go index bee31a9fd..0e28b568b 100644 --- a/android/util_test.go +++ b/android/util_test.go @@ -385,7 +385,7 @@ func TestCopyOfEmptyAndNil(t *testing.T) { emptyList := []string{} copyOfEmptyList := CopyOf(emptyList) AssertBoolEquals(t, "Copy of an empty list should be an empty list and not nil", true, copyOfEmptyList != nil) - copyOfNilList := CopyOf(nil) + copyOfNilList := CopyOf([]string(nil)) AssertBoolEquals(t, "Copy of a nil list should be a nil list and not an empty list", true, copyOfNilList == nil) } diff --git a/dexpreopt/dexpreopt.go b/dexpreopt/dexpreopt.go index 20737e307..7db609bda 100644 --- a/dexpreopt/dexpreopt.go +++ b/dexpreopt/dexpreopt.go @@ -630,5 +630,3 @@ func contains(l []string, s string) bool { } return false } - -var copyOf = android.CopyOf diff --git a/java/dexpreopt_config.go b/java/dexpreopt_config.go index 9100e87c6..28f50d7e3 100644 --- a/java/dexpreopt_config.go +++ b/java/dexpreopt_config.go @@ -250,8 +250,6 @@ func bcpForDexpreopt(ctx android.PathContext, withUpdatable bool) (android.Writa var defaultBootclasspathKey = android.NewOnceKey("defaultBootclasspath") -var copyOf = android.CopyOf - func init() { android.RegisterMakeVarsProvider(pctx, dexpreoptConfigMakevars) } diff --git a/java/robolectric.go b/java/robolectric.go index 6bbe872bb..0041af429 100644 --- a/java/robolectric.go +++ b/java/robolectric.go @@ -299,7 +299,7 @@ func generateSameDirRoboTestConfigJar(ctx android.ModuleContext, outputFile andr func (r *robolectricTest) generateRoboSrcJar(ctx android.ModuleContext, outputFile android.WritablePath, instrumentedApp *AndroidApp) { - srcJarArgs := copyOf(instrumentedApp.srcJarArgs) + srcJarArgs := android.CopyOf(instrumentedApp.srcJarArgs) srcJarDeps := append(android.Paths(nil), instrumentedApp.srcJarDeps...) for _, m := range ctx.GetDirectDepsWithTag(roboCoverageLibsTag) {