From b89d91c67ccfefc8ff0293dbe1fa931c491137c1 Mon Sep 17 00:00:00 2001 From: Colin Cross Date: Fri, 17 Jan 2020 16:52:47 -0800 Subject: [PATCH 1/2] Make FilterPropertyStructSharded smarter FilterPropertyStructSharded was just sharding the top level properties into groups of 10. For nested property structs this can be insufficient - there could be a single top level property with many properties below it. Take a maximum name size, and track the size used by parent structs to determine when sharding a nested struct is necessary. Bug: 146234651 Test: filter_test.go Change-Id: I5b5ed11ea27a0325b2fd6c2c3fb427ea1e2af0c2 --- proptools/filter.go | 204 +++++++++++++++++----------- proptools/filter_test.go | 279 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 404 insertions(+), 79 deletions(-) diff --git a/proptools/filter.go b/proptools/filter.go index 7a61b02..59eca5a 100644 --- a/proptools/filter.go +++ b/proptools/filter.go @@ -15,12 +15,60 @@ package proptools import ( + "fmt" "reflect" + "strconv" ) type FilterFieldPredicate func(field reflect.StructField, string string) (bool, reflect.StructField) -func filterPropertyStructFields(fields []reflect.StructField, prefix string, predicate FilterFieldPredicate) (filteredFields []reflect.StructField, filtered bool) { +type cantFitPanic struct { + field reflect.StructField + size int +} + +func (x cantFitPanic) Error() string { + return fmt.Sprintf("Can't fit field %s %s %s size %d into %d", + x.field.Name, x.field.Type.String(), strconv.Quote(string(x.field.Tag)), + fieldToTypeNameSize(x.field, true)+2, x.size) +} + +// All runtime created structs will have a name that starts with "struct {" and ends with "}" +const emptyStructTypeNameSize = len("struct {}") + +func filterPropertyStructFields(fields []reflect.StructField, prefix string, maxTypeNameSize int, + predicate FilterFieldPredicate) (filteredFieldsShards [][]reflect.StructField, filtered bool) { + + structNameSize := emptyStructTypeNameSize + + var filteredFields []reflect.StructField + + appendAndShardIfNameFull := func(field reflect.StructField) { + fieldTypeNameSize := fieldToTypeNameSize(field, true) + // Every field will have a space before it and either a semicolon or space after it. + fieldTypeNameSize += 2 + + if maxTypeNameSize > 0 && structNameSize+fieldTypeNameSize > maxTypeNameSize { + if len(filteredFields) == 0 { + if field.Type.Kind() == reflect.Struct || + field.Type.Kind() == reflect.Ptr && field.Type.Elem().Kind() == reflect.Struct { + // An error fitting the nested struct should have been caught when recursing + // into the nested struct. + panic(fmt.Errorf("Shouldn't happen: can't fit nested struct %q (%d) into %d", + field.Type.String(), len(field.Type.String()), maxTypeNameSize-structNameSize)) + } + panic(cantFitPanic{field, maxTypeNameSize - structNameSize}) + + } + filteredFieldsShards = append(filteredFieldsShards, filteredFields) + filteredFields = nil + structNameSize = emptyStructTypeNameSize + } + + filteredFields = append(filteredFields, field) + structNameSize += fieldTypeNameSize + } + for _, field := range fields { var keep bool if keep, field = predicate(field, prefix); !keep { @@ -33,32 +81,61 @@ func filterPropertyStructFields(fields []reflect.StructField, prefix string, pre subPrefix = prefix + "." + subPrefix } - // Recurse into structs - switch field.Type.Kind() { - case reflect.Struct: - var subFiltered bool - field.Type, subFiltered = filterPropertyStruct(field.Type, subPrefix, predicate) - filtered = filtered || subFiltered - if field.Type == nil { - continue - } - case reflect.Ptr: - if field.Type.Elem().Kind() == reflect.Struct { - nestedType, subFiltered := filterPropertyStruct(field.Type.Elem(), subPrefix, predicate) - filtered = filtered || subFiltered - if nestedType == nil { - continue - } - field.Type = reflect.PtrTo(nestedType) - } - case reflect.Interface: - panic("Interfaces are not supported in filtered property structs") + ptrToStruct := false + if field.Type.Kind() == reflect.Ptr && field.Type.Elem().Kind() == reflect.Struct { + ptrToStruct = true } - filteredFields = append(filteredFields, field) + // Recurse into structs + if ptrToStruct || field.Type.Kind() == reflect.Struct { + subMaxTypeNameSize := maxTypeNameSize + if maxTypeNameSize > 0 { + // In the worst case where only this nested struct will fit in the outer struct, the + // outer struct will contribute struct{}, the name and tag of the field that contains + // the nested struct, and one space before and after the field. + subMaxTypeNameSize -= emptyStructTypeNameSize + fieldToTypeNameSize(field, false) + 2 + } + typ := field.Type + if ptrToStruct { + subMaxTypeNameSize -= len("*") + typ = typ.Elem() + } + nestedTypes, subFiltered := filterPropertyStruct(typ, subPrefix, subMaxTypeNameSize, predicate) + filtered = filtered || subFiltered + if nestedTypes == nil { + continue + } + + for _, nestedType := range nestedTypes { + if ptrToStruct { + nestedType = reflect.PtrTo(nestedType) + } + field.Type = nestedType + appendAndShardIfNameFull(field) + } + } else { + appendAndShardIfNameFull(field) + } } - return filteredFields, filtered + if len(filteredFields) > 0 { + filteredFieldsShards = append(filteredFieldsShards, filteredFields) + } + + return filteredFieldsShards, filtered +} + +func fieldToTypeNameSize(field reflect.StructField, withType bool) int { + nameSize := len(field.Name) + nameSize += len(" ") + if withType { + nameSize += len(field.Type.String()) + } + if field.Tag != "" { + nameSize += len(" ") + nameSize += len(strconv.Quote(string(field.Tag))) + } + return nameSize } // FilterPropertyStruct takes a reflect.Type that is either a struct or a pointer to a struct, and returns a @@ -66,10 +143,20 @@ func filterPropertyStructFields(fields []reflect.StructField, prefix string, pre // that is true if the new struct type has fewer fields than the original type. If there are no fields in the // original type for which predicate returns true it returns nil and true. func FilterPropertyStruct(prop reflect.Type, predicate FilterFieldPredicate) (filteredProp reflect.Type, filtered bool) { - return filterPropertyStruct(prop, "", predicate) + filteredFieldsShards, filtered := filterPropertyStruct(prop, "", -1, predicate) + switch len(filteredFieldsShards) { + case 0: + return nil, filtered + case 1: + return filteredFieldsShards[0], filtered + default: + panic("filterPropertyStruct should only return 1 struct if maxNameSize < 0") + } } -func filterPropertyStruct(prop reflect.Type, prefix string, predicate FilterFieldPredicate) (filteredProp reflect.Type, filtered bool) { +func filterPropertyStruct(prop reflect.Type, prefix string, maxNameSize int, + predicate FilterFieldPredicate) (filteredProp []reflect.Type, filtered bool) { + var fields []reflect.StructField ptr := prop.Kind() == reflect.Ptr @@ -81,22 +168,26 @@ func filterPropertyStruct(prop reflect.Type, prefix string, predicate FilterFiel fields = append(fields, prop.Field(i)) } - filteredFields, filtered := filterPropertyStructFields(fields, prefix, predicate) + filteredFieldsShards, filtered := filterPropertyStructFields(fields, prefix, maxNameSize, predicate) - if len(filteredFields) == 0 { + if len(filteredFieldsShards) == 0 { return nil, true } if !filtered { if ptr { - return reflect.PtrTo(prop), false + return []reflect.Type{reflect.PtrTo(prop)}, false } - return prop, false + return []reflect.Type{prop}, false } - ret := reflect.StructOf(filteredFields) - if ptr { - ret = reflect.PtrTo(ret) + var ret []reflect.Type + for _, filteredFields := range filteredFieldsShards { + p := reflect.StructOf(filteredFields) + if ptr { + p = reflect.PtrTo(p) + } + ret = append(ret, p) } return ret, true @@ -109,51 +200,6 @@ func filterPropertyStruct(prop reflect.Type, prefix string, predicate FilterFiel // level fields in it to attempt to avoid hitting the 65535 byte type name length limit in reflect.StructOf // (reflect.nameFrom: name too long), although the limit can still be reached with a single struct field with many // fields in it. -func FilterPropertyStructSharded(prop reflect.Type, predicate FilterFieldPredicate) (filteredProp []reflect.Type, filtered bool) { - var fields []reflect.StructField - - ptr := prop.Kind() == reflect.Ptr - if ptr { - prop = prop.Elem() - } - - for i := 0; i < prop.NumField(); i++ { - fields = append(fields, prop.Field(i)) - } - - fields, filtered = filterPropertyStructFields(fields, "", predicate) - if !filtered { - if ptr { - return []reflect.Type{reflect.PtrTo(prop)}, false - } - return []reflect.Type{prop}, false - } - - if len(fields) == 0 { - return nil, true - } - - shards := shardFields(fields, 10) - - for _, shard := range shards { - s := reflect.StructOf(shard) - if ptr { - s = reflect.PtrTo(s) - } - filteredProp = append(filteredProp, s) - } - - return filteredProp, true -} - -func shardFields(fields []reflect.StructField, shardSize int) [][]reflect.StructField { - ret := make([][]reflect.StructField, 0, (len(fields)+shardSize-1)/shardSize) - for len(fields) > shardSize { - ret = append(ret, fields[0:shardSize]) - fields = fields[shardSize:] - } - if len(fields) > 0 { - ret = append(ret, fields) - } - return ret +func FilterPropertyStructSharded(prop reflect.Type, maxTypeNameSize int, predicate FilterFieldPredicate) (filteredProp []reflect.Type, filtered bool) { + return filterPropertyStruct(prop, "", maxTypeNameSize, predicate) } diff --git a/proptools/filter_test.go b/proptools/filter_test.go index 695549a..0ea04bb 100644 --- a/proptools/filter_test.go +++ b/proptools/filter_test.go @@ -16,6 +16,7 @@ package proptools import ( "reflect" + "strings" "testing" ) @@ -237,3 +238,281 @@ func TestFilterPropertyStruct(t *testing.T) { }) } } + +func TestFilterPropertyStructSharded(t *testing.T) { + tests := []struct { + name string + maxNameSize int + in interface{} + out []interface{} + filtered bool + }{ + // Property tests + { + name: "basic", + maxNameSize: 20, + in: &struct { + A *string `keep:"true"` + B *string `keep:"true"` + C *string + }{}, + out: []interface{}{ + &struct { + A *string + }{}, + &struct { + B *string + }{}, + }, + filtered: true, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + out, filtered := filterPropertyStruct(reflect.TypeOf(test.in), "", test.maxNameSize, + func(field reflect.StructField, prefix string) (bool, reflect.StructField) { + if HasTag(field, "keep", "true") { + field.Tag = "" + return true, field + } + return false, field + }) + if filtered != test.filtered { + t.Errorf("expected filtered %v, got %v", test.filtered, filtered) + } + var expected []reflect.Type + for _, t := range test.out { + expected = append(expected, reflect.TypeOf(t)) + } + if !reflect.DeepEqual(out, expected) { + t.Errorf("expected type %v, got %v", expected, out) + } + }) + } +} + +func Test_fieldToTypeNameSize(t *testing.T) { + tests := []struct { + name string + field reflect.StructField + }{ + { + name: "string", + field: reflect.StructField{ + Name: "Foo", + Type: reflect.TypeOf(""), + }, + }, + { + name: "string pointer", + field: reflect.StructField{ + Name: "Foo", + Type: reflect.TypeOf(StringPtr("")), + }, + }, + { + name: "anonymous struct", + field: reflect.StructField{ + Name: "Foo", + Type: reflect.TypeOf(struct{ foo string }{}), + }, + }, { + name: "anonymous struct pointer", + field: reflect.StructField{ + Name: "Foo", + Type: reflect.TypeOf(&struct{ foo string }{}), + }, + }, + } + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + typeName := reflect.StructOf([]reflect.StructField{test.field}).String() + typeName = strings.TrimPrefix(typeName, "struct { ") + typeName = strings.TrimSuffix(typeName, " }") + if g, w := fieldToTypeNameSize(test.field, true), len(typeName); g != w { + t.Errorf("want fieldToTypeNameSize(..., true) = %v, got %v", w, g) + } + if g, w := fieldToTypeNameSize(test.field, false), len(typeName)-len(test.field.Type.String()); g != w { + t.Errorf("want fieldToTypeNameSize(..., false) = %v, got %v", w, g) + } + }) + } +} + +func Test_filterPropertyStructFields(t *testing.T) { + type args struct { + } + tests := []struct { + name string + maxTypeNameSize int + in interface{} + out []interface{} + }{ + { + name: "empty", + maxTypeNameSize: -1, + in: struct{}{}, + out: nil, + }, + { + name: "one", + maxTypeNameSize: -1, + in: struct { + A *string + }{}, + out: []interface{}{ + struct { + A *string + }{}, + }, + }, + { + name: "two", + maxTypeNameSize: 20, + in: struct { + A *string + B *string + }{}, + out: []interface{}{ + struct { + A *string + }{}, + struct { + B *string + }{}, + }, + }, + { + name: "nested", + maxTypeNameSize: 36, + in: struct { + AAAAA struct { + A string + } + BBBBB struct { + B string + } + }{}, + out: []interface{}{ + struct { + AAAAA struct { + A string + } + }{}, + struct { + BBBBB struct { + B string + } + }{}, + }, + }, + { + name: "nested pointer", + maxTypeNameSize: 37, + in: struct { + AAAAA *struct { + A string + } + BBBBB *struct { + B string + } + }{}, + out: []interface{}{ + struct { + AAAAA *struct { + A string + } + }{}, + struct { + BBBBB *struct { + B string + } + }{}, + }, + }, + { + name: "doubly nested", + maxTypeNameSize: 49, + in: struct { + AAAAA struct { + A struct { + A string + } + } + BBBBB struct { + B struct { + B string + } + } + }{}, + out: []interface{}{ + struct { + AAAAA struct { + A struct { + A string + } + } + }{}, + struct { + BBBBB struct { + B struct { + B string + } + } + }{}, + }, + }, + } + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + inType := reflect.TypeOf(test.in) + var in []reflect.StructField + for i := 0; i < inType.NumField(); i++ { + in = append(in, inType.Field(i)) + } + + keep := func(field reflect.StructField, string string) (bool, reflect.StructField) { + return true, field + } + + // Test that maxTypeNameSize is the + if test.maxTypeNameSize > 0 { + correctPanic := false + func() { + defer func() { + if r := recover(); r != nil { + if _, ok := r.(cantFitPanic); ok { + correctPanic = true + } else { + panic(r) + } + } + }() + + _, _ = filterPropertyStructFields(in, "", test.maxTypeNameSize-1, keep) + }() + + if !correctPanic { + t.Errorf("filterPropertyStructFields() with size-1 should produce cantFitPanic") + } + } + + filteredFieldsShards, _ := filterPropertyStructFields(in, "", test.maxTypeNameSize, keep) + + var out []interface{} + for _, filteredFields := range filteredFieldsShards { + typ := reflect.StructOf(filteredFields) + if test.maxTypeNameSize > 0 && len(typ.String()) > test.maxTypeNameSize { + t.Errorf("out %q expected size <= %d, got %d", + typ.String(), test.maxTypeNameSize, len(typ.String())) + } + out = append(out, reflect.Zero(typ).Interface()) + } + + if g, w := out, test.out; !reflect.DeepEqual(g, w) { + t.Errorf("filterPropertyStructFields() want %v, got %v", w, g) + } + }) + } +} From 571f77a60d6be13362d1078d0866c5dbd72936b2 Mon Sep 17 00:00:00 2001 From: Colin Cross Date: Mon, 20 Jan 2020 17:12:29 -0800 Subject: [PATCH 2/2] Remove blueprint:"filter(*)" tag support The filter tag is unused, replaced with FilterPropertyStruct to generate a new type at runtime that only contains the filtered fields. Test: unpack_test.go Change-Id: Id91cf99290832094d05426f3263279836f0fea73 --- bootstrap/bpdoc/bpdoc.go | 8 ----- proptools/unpack.go | 70 ++++------------------------------------ proptools/unpack_test.go | 34 ------------------- 3 files changed, 6 insertions(+), 106 deletions(-) diff --git a/bootstrap/bpdoc/bpdoc.go b/bootstrap/bpdoc/bpdoc.go index a33886d..4abf2e7 100644 --- a/bootstrap/bpdoc/bpdoc.go +++ b/bootstrap/bpdoc/bpdoc.go @@ -145,14 +145,6 @@ func assembleModuleTypeInfo(r *Reader, name string, factory reflect.Value, return nil, fmt.Errorf("nesting point %q not found", nestedName) } - key, value, err := proptools.HasFilter(nestPoint.Tag) - if err != nil { - return nil, err - } - if key != "" { - nested.IncludeByTag(key, value) - } - nestPoint.Nest(nested) } mt.PropertyStructs = append(mt.PropertyStructs, ps) diff --git a/proptools/unpack.go b/proptools/unpack.go index 1d95444..e7d4fff 100644 --- a/proptools/unpack.go +++ b/proptools/unpack.go @@ -17,8 +17,6 @@ package proptools import ( "fmt" "reflect" - "strconv" - "strings" "text/scanner" "github.com/google/blueprint/parser" @@ -60,7 +58,7 @@ func UnpackProperties(propertyDefs []*parser.Property, panic("properties must be a pointer to a struct") } - newErrs := unpackStructValue("", propertiesValue, propertyMap, "", "") + newErrs := unpackStructValue("", propertiesValue, propertyMap) errs = append(errs, newErrs...) if len(errs) >= maxUnpackErrors { @@ -130,7 +128,7 @@ func buildPropertyMap(namePrefix string, propertyDefs []*parser.Property, } func unpackStructValue(namePrefix string, structValue reflect.Value, - propertyMap map[string]*packedProperty, filterKey, filterValue string) []error { + propertyMap map[string]*packedProperty) []error { structType := structValue.Type() @@ -215,7 +213,7 @@ func unpackStructValue(namePrefix string, structValue reflect.Value, } if field.Anonymous && fieldValue.Kind() == reflect.Struct { - newErrs := unpackStructValue(namePrefix, fieldValue, propertyMap, filterKey, filterValue) + newErrs := unpackStructValue(namePrefix, fieldValue, propertyMap) errs = append(errs, newErrs...) continue } @@ -239,40 +237,11 @@ func unpackStructValue(namePrefix string, structValue reflect.Value, continue } - if filterKey != "" && !HasTag(field, filterKey, filterValue) { - errs = append(errs, - &UnpackError{ - Err: fmt.Errorf("filtered field %s cannot be set in a Blueprint file", propertyName), - Pos: packedProperty.property.ColonPos, - }) - if len(errs) >= maxUnpackErrors { - return errs - } - continue - } - var newErrs []error if fieldValue.Kind() == reflect.Struct { - localFilterKey, localFilterValue := filterKey, filterValue - if k, v, err := HasFilter(field.Tag); err != nil { - errs = append(errs, err) - if len(errs) >= maxUnpackErrors { - return errs - } - } else if k != "" { - if filterKey != "" { - errs = append(errs, fmt.Errorf("nested filter tag not supported on field %q", - field.Name)) - if len(errs) >= maxUnpackErrors { - return errs - } - } else { - localFilterKey, localFilterValue = k, v - } - } newErrs = unpackStruct(propertyName+".", fieldValue, - packedProperty.property, propertyMap, localFilterKey, localFilterValue) + packedProperty.property, propertyMap) errs = append(errs, newErrs...) if len(errs) >= maxUnpackErrors { @@ -365,8 +334,7 @@ func propertyToValue(typ reflect.Type, property *parser.Property) (reflect.Value } func unpackStruct(namePrefix string, structValue reflect.Value, - property *parser.Property, propertyMap map[string]*packedProperty, - filterKey, filterValue string) []error { + property *parser.Property, propertyMap map[string]*packedProperty) []error { m, ok := property.Value.Eval().(*parser.Map) if !ok { @@ -381,31 +349,5 @@ func unpackStruct(namePrefix string, structValue reflect.Value, return errs } - return unpackStructValue(namePrefix, structValue, propertyMap, filterKey, filterValue) -} - -func HasFilter(field reflect.StructTag) (k, v string, err error) { - tag := field.Get("blueprint") - for _, entry := range strings.Split(tag, ",") { - if strings.HasPrefix(entry, "filter") { - if !strings.HasPrefix(entry, "filter(") || !strings.HasSuffix(entry, ")") { - return "", "", fmt.Errorf("unexpected format for filter %q: missing ()", entry) - } - entry = strings.TrimPrefix(entry, "filter(") - entry = strings.TrimSuffix(entry, ")") - - s := strings.Split(entry, ":") - if len(s) != 2 { - return "", "", fmt.Errorf("unexpected format for filter %q: expected single ':'", entry) - } - k = s[0] - v, err = strconv.Unquote(s[1]) - if err != nil { - return "", "", fmt.Errorf("unexpected format for filter %q: %s", entry, err.Error()) - } - return k, v, nil - } - } - - return "", "", nil + return unpackStructValue(namePrefix, structValue, propertyMap) } diff --git a/proptools/unpack_test.go b/proptools/unpack_test.go index 8d8c543..931cfdd 100644 --- a/proptools/unpack_test.go +++ b/proptools/unpack_test.go @@ -16,7 +16,6 @@ package proptools import ( "bytes" - "fmt" "reflect" "testing" "text/scanner" @@ -219,39 +218,6 @@ var validUnpackTestCases = []struct { }, }, - { - input: ` - m { - nested: { - foo: "abc", - }, - bar: false, - baz: ["def", "ghi"], - } - `, - output: []interface{}{ - struct { - Nested struct { - Foo string - } `blueprint:"filter(allowNested:\"true\")"` - Bar bool - Baz []string - }{ - Nested: struct{ Foo string }{ - Foo: "", - }, - Bar: false, - Baz: []string{"def", "ghi"}, - }, - }, - errs: []error{ - &UnpackError{ - Err: fmt.Errorf("filtered field nested.foo cannot be set in a Blueprint file"), - Pos: mkpos(30, 4, 9), - }, - }, - }, - // Anonymous struct { input: `