From 8a8714c781175f8f1a6c189d919ee8b0ee8c1e27 Mon Sep 17 00:00:00 2001 From: Spandan Das Date: Tue, 25 Apr 2023 18:03:54 +0000 Subject: [PATCH 1/2] Do not modify input in-place SortedUniqueStrings and FirstUniqueStrings dedupes repeating elements and returns the deduped list. Currently, it also modifies the input list in-place, which causes non-determinisitc failures like b/275313114 Operate on a copy of the input so that the input remains untouched. SortedUniqueStrings is O(NlogN) and FirstUniqueStrings is ~O(N), so creating a copy (O(N)) should not result in major performance regressions. Numbers for this single unit test: ``` go test . -run TestStubsForLibraryInMultipleApexes -v -count 1000 Before: 174s After: 172s ``` Test: go test ./android Test: go test . -run TestStubsForLibraryInMultipleApexes -v -count 1000 Change-Id: Id859723b2c2ebdc0023876c4b6fabe75d870bad7 --- android/apex.go | 5 ++--- android/util.go | 3 +++ 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/android/apex.go b/android/apex.go index 5dcf73bc6..823afbb11 100644 --- a/android/apex.go +++ b/android/apex.go @@ -513,9 +513,8 @@ func (m *ApexModuleBase) checkApexAvailableProperty(mctx BaseModuleContext) { // exactly the same set of APEXes (and platform), i.e. if their apex_available // properties have the same elements. func AvailableToSameApexes(mod1, mod2 ApexModule) bool { - // Use CopyOf to prevent non-determinism (b/275313114#comment1) - mod1ApexAvail := SortedUniqueStrings(CopyOf(mod1.apexModuleBase().ApexProperties.Apex_available)) - mod2ApexAvail := SortedUniqueStrings(CopyOf(mod2.apexModuleBase().ApexProperties.Apex_available)) + mod1ApexAvail := SortedUniqueStrings(mod1.apexModuleBase().ApexProperties.Apex_available) + mod2ApexAvail := SortedUniqueStrings(mod2.apexModuleBase().ApexProperties.Apex_available) if len(mod1ApexAvail) != len(mod2ApexAvail) { return false } diff --git a/android/util.go b/android/util.go index 38e0a4dbb..fb09128f8 100644 --- a/android/util.go +++ b/android/util.go @@ -276,6 +276,8 @@ 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. 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 firstUniqueStringsMap(list) @@ -332,6 +334,7 @@ func LastUniqueStrings(list []string) []string { // SortedUniqueStrings returns what the name says func SortedUniqueStrings(list []string) []string { + // FirstUniqueStrings creates a copy of `list`, so the input remains untouched. unique := FirstUniqueStrings(list) sort.Strings(unique) return unique From cc4da765113299fa11dcb1e651ec4ae33e6f8f9b Mon Sep 17 00:00:00 2001 From: Spandan Das Date: Thu, 27 Apr 2023 19:34:08 +0000 Subject: [PATCH 2/2] Differentiate between empty and nil input Previously, CopyOf on an empty list was returning nil. With the updates to SortedUniqueStrings and FirstUniqueStrings, we need to differentiate between empty lists and nil. Test: m nothing Change-Id: I91063ebbe5013cbda5d8f70efde4683c66581599 --- android/util.go | 6 +++++- android/util_test.go | 8 ++++++++ 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/android/util.go b/android/util.go index fb09128f8..08a3521a5 100644 --- a/android/util.go +++ b/android/util.go @@ -26,7 +26,11 @@ import ( // CopyOf returns a new slice that has the same contents as s. func CopyOf(s []string) []string { - return append([]string(nil), s...) + // If the input is nil, return nil and not an empty list + if s == nil { + return s + } + return append([]string{}, s...) } // Concat returns a new slice concatenated from the two input slices. It does not change the input diff --git a/android/util_test.go b/android/util_test.go index 5584b38f7..a2ef58958 100644 --- a/android/util_test.go +++ b/android/util_test.go @@ -381,6 +381,14 @@ func TestRemoveFromList(t *testing.T) { } } +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) + AssertBoolEquals(t, "Copy of a nil list should be a nil list and not an empty list", true, copyOfNilList == nil) +} + func ExampleCopyOf() { a := []string{"1", "2", "3"} b := CopyOf(a)