Merge remote-tracking branch 'aosp/upstream'

* aosp/upstream:
  Fix detecting cycles in parallelVisit
  Optimize HasTag function

Test: m checkbuild
Change-Id: Ia121f48bafa1461b8ea0ae6103d1b5bb503e6c16

context.go

@@ -2074,23 +2074,14 @@ func parallelVisit(modules []*moduleInfo, order visitOrderer, limit int,
 		}
 
 		if len(pauseMap) > 0 {
-			// Probably a deadlock due to a newly added dependency cycle.
-			// Start from a semi-random module being paused for and perform a depth-first
-			// search for the module it is paused on, ignoring modules that are marked as
-			// done.  Note this traverses from modules to the modules that would have been
-			// unblocked when that module finished.
-			var start, end *moduleInfo
-			for _, pauseSpecs := range pauseMap {
-				for _, pauseSpec := range pauseSpecs {
-					if start == nil || start.String() > pauseSpec.paused.String() {
-						start = pauseSpec.paused
-						end = pauseSpec.until
-					}
-				}
-			}
+			// Probably a deadlock due to a newly added dependency cycle. Start from each module in
+			// the order of the input modules list and perform a depth-first search for the module
+			// it is paused on, ignoring modules that are marked as done.  Note this traverses from
+			// modules to the modules that would have been unblocked when that module finished, i.e
+			// the reverse of the visitOrderer.
 
-			var check func(group *moduleInfo) []*moduleInfo
-			check = func(module *moduleInfo) []*moduleInfo {
+			var check func(module, end *moduleInfo) []*moduleInfo
+			check = func(module, end *moduleInfo) []*moduleInfo {
 				if module.waitingCount == -1 {
 					// This module was finished, it can't be part of a loop.
 					return nil
@@ -2101,13 +2092,13 @@ func parallelVisit(modules []*moduleInfo, order visitOrderer, limit int,
 				}
 
 				for _, dep := range order.propagate(module) {
-					cycle := check(dep)
+					cycle := check(dep, end)
 					if cycle != nil {
 						return append([]*moduleInfo{module}, cycle...)
 					}
 				}
 				for _, depPauseSpec := range pauseMap[module] {
-					cycle := check(depPauseSpec.paused)
+					cycle := check(depPauseSpec.paused, end)
 					if cycle != nil {
 						return append([]*moduleInfo{module}, cycle...)
 					}
@@ -2116,9 +2107,14 @@ func parallelVisit(modules []*moduleInfo, order visitOrderer, limit int,
 				return nil
 			}
 
-			cycle := check(start)
-			if cycle != nil {
-				return cycleError(cycle)
+			// Iterate over the modules list instead of pauseMap to provide deterministic ordering.
+			for _, module := range modules {
+				for _, pauseSpec := range pauseMap[module] {
+					cycle := check(pauseSpec.paused, pauseSpec.until)
+					if len(cycle) > 0 {
+						return cycleError(cycle)
+					}
+				}
 			}
 		}
 

context_test.go

@@ -840,38 +840,31 @@ func Test_findVariant(t *testing.T) {
 }
 
 func Test_parallelVisit(t *testing.T) {
-	moduleA := &moduleInfo{
-		group: &moduleGroup{
-			name: "A",
-		},
-	}
-	moduleB := &moduleInfo{
-		group: &moduleGroup{
-			name: "B",
-		},
-	}
-	moduleC := &moduleInfo{
-		group: &moduleGroup{
-			name: "C",
-		},
-	}
-	moduleD := &moduleInfo{
-		group: &moduleGroup{
-			name: "D",
-		},
-	}
-	moduleA.group.modules = modulesOrAliases{moduleA}
-	moduleB.group.modules = modulesOrAliases{moduleB}
-	moduleC.group.modules = modulesOrAliases{moduleC}
-	moduleD.group.modules = modulesOrAliases{moduleD}
-
 	addDep := func(from, to *moduleInfo) {
 		from.directDeps = append(from.directDeps, depInfo{to, nil})
 		from.forwardDeps = append(from.forwardDeps, to)
 		to.reverseDeps = append(to.reverseDeps, from)
 	}
 
-	// A depends on B, B depends on C.  Nothing depends on D, and D doesn't depend on anything.
+	create := func(name string) *moduleInfo {
+		m := &moduleInfo{
+			group: &moduleGroup{
+				name: name,
+			},
+		}
+		m.group.modules = modulesOrAliases{m}
+		return m
+	}
+	moduleA := create("A")
+	moduleB := create("B")
+	moduleC := create("C")
+	moduleD := create("D")
+	moduleE := create("E")
+	moduleF := create("F")
+	moduleG := create("G")
+
+	// A depends on B, B depends on C.  Nothing depends on D through G, and they don't depend on
+	// anything.
 	addDep(moduleA, moduleB)
 	addDep(moduleB, moduleC)
 
@@ -1037,8 +1030,45 @@ func Test_parallelVisit(t *testing.T) {
 			})
 		want := []string{
 			`encountered dependency cycle`,
-			`"C" depends on "D"`,
 			`"D" depends on "C"`,
+			`"C" depends on "D"`,
+		}
+		for i := range want {
+			if len(errs) <= i {
+				t.Errorf("missing error %s", want[i])
+			} else if !strings.Contains(errs[i].Error(), want[i]) {
+				t.Errorf("expected error %s, got %s", want[i], errs[i])
+			}
+		}
+		if len(errs) > len(want) {
+			for _, err := range errs[len(want):] {
+				t.Errorf("unexpected error %s", err.Error())
+			}
+		}
+	})
+	t.Run("pause cycle with deps", func(t *testing.T) {
+		pauseDeps := map[*moduleInfo]*moduleInfo{
+			// F and G form a pause cycle
+			moduleF: moduleG,
+			moduleG: moduleF,
+			// D depends on E which depends on the pause cycle, making E the first alphabetical
+			// entry in pauseMap, which is not part of the cycle.
+			moduleD: moduleE,
+			moduleE: moduleF,
+		}
+		errs := parallelVisit([]*moduleInfo{moduleD, moduleE, moduleF, moduleG}, bottomUpVisitorImpl{}, 4,
+			func(module *moduleInfo, pause chan<- pauseSpec) bool {
+				if dep, ok := pauseDeps[module]; ok {
+					unpause := make(chan struct{})
+					pause <- pauseSpec{module, dep, unpause}
+					<-unpause
+				}
+				return false
+			})
+		want := []string{
+			`encountered dependency cycle`,
+			`"G" depends on "F"`,
+			`"F" depends on "G"`,
 		}
 		for i := range want {
 			if len(errs) <= i {

proptools/tag.go

@@ -23,10 +23,17 @@ import (
 // HasTag returns true if a StructField has a tag in the form `name:"foo,value"`.
 func HasTag(field reflect.StructField, name, value string) bool {
 	tag := field.Tag.Get(name)
-	for _, entry := range strings.Split(tag, ",") {
-		if entry == value {
+	for len(tag) > 0 {
+		idx := strings.Index(tag, ",")
+
+		if idx < 0 {
+			return tag == value
+		}
+		if tag[:idx] == value {
 			return true
 		}
+
+		tag = tag[idx+1:]
 	}
 
 	return false

proptools/tag_test.go

@@ -19,16 +19,16 @@ import (
 	"testing"
 )
 
-func TestHasTag(t *testing.T) {
-	type testType struct {
-		NoTag       string
-		EmptyTag    string ``
-		OtherTag    string `foo:"bar"`
-		MatchingTag string `name:"value"`
-		ExtraValues string `name:"foo,value,bar"`
-		ExtraTags   string `foo:"bar" name:"value"`
-	}
+type testType struct {
+	NoTag       string
+	EmptyTag    string ``
+	OtherTag    string `foo:"bar"`
+	MatchingTag string `name:"value"`
+	ExtraValues string `name:"foo,value,bar"`
+	ExtraTags   string `foo:"bar" name:"value"`
+}
 
+func TestHasTag(t *testing.T) {
 	tests := []struct {
 		field string
 		want  bool
@@ -68,6 +68,39 @@ func TestHasTag(t *testing.T) {
 	}
 }
 
+func BenchmarkHasTag(b *testing.B) {
+	tests := []struct {
+		field string
+	}{
+		{
+			field: "NoTag",
+		},
+		{
+			field: "EmptyTag",
+		},
+		{
+			field: "OtherTag",
+		},
+		{
+			field: "MatchingTag",
+		},
+		{
+			field: "ExtraValues",
+		},
+		{
+			field: "ExtraTags",
+		},
+	}
+	for _, test := range tests {
+		b.Run(test.field, func(b *testing.B) {
+			field, _ := reflect.TypeOf(testType{}).FieldByName(test.field)
+			for i := 0; i < b.N; i++ {
+				HasTag(field, "name", "value")
+			}
+		})
+	}
+}
+
 func TestPropertyIndexesWithTag(t *testing.T) {
 	tests := []struct {
 		name string