Merge pull request #110 from colincross/opt

Optimize proptools
This commit is contained in:
colincross 2016-08-08 15:48:13 -07:00 committed by GitHub
commit fd1ef9ec7a
9 changed files with 432 additions and 193 deletions

View file

@ -152,15 +152,18 @@ func setDefaults(properties []Property, defaults reflect.Value) {
continue
}
if f.Type().Kind() == reflect.Interface {
if f.Kind() == reflect.Interface {
f = f.Elem()
}
if f.Type().Kind() == reflect.Ptr {
if f.Kind() == reflect.Ptr {
if f.IsNil() {
continue
}
f = f.Elem()
}
if f.Type().Kind() == reflect.Struct {
if f.Kind() == reflect.Struct {
setDefaults(prop.Properties, f)
} else {
prop.Default = fmt.Sprintf("%v", f.Interface())

View file

@ -23,6 +23,7 @@ import (
"path/filepath"
"runtime"
"runtime/pprof"
"runtime/trace"
"github.com/google/blueprint"
"github.com/google/blueprint/deptools"
@ -36,6 +37,7 @@ var (
manifestFile string
docFile string
cpuprofile string
traceFile string
runGoTests bool
BuildDir string
@ -50,6 +52,7 @@ func init() {
flag.StringVar(&manifestFile, "m", "", "the bootstrap manifest file")
flag.StringVar(&docFile, "docs", "", "build documentation file to output")
flag.StringVar(&cpuprofile, "cpuprofile", "", "write cpu profile to file")
flag.StringVar(&traceFile, "trace", "", "write trace to file")
flag.BoolVar(&runGoTests, "t", false, "build and run go tests during bootstrap")
}
@ -70,6 +73,16 @@ func Main(ctx *blueprint.Context, config interface{}, extraNinjaFileDeps ...stri
defer pprof.StopCPUProfile()
}
if traceFile != "" {
f, err := os.Create(traceFile)
if err != nil {
fatalf("error opening trace: %s", err)
}
trace.Start(f)
defer f.Close()
defer trace.Stop()
}
if flag.NArg() != 1 {
fatalf("no Blueprints file specified")
}

View file

@ -17,6 +17,8 @@ package proptools
import (
"fmt"
"reflect"
"sync"
"sync/atomic"
)
func CloneProperties(structValue reflect.Value) reflect.Value {
@ -32,8 +34,7 @@ func CopyProperties(dstValue, srcValue reflect.Value) {
dstValue.Kind(), srcValue.Kind()))
}
for i := 0; i < srcValue.NumField(); i++ {
field := typ.Field(i)
for i, field := range typeFields(typ) {
if field.PkgPath != "" {
// The field is not exported so just skip it.
continue
@ -42,6 +43,7 @@ func CopyProperties(dstValue, srcValue reflect.Value) {
srcFieldValue := srcValue.Field(i)
dstFieldValue := dstValue.Field(i)
dstFieldInterfaceValue := reflect.Value{}
origDstFieldValue := dstFieldValue
switch srcFieldValue.Kind() {
case reflect.Bool, reflect.String, reflect.Int, reflect.Uint:
@ -92,7 +94,7 @@ func CopyProperties(dstValue, srcValue reflect.Value) {
fallthrough
case reflect.Ptr:
if srcFieldValue.IsNil() {
dstFieldValue.Set(srcFieldValue)
origDstFieldValue.Set(srcFieldValue)
break
}
@ -109,13 +111,13 @@ func CopyProperties(dstValue, srcValue reflect.Value) {
if dstFieldInterfaceValue.IsValid() {
dstFieldInterfaceValue.Set(newValue)
} else {
dstFieldValue.Set(newValue)
origDstFieldValue.Set(newValue)
}
}
case reflect.Bool, reflect.String:
newValue := reflect.New(srcFieldValue.Type())
newValue.Elem().Set(srcFieldValue)
dstFieldValue.Set(newValue)
origDstFieldValue.Set(newValue)
default:
panic(fmt.Errorf("can't clone field %q: points to a %s",
field.Name, srcFieldValue.Kind()))
@ -130,8 +132,7 @@ func CopyProperties(dstValue, srcValue reflect.Value) {
func ZeroProperties(structValue reflect.Value) {
typ := structValue.Type()
for i := 0; i < structValue.NumField(); i++ {
field := typ.Field(i)
for i, field := range typeFields(typ) {
if field.PkgPath != "" {
// The field is not exported so just skip it.
continue
@ -189,8 +190,7 @@ func CloneEmptyProperties(structValue reflect.Value) reflect.Value {
func cloneEmptyProperties(dstValue, srcValue reflect.Value) {
typ := srcValue.Type()
for i := 0; i < srcValue.NumField(); i++ {
field := typ.Field(i)
for i, field := range typeFields(typ) {
if field.PkgPath != "" {
// The field is not exported so just skip it.
continue
@ -250,3 +250,50 @@ func cloneEmptyProperties(dstValue, srcValue reflect.Value) {
}
}
}
// reflect.Type.Field allocates a []int{} to hold the index every time it is called, which ends up
// being a significant portion of the GC pressure. It can't reuse the same one in case a caller
// modifies the backing array through the slice. Since we don't modify it, cache the result
// locally to reduce allocations.
type typeFieldMap map[reflect.Type][]reflect.StructField
var (
// Stores an atomic pointer to map caching Type to its StructField
typeFieldCache atomic.Value
// Lock used by slow path updating the cache pointer
typeFieldCacheWriterLock sync.Mutex
)
func init() {
typeFieldCache.Store(make(typeFieldMap))
}
func typeFields(typ reflect.Type) []reflect.StructField {
// Fast path
cache := typeFieldCache.Load().(typeFieldMap)
if typeFields, ok := cache[typ]; ok {
return typeFields
}
// Slow path
typeFields := make([]reflect.StructField, typ.NumField())
for i := range typeFields {
typeFields[i] = typ.Field(i)
}
typeFieldCacheWriterLock.Lock()
defer typeFieldCacheWriterLock.Unlock()
old := typeFieldCache.Load().(typeFieldMap)
cache = make(typeFieldMap)
for k, v := range old {
cache[k] = v
}
cache[typ] = typeFields
typeFieldCache.Store(cache)
return typeFields
}

View file

@ -191,11 +191,15 @@ func extendPropertyErrorf(property string, format string, a ...interface{}) *Ext
func extendProperties(dst interface{}, src interface{}, filter ExtendPropertyFilterFunc,
order ExtendPropertyOrderFunc) error {
dstValue, err := getStruct(dst)
srcValue, err := getStruct(src)
if err != nil {
if _, ok := err.(getStructEmptyError); ok {
return nil
}
return err
}
srcValue, err := getStruct(src)
dstValue, err := getOrCreateStruct(dst)
if err != nil {
return err
}
@ -212,20 +216,23 @@ func extendProperties(dst interface{}, src interface{}, filter ExtendPropertyFil
func extendMatchingProperties(dst []interface{}, src interface{}, filter ExtendPropertyFilterFunc,
order ExtendPropertyOrderFunc) error {
srcValue, err := getStruct(src)
if err != nil {
if _, ok := err.(getStructEmptyError); ok {
return nil
}
return err
}
dstValues := make([]reflect.Value, len(dst))
for i := range dst {
var err error
dstValues[i], err = getStruct(dst[i])
dstValues[i], err = getOrCreateStruct(dst[i])
if err != nil {
return err
}
}
srcValue, err := getStruct(src)
if err != nil {
return err
}
return extendPropertiesRecursive(dstValues, srcValue, "", filter, false, order)
}
@ -234,8 +241,7 @@ func extendPropertiesRecursive(dstValues []reflect.Value, srcValue reflect.Value
order ExtendPropertyOrderFunc) error {
srcType := srcValue.Type()
for i := 0; i < srcValue.NumField(); i++ {
srcField := srcType.Field(i)
for i, srcField := range typeFields(srcType) {
if srcField.PkgPath != "" {
// The field is not exported so just skip it.
continue
@ -252,11 +258,17 @@ func extendPropertiesRecursive(dstValues []reflect.Value, srcValue reflect.Value
dstType := dstValue.Type()
var dstField reflect.StructField
dstFields := typeFields(dstType)
if dstType == srcType {
dstField = dstType.Field(i)
dstField = dstFields[i]
} else {
var ok bool
dstField, ok = dstType.FieldByName(srcField.Name)
for _, field := range dstFields {
if field.Name == srcField.Name {
dstField = field
ok = true
}
}
if !ok {
continue
}
@ -265,6 +277,7 @@ func extendPropertiesRecursive(dstValues []reflect.Value, srcValue reflect.Value
found = true
dstFieldValue := dstValue.FieldByIndex(dstField.Index)
origDstFieldValue := dstFieldValue
if srcFieldValue.Kind() != dstFieldValue.Kind() {
return extendPropertyErrorf(propertyName, "mismatched types %s and %s",
@ -301,11 +314,12 @@ func extendPropertiesRecursive(dstValues []reflect.Value, srcValue reflect.Value
}
// Pointer to a struct
if dstFieldValue.IsNil() != srcFieldValue.IsNil() {
return extendPropertyErrorf(propertyName, "nilitude mismatch")
if srcFieldValue.IsNil() {
continue
}
if dstFieldValue.IsNil() {
continue
dstFieldValue = reflect.New(srcFieldValue.Type().Elem())
origDstFieldValue.Set(dstFieldValue)
}
dstFieldValue = dstFieldValue.Elem()
@ -335,9 +349,12 @@ func extendPropertiesRecursive(dstValues []reflect.Value, srcValue reflect.Value
srcFieldValue.Kind())
}
dstFieldInterface := dstFieldValue.Interface()
srcFieldInterface := srcFieldValue.Interface()
if filter != nil {
b, err := filter(propertyName, dstField, srcField,
dstFieldValue.Interface(), srcFieldValue.Interface())
dstFieldInterface, srcFieldInterface)
if err != nil {
return &ExtendPropertyError{
Property: propertyName,
@ -352,7 +369,7 @@ func extendPropertiesRecursive(dstValues []reflect.Value, srcValue reflect.Value
prepend := false
if order != nil {
b, err := order(propertyName, dstField, srcField,
dstFieldValue.Interface(), srcFieldValue.Interface())
dstFieldInterface, srcFieldInterface)
if err != nil {
return &ExtendPropertyError{
Property: propertyName,
@ -427,14 +444,32 @@ func extendPropertiesRecursive(dstValues []reflect.Value, srcValue reflect.Value
return nil
}
type getStructEmptyError struct{}
func (getStructEmptyError) Error() string { return "interface containing nil pointer" }
func getOrCreateStruct(in interface{}) (reflect.Value, error) {
value, err := getStruct(in)
if _, ok := err.(getStructEmptyError); ok {
value := reflect.ValueOf(in)
newValue := reflect.New(value.Type().Elem())
value.Set(newValue)
}
return value, err
}
func getStruct(in interface{}) (reflect.Value, error) {
value := reflect.ValueOf(in)
if value.Kind() != reflect.Ptr {
return reflect.Value{}, fmt.Errorf("expected pointer to struct, got %T", in)
}
value = value.Elem()
if value.Kind() != reflect.Struct {
if value.Type().Elem().Kind() != reflect.Struct {
return reflect.Value{}, fmt.Errorf("expected pointer to struct, got %T", in)
}
if value.IsNil() {
return reflect.Value{}, getStructEmptyError{}
}
value = value.Elem()
return value, nil
}

View file

@ -496,12 +496,69 @@ func appendPropertiesTestCases() []appendPropertyTestCase {
},
},
},
{
// Nil pointer to a struct
in1: &struct {
Nested *struct {
S string
}
}{},
in2: &struct {
Nested *struct {
S string
}
}{
Nested: &struct {
S string
}{
S: "string",
},
},
out: &struct {
Nested *struct {
S string
}
}{
Nested: &struct {
S string
}{
S: "string",
},
},
},
{
// Nil pointer to a struct in an interface
in1: &struct {
Nested interface{}
}{
Nested: (*struct{ S string })(nil),
},
in2: &struct {
Nested interface{}
}{
Nested: &struct {
S string
}{
S: "string",
},
},
out: &struct {
Nested interface{}
}{
Nested: &struct {
S string
}{
S: "string",
},
},
},
// Errors
{
// Non-pointer in1
in1: struct{}{},
in2: &struct{}{},
err: errors.New("expected pointer to struct, got struct {}"),
out: struct{}{},
},
@ -515,6 +572,7 @@ func appendPropertiesTestCases() []appendPropertyTestCase {
{
// Non-struct in1
in1: &[]string{"bad"},
in2: &struct{}{},
err: errors.New("expected pointer to struct, got *[]string"),
out: &[]string{"bad"},
},
@ -606,23 +664,6 @@ func appendPropertiesTestCases() []appendPropertyTestCase {
},
err: extendPropertyErrorf("s", "interface not a pointer"),
},
{
// Pointer nilitude mismatch
in1: &struct{ S *struct{ S string } }{
S: &struct{ S string }{
S: "string1",
},
},
in2: &struct{ S *struct{ S string } }{
S: nil,
},
out: &struct{ S *struct{ S string } }{
S: &struct{ S string }{
S: "string1",
},
},
err: extendPropertyErrorf("s", "nilitude mismatch"),
},
{
// Pointer not a struct
in1: &struct{ S *[]string }{
@ -912,6 +953,7 @@ func appendMatchingPropertiesTestCases() []appendMatchingPropertiesTestCase {
{
// Non-pointer in1
in1: []interface{}{struct{}{}},
in2: &struct{}{},
err: errors.New("expected pointer to struct, got struct {}"),
out: []interface{}{struct{}{}},
},
@ -925,6 +967,7 @@ func appendMatchingPropertiesTestCases() []appendMatchingPropertiesTestCase {
{
// Non-struct in1
in1: []interface{}{&[]string{"bad"}},
in2: &struct{}{},
err: errors.New("expected pointer to struct, got *[]string"),
out: []interface{}{&[]string{"bad"}},
},

View file

@ -46,12 +46,14 @@ func typeEqual(v1, v2 reflect.Value) bool {
if v1.Type().Elem().Kind() != reflect.Struct {
return true
}
if v1.IsNil() != v2.IsNil() {
return false
}
if v1.IsNil() {
if v1.IsNil() && !v2.IsNil() {
return concreteType(v2)
} else if v2.IsNil() && !v1.IsNil() {
return concreteType(v1)
} else if v1.IsNil() && v2.IsNil() {
return true
}
v1 = v1.Elem()
v2 = v2.Elem()
}
@ -74,3 +76,34 @@ func typeEqual(v1, v2 reflect.Value) bool {
return true
}
// Returns true if v recursively contains no interfaces
func concreteType(v reflect.Value) bool {
if v.Kind() == reflect.Interface {
return false
}
if v.Kind() == reflect.Ptr {
if v.IsNil() {
return true
}
v = v.Elem()
}
if v.Kind() != reflect.Struct {
return true
}
for i := 0; i < v.NumField(); i++ {
v := v.Field(i)
switch kind := v.Kind(); kind {
case reflect.Interface, reflect.Ptr, reflect.Struct:
if !concreteType(v) {
return false
}
}
}
return true
}

View file

@ -88,7 +88,7 @@ var typeEqualTestCases = []struct {
// Mismatching nilitude embedded pointer to struct
in1: &struct{ S *struct{ S1 string } }{S: &struct{ S1 string }{}},
in2: &struct{ S *struct{ S1 string } }{},
out: false,
out: true,
},
{
// Matching embedded interface to pointer to struct
@ -116,13 +116,13 @@ var typeEqualTestCases = []struct {
},
{
// Matching pointer to non-struct
in1: struct{ S1 *string }{ S1: StringPtr("test1") },
in2: struct{ S1 *string }{ S1: StringPtr("test2") },
in1: struct{ S1 *string }{S1: StringPtr("test1")},
in2: struct{ S1 *string }{S1: StringPtr("test2")},
out: true,
},
{
// Matching nilitude pointer to non-struct
in1: struct{ S1 *string }{ S1: StringPtr("test1") },
in1: struct{ S1 *string }{S1: StringPtr("test1")},
in2: struct{ S1 *string }{},
out: true,
},

View file

@ -139,6 +139,8 @@ func unpackStructValue(namePrefix string, structValue reflect.Value,
panic(fmt.Errorf("field %s is not settable", propertyName))
}
origFieldValue := fieldValue
// To make testing easier we validate the struct field's type regardless
// of whether or not the property was specified in the parsed string.
switch kind := fieldValue.Kind(); kind {
@ -163,7 +165,8 @@ func unpackStructValue(namePrefix string, structValue reflect.Value,
switch ptrKind := fieldValue.Type().Elem().Kind(); ptrKind {
case reflect.Struct:
if fieldValue.IsNil() {
panic(fmt.Errorf("field %s contains a nil pointer", propertyName))
fieldValue = reflect.New(fieldValue.Type().Elem())
origFieldValue.Set(fieldValue)
}
fieldValue = fieldValue.Elem()
case reflect.Bool, reflect.String:

View file

@ -28,15 +28,17 @@ import (
var validUnpackTestCases = []struct {
input string
output []interface{}
empty []interface{}
errs []error
}{
{`
m {
name: "abc",
blank: "",
}
{
input: `
m {
name: "abc",
blank: "",
}
`,
[]interface{}{
output: []interface{}{
struct {
Name *string
Blank *string
@ -47,46 +49,46 @@ var validUnpackTestCases = []struct {
Unset: nil,
},
},
nil,
},
{`
m {
name: "abc",
}
{
input: `
m {
name: "abc",
}
`,
[]interface{}{
output: []interface{}{
struct {
Name string
}{
Name: "abc",
},
},
nil,
},
{`
m {
isGood: true,
}
{
input: `
m {
isGood: true,
}
`,
[]interface{}{
output: []interface{}{
struct {
IsGood bool
}{
IsGood: true,
},
},
nil,
},
{`
m {
isGood: true,
isBad: false,
}
{
input: `
m {
isGood: true,
isBad: false,
}
`,
[]interface{}{
output: []interface{}{
struct {
IsGood *bool
IsBad *bool
@ -97,17 +99,17 @@ var validUnpackTestCases = []struct {
IsUgly: nil,
},
},
nil,
},
{`
m {
stuff: ["asdf", "jkl;", "qwert",
"uiop", "bnm,"],
empty: []
}
{
input: `
m {
stuff: ["asdf", "jkl;", "qwert",
"uiop", "bnm,"],
empty: []
}
`,
[]interface{}{
output: []interface{}{
struct {
Stuff []string
Empty []string
@ -118,17 +120,17 @@ var validUnpackTestCases = []struct {
Nil: nil,
},
},
nil,
},
{`
m {
nested: {
name: "abc",
{
input: `
m {
nested: {
name: "abc",
}
}
}
`,
[]interface{}{
output: []interface{}{
struct {
Nested struct {
Name string
@ -139,17 +141,17 @@ var validUnpackTestCases = []struct {
},
},
},
nil,
},
{`
m {
nested: {
name: "def",
{
input: `
m {
nested: {
name: "def",
}
}
}
`,
[]interface{}{
output: []interface{}{
struct {
Nested interface{}
}{
@ -158,19 +160,19 @@ var validUnpackTestCases = []struct {
},
},
},
nil,
},
{`
m {
nested: {
foo: "abc",
},
bar: false,
baz: ["def", "ghi"],
}
{
input: `
m {
nested: {
foo: "abc",
},
bar: false,
baz: ["def", "ghi"],
}
`,
[]interface{}{
output: []interface{}{
struct {
Nested struct {
Foo string
@ -185,19 +187,19 @@ var validUnpackTestCases = []struct {
Baz: []string{"def", "ghi"},
},
},
nil,
},
{`
m {
nested: {
foo: "abc",
},
bar: false,
baz: ["def", "ghi"],
}
{
input: `
m {
nested: {
foo: "abc",
},
bar: false,
baz: ["def", "ghi"],
}
`,
[]interface{}{
output: []interface{}{
struct {
Nested struct {
Foo string `allowNested:"true"`
@ -214,19 +216,19 @@ var validUnpackTestCases = []struct {
Baz: []string{"def", "ghi"},
},
},
nil,
},
{`
m {
nested: {
foo: "abc",
},
bar: false,
baz: ["def", "ghi"],
}
{
input: `
m {
nested: {
foo: "abc",
},
bar: false,
baz: ["def", "ghi"],
}
`,
[]interface{}{
output: []interface{}{
struct {
Nested struct {
Foo string
@ -241,24 +243,25 @@ var validUnpackTestCases = []struct {
Baz: []string{"def", "ghi"},
},
},
[]error{
errs: []error{
&Error{
Err: fmt.Errorf("filtered field nested.foo cannot be set in a Blueprint file"),
Pos: mkpos(27, 4, 8),
Pos: mkpos(30, 4, 9),
},
},
},
// Anonymous struct
{`
m {
name: "abc",
nested: {
name: "def",
},
}
{
input: `
m {
name: "abc",
nested: {
name: "def",
},
}
`,
[]interface{}{
output: []interface{}{
struct {
EmbeddedStruct
Nested struct {
@ -277,19 +280,19 @@ var validUnpackTestCases = []struct {
},
},
},
nil,
},
// Anonymous interface
{`
m {
name: "abc",
nested: {
name: "def",
},
}
{
input: `
m {
name: "abc",
nested: {
name: "def",
},
}
`,
[]interface{}{
output: []interface{}{
struct {
EmbeddedInterface
Nested struct {
@ -308,19 +311,19 @@ var validUnpackTestCases = []struct {
},
},
},
nil,
},
// Anonymous struct with name collision
{`
m {
name: "abc",
nested: {
name: "def",
},
}
{
input: `
m {
name: "abc",
nested: {
name: "def",
},
}
`,
[]interface{}{
output: []interface{}{
struct {
Name string
EmbeddedStruct
@ -344,19 +347,19 @@ var validUnpackTestCases = []struct {
},
},
},
nil,
},
// Anonymous interface with name collision
{`
m {
name: "abc",
nested: {
name: "def",
},
}
{
input: `
m {
name: "abc",
nested: {
name: "def",
},
}
`,
[]interface{}{
output: []interface{}{
struct {
Name string
EmbeddedInterface
@ -380,21 +383,21 @@ var validUnpackTestCases = []struct {
},
},
},
nil,
},
// Variables
{`
list = ["abc"]
string = "def"
list_with_variable = [string]
m {
name: string,
list: list,
list2: list_with_variable,
}
{
input: `
list = ["abc"]
string = "def"
list_with_variable = [string]
m {
name: string,
list: list,
list2: list_with_variable,
}
`,
[]interface{}{
output: []interface{}{
struct {
Name string
List []string
@ -405,18 +408,18 @@ var validUnpackTestCases = []struct {
List2: []string{"def"},
},
},
nil,
},
// Multiple property structs
{`
m {
nested: {
name: "abc",
{
input: `
m {
nested: {
name: "abc",
}
}
}
`,
[]interface{}{
output: []interface{}{
struct {
Nested struct {
Name string
@ -438,7 +441,62 @@ var validUnpackTestCases = []struct {
struct {
}{},
},
nil,
},
// Nil pointer to struct
{
input: `
m {
nested: {
name: "abc",
}
}
`,
output: []interface{}{
struct {
Nested *struct {
Name string
}
}{
Nested: &struct{ Name string }{
Name: "abc",
},
},
},
empty: []interface{}{
&struct {
Nested *struct {
Name string
}
}{},
},
},
// Interface containing nil pointer to struct
{
input: `
m {
nested: {
name: "abc",
}
}
`,
output: []interface{}{
struct {
Nested interface{}
}{
Nested: &EmbeddedStruct{
Name: "abc",
},
},
},
empty: []interface{}{
&struct {
Nested interface{}
}{
Nested: (*EmbeddedStruct)(nil),
},
},
},
}
@ -464,9 +522,13 @@ func TestUnpackProperties(t *testing.T) {
continue
}
output := []interface{}{}
for _, p := range testCase.output {
output = append(output, proptools.CloneEmptyProperties(reflect.ValueOf(p)).Interface())
var output []interface{}
if len(testCase.empty) > 0 {
output = testCase.empty
} else {
for _, p := range testCase.output {
output = append(output, proptools.CloneEmptyProperties(reflect.ValueOf(p)).Interface())
}
}
_, errs = unpackProperties(module.Properties, output...)
if len(errs) != 0 && len(testCase.errs) == 0 {