Optimize filterArchStruct when nothing is filtered am: cb9880786d

am: f53ca3118f

Change-Id: I99f90f0f1ef5db89bcd1e0437a3ccedae6e459f6

Android.bp

@@ -71,6 +71,7 @@ bootstrap_go_package {
         "android/env.go",
     ],
     testSrcs: [
+        "android/arch_test.go",
         "android/config_test.go",
         "android/expand_test.go",
         "android/namespace_test.go",

android/arch.go

@@ -616,10 +616,10 @@ func decodeMultilib(base *ModuleBase, class OsClass) (multilib, extraMultilib st
 	}
 }
 
-func filterArchStructFields(fields []reflect.StructField) []reflect.StructField {
+func filterArchStructFields(fields []reflect.StructField) (filteredFields []reflect.StructField, filtered bool) {
-	var ret []reflect.StructField
 	for _, field := range fields {
 		if !proptools.HasTag(field, "android", "arch_variant") {
+			filtered = true
 			continue
 		}
 
@@ -637,15 +637,17 @@ func filterArchStructFields(fields []reflect.StructField) []reflect.StructField
 		// Recurse into structs
 		switch field.Type.Kind() {
 		case reflect.Struct:
-			var ok bool
+			var subFiltered bool
-			field.Type, ok = filterArchStruct(field.Type)
+			field.Type, subFiltered = filterArchStruct(field.Type)
-			if !ok {
+			filtered = filtered || subFiltered
+			if field.Type == nil {
 				continue
 			}
 		case reflect.Ptr:
 			if field.Type.Elem().Kind() == reflect.Struct {
-				nestedType, ok := filterArchStruct(field.Type.Elem())
+				nestedType, subFiltered := filterArchStruct(field.Type.Elem())
-				if !ok {
+				filtered = filtered || subFiltered
+				if nestedType == nil {
 					continue
 				}
 				field.Type = reflect.PtrTo(nestedType)
@@ -654,13 +656,17 @@ func filterArchStructFields(fields []reflect.StructField) []reflect.StructField
 			panic("Interfaces are not supported in arch_variant properties")
 		}
 
-		ret = append(ret, field)
+		filteredFields = append(filteredFields, field)
 	}
 
-	return ret
+	return filteredFields, filtered
 }
 
-func filterArchStruct(prop reflect.Type) (reflect.Type, bool) {
+// filterArchStruct takes a reflect.Type that is either a sturct or a pointer to a struct, and returns a reflect.Type
+// that only contains the fields in the original type that have an `android:"arch_variant"` struct tag, and a bool
+// that is true if the new struct type has fewer fields than the original type.  If there are no fields in the
+// original type with the struct tag it returns nil and true.
+func filterArchStruct(prop reflect.Type) (filteredProp reflect.Type, filtered bool) {
 	var fields []reflect.StructField
 
 	ptr := prop.Kind() == reflect.Ptr
@@ -672,13 +678,20 @@ func filterArchStruct(prop reflect.Type) (reflect.Type, bool) {
 		fields = append(fields, prop.Field(i))
 	}
 
-	fields = filterArchStructFields(fields)
+	filteredFields, filtered := filterArchStructFields(fields)
 
-	if len(fields) == 0 {
+	if len(filteredFields) == 0 {
-		return nil, false
+		return nil, true
 	}
 
-	ret := reflect.StructOf(fields)
+	if !filtered {
+		if ptr {
+			return reflect.PtrTo(prop), false
+		}
+		return prop, false
+	}
+
+	ret := reflect.StructOf(filteredFields)
 	if ptr {
 		ret = reflect.PtrTo(ret)
 	}
@@ -686,7 +699,13 @@ func filterArchStruct(prop reflect.Type) (reflect.Type, bool) {
 	return ret, true
 }
 
-func filterArchStructSharded(prop reflect.Type) ([]reflect.Type, bool) {
+// filterArchStruct takes a reflect.Type that is either a sturct or a pointer to a struct, and returns a list of
+// reflect.Type that only contains the fields in the original type that have an `android:"arch_variant"` struct tag,
+// and a bool that is true if the new struct type has fewer fields than the original type.  If there are no fields in
+// the original type with the struct tag it returns nil and true.  Each returned struct type will have a maximum of
+// 10 top level fields in it to attempt to avoid hitting the reflect.StructOf name length limit, although the limit
+// can still be reached with a single struct field with many fields in it.
+func filterArchStructSharded(prop reflect.Type) (filteredProp []reflect.Type, filtered bool) {
 	var fields []reflect.StructField
 
 	ptr := prop.Kind() == reflect.Ptr
@@ -698,24 +717,29 @@ func filterArchStructSharded(prop reflect.Type) ([]reflect.Type, bool) {
 		fields = append(fields, prop.Field(i))
 	}
 
-	fields = filterArchStructFields(fields)
+	fields, filtered = filterArchStructFields(fields)
+	if !filtered {
+		if ptr {
+			return []reflect.Type{reflect.PtrTo(prop)}, false
+		}
+		return []reflect.Type{prop}, false
+	}
 
 	if len(fields) == 0 {
-		return nil, false
+		return nil, true
 	}
 
 	shards := shardFields(fields, 10)
 
-	var ret []reflect.Type
 	for _, shard := range shards {
 		s := reflect.StructOf(shard)
 		if ptr {
 			s = reflect.PtrTo(s)
 		}
-		ret = append(ret, s)
+		filteredProp = append(filteredProp, s)
 	}
 
-	return ret, true
+	return filteredProp, true
 }
 
 func shardFields(fields []reflect.StructField, shardSize int) [][]reflect.StructField {
@@ -730,9 +754,12 @@ func shardFields(fields []reflect.StructField, shardSize int) [][]reflect.Struct
 	return ret
 }
 
+// createArchType takes a reflect.Type that is either a struct or a pointer to a struct, and returns a list of
+// reflect.Type that contains the arch-variant properties inside structs for each architecture, os, target, multilib,
+// etc.
 func createArchType(props reflect.Type) []reflect.Type {
-	propShards, ok := filterArchStructSharded(props)
+	propShards, _ := filterArchStructSharded(props)
-	if !ok {
+	if len(propShards) == 0 {
 		return nil
 	}
 

android/arch_test.go

@@ -0,0 +1,232 @@
+// Copyright 2019 Google Inc. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package android
+
+import (
+	"reflect"
+	"testing"
+)
+
+type Named struct {
+	A *string `android:"arch_variant"`
+	B *string
+}
+
+type NamedAllFiltered struct {
+	A *string
+}
+
+type NamedNoneFiltered struct {
+	A *string `android:"arch_variant"`
+}
+
+func TestFilterArchStruct(t *testing.T) {
+	tests := []struct {
+		name     string
+		in       interface{}
+		out      interface{}
+		filtered bool
+	}{
+		// Property tests
+		{
+			name: "basic",
+			in: &struct {
+				A *string `android:"arch_variant"`
+				B *string
+			}{},
+			out: &struct {
+				A *string
+			}{},
+			filtered: true,
+		},
+		{
+			name: "all filtered",
+			in: &struct {
+				A *string
+			}{},
+			out:      nil,
+			filtered: true,
+		},
+		{
+			name: "none filtered",
+			in: &struct {
+				A *string `android:"arch_variant"`
+			}{},
+			out: &struct {
+				A *string `android:"arch_variant"`
+			}{},
+			filtered: false,
+		},
+
+		// Sub-struct tests
+		{
+			name: "substruct",
+			in: &struct {
+				A struct {
+					A *string `android:"arch_variant"`
+					B *string
+				} `android:"arch_variant"`
+			}{},
+			out: &struct {
+				A struct {
+					A *string
+				}
+			}{},
+			filtered: true,
+		},
+		{
+			name: "substruct all filtered",
+			in: &struct {
+				A struct {
+					A *string
+				} `android:"arch_variant"`
+			}{},
+			out:      nil,
+			filtered: true,
+		},
+		{
+			name: "substruct none filtered",
+			in: &struct {
+				A struct {
+					A *string `android:"arch_variant"`
+				} `android:"arch_variant"`
+			}{},
+			out: &struct {
+				A struct {
+					A *string `android:"arch_variant"`
+				} `android:"arch_variant"`
+			}{},
+			filtered: false,
+		},
+
+		// Named sub-struct tests
+		{
+			name: "named substruct",
+			in: &struct {
+				A Named `android:"arch_variant"`
+			}{},
+			out: &struct {
+				A struct {
+					A *string
+				}
+			}{},
+			filtered: true,
+		},
+		{
+			name: "substruct all filtered",
+			in: &struct {
+				A NamedAllFiltered `android:"arch_variant"`
+			}{},
+			out:      nil,
+			filtered: true,
+		},
+		{
+			name: "substruct none filtered",
+			in: &struct {
+				A NamedNoneFiltered `android:"arch_variant"`
+			}{},
+			out: &struct {
+				A NamedNoneFiltered `android:"arch_variant"`
+			}{},
+			filtered: false,
+		},
+
+		// Pointer to sub-struct tests
+		{
+			name: "pointer substruct",
+			in: &struct {
+				A *struct {
+					A *string `android:"arch_variant"`
+					B *string
+				} `android:"arch_variant"`
+			}{},
+			out: &struct {
+				A *struct {
+					A *string
+				}
+			}{},
+			filtered: true,
+		},
+		{
+			name: "pointer substruct all filtered",
+			in: &struct {
+				A *struct {
+					A *string
+				} `android:"arch_variant"`
+			}{},
+			out:      nil,
+			filtered: true,
+		},
+		{
+			name: "pointer substruct none filtered",
+			in: &struct {
+				A *struct {
+					A *string `android:"arch_variant"`
+				} `android:"arch_variant"`
+			}{},
+			out: &struct {
+				A *struct {
+					A *string `android:"arch_variant"`
+				} `android:"arch_variant"`
+			}{},
+			filtered: false,
+		},
+
+		// Pointer to named sub-struct tests
+		{
+			name: "pointer named substruct",
+			in: &struct {
+				A *Named `android:"arch_variant"`
+			}{},
+			out: &struct {
+				A *struct {
+					A *string
+				}
+			}{},
+			filtered: true,
+		},
+		{
+			name: "pointer substruct all filtered",
+			in: &struct {
+				A *NamedAllFiltered `android:"arch_variant"`
+			}{},
+			out:      nil,
+			filtered: true,
+		},
+		{
+			name: "pointer substruct none filtered",
+			in: &struct {
+				A *NamedNoneFiltered `android:"arch_variant"`
+			}{},
+			out: &struct {
+				A *NamedNoneFiltered `android:"arch_variant"`
+			}{},
+			filtered: false,
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			out, filtered := filterArchStruct(reflect.TypeOf(test.in))
+			if filtered != test.filtered {
+				t.Errorf("expected filtered %v, got %v", test.filtered, filtered)
+			}
+			expected := reflect.TypeOf(test.out)
+			if out != expected {
+				t.Errorf("expected type %v, got %v", expected, out)
+			}
+		})
+	}
+}