From c4fc4b447f034045022c77a9a8e3b091a6b70446 Mon Sep 17 00:00:00 2001 From: Colin Cross Date: Thu, 1 Feb 2024 14:37:14 -0800 Subject: [PATCH] Optimize (Extend|Append|Prepend)[Matching]Properties The property squashing functions were performing 19% of all allocations and allocating 3.43 GB, most of which was used to track the name of the property, but the name of the property is only used to print errors. Keep the elements of the name in a preallocated slice instead, and only compute the name when an error occurs. Calling reflect.Value.Interface() was also expensive, and only passed to filter and order functions, none of which use the values. Modify the signature of the filter and order functions to remove the interfaces and the property name. Test: extend_test.go Change-Id: I517f89daf251bb43f7cfefa6f1e83951c0e271b7 --- proptools/extend.go | 74 ++++++++++++++++++---------------------- proptools/extend_test.go | 28 ++++----------- 2 files changed, 41 insertions(+), 61 deletions(-) diff --git a/proptools/extend.go b/proptools/extend.go index 4e2f498..63ff1d7 100644 --- a/proptools/extend.go +++ b/proptools/extend.go @@ -17,6 +17,7 @@ package proptools import ( "fmt" "reflect" + "strings" ) // AppendProperties appends the values of properties in the property struct src to the property @@ -157,29 +158,19 @@ const ( Replace ) -type ExtendPropertyFilterFunc func(property string, - dstField, srcField reflect.StructField, - dstValue, srcValue interface{}) (bool, error) +type ExtendPropertyFilterFunc func(dstField, srcField reflect.StructField) (bool, error) -type ExtendPropertyOrderFunc func(property string, - dstField, srcField reflect.StructField, - dstValue, srcValue interface{}) (Order, error) +type ExtendPropertyOrderFunc func(dstField, srcField reflect.StructField) (Order, error) -func OrderAppend(property string, - dstField, srcField reflect.StructField, - dstValue, srcValue interface{}) (Order, error) { +func OrderAppend(dstField, srcField reflect.StructField) (Order, error) { return Append, nil } -func OrderPrepend(property string, - dstField, srcField reflect.StructField, - dstValue, srcValue interface{}) (Order, error) { +func OrderPrepend(dstField, srcField reflect.StructField) (Order, error) { return Prepend, nil } -func OrderReplace(property string, - dstField, srcField reflect.StructField, - dstValue, srcValue interface{}) (Order, error) { +func OrderReplace(dstField, srcField reflect.StructField) (Order, error) { return Replace, nil } @@ -221,7 +212,7 @@ func extendProperties(dst interface{}, src interface{}, filter ExtendPropertyFil dstValues := []reflect.Value{dstValue} - return extendPropertiesRecursive(dstValues, srcValue, "", filter, true, order) + return extendPropertiesRecursive(dstValues, srcValue, make([]string, 0, 8), filter, true, order) } func extendMatchingProperties(dst []interface{}, src interface{}, filter ExtendPropertyFilterFunc, @@ -244,22 +235,30 @@ func extendMatchingProperties(dst []interface{}, src interface{}, filter ExtendP } } - return extendPropertiesRecursive(dstValues, srcValue, "", filter, false, order) + return extendPropertiesRecursive(dstValues, srcValue, make([]string, 0, 8), filter, false, order) } func extendPropertiesRecursive(dstValues []reflect.Value, srcValue reflect.Value, - prefix string, filter ExtendPropertyFilterFunc, sameTypes bool, + prefix []string, filter ExtendPropertyFilterFunc, sameTypes bool, orderFunc ExtendPropertyOrderFunc) error { dstValuesCopied := false + propertyName := func(field reflect.StructField) string { + names := make([]string, 0, len(prefix)+1) + for _, s := range prefix { + names = append(names, PropertyNameForField(s)) + } + names = append(names, PropertyNameForField(field.Name)) + return strings.Join(names, ".") + } + srcType := srcValue.Type() for i, srcField := range typeFields(srcType) { if ShouldSkipProperty(srcField) { continue } - propertyName := prefix + PropertyNameForField(srcField.Name) srcFieldValue := srcValue.Field(i) // Step into source interfaces @@ -271,7 +270,7 @@ func extendPropertiesRecursive(dstValues []reflect.Value, srcValue reflect.Value srcFieldValue = srcFieldValue.Elem() if srcFieldValue.Kind() != reflect.Ptr { - return extendPropertyErrorf(propertyName, "interface not a pointer") + return extendPropertyErrorf(propertyName(srcField), "interface not a pointer") } } @@ -311,8 +310,8 @@ func extendPropertiesRecursive(dstValues []reflect.Value, srcValue reflect.Value embeddedDstValue = embeddedDstValue.Elem() } if !isStruct(embeddedDstValue.Type()) { - return extendPropertyErrorf(propertyName, "%s is not a struct (%s)", - prefix+field.Name, embeddedDstValue.Type()) + return extendPropertyErrorf(propertyName(srcField), "%s is not a struct (%s)", + propertyName(field), embeddedDstValue.Type()) } // The destination struct contains an embedded struct, add it to the list // of destinations to consider. Make a copy of dstValues if necessary @@ -337,13 +336,13 @@ func extendPropertiesRecursive(dstValues []reflect.Value, srcValue reflect.Value // Step into destination interfaces if dstFieldValue.Kind() == reflect.Interface { if dstFieldValue.IsNil() { - return extendPropertyErrorf(propertyName, "nilitude mismatch") + return extendPropertyErrorf(propertyName(srcField), "nilitude mismatch") } dstFieldValue = dstFieldValue.Elem() if dstFieldValue.Kind() != reflect.Ptr { - return extendPropertyErrorf(propertyName, "interface not a pointer") + return extendPropertyErrorf(propertyName(srcField), "interface not a pointer") } } @@ -360,7 +359,7 @@ func extendPropertiesRecursive(dstValues []reflect.Value, srcValue reflect.Value switch srcFieldValue.Kind() { case reflect.Struct: if sameTypes && dstFieldValue.Type() != srcFieldValue.Type() { - return extendPropertyErrorf(propertyName, "mismatched types %s and %s", + return extendPropertyErrorf(propertyName(srcField), "mismatched types %s and %s", dstFieldValue.Type(), srcFieldValue.Type()) } @@ -369,34 +368,30 @@ func extendPropertiesRecursive(dstValues []reflect.Value, srcValue reflect.Value continue case reflect.Bool, reflect.String, reflect.Slice, reflect.Map: if srcFieldValue.Type() != dstFieldValue.Type() { - return extendPropertyErrorf(propertyName, "mismatched types %s and %s", + return extendPropertyErrorf(propertyName(srcField), "mismatched types %s and %s", dstFieldValue.Type(), srcFieldValue.Type()) } case reflect.Ptr: if srcFieldValue.Type() != dstFieldValue.Type() { - return extendPropertyErrorf(propertyName, "mismatched types %s and %s", + return extendPropertyErrorf(propertyName(srcField), "mismatched types %s and %s", dstFieldValue.Type(), srcFieldValue.Type()) } switch ptrKind := srcFieldValue.Type().Elem().Kind(); ptrKind { case reflect.Bool, reflect.Int64, reflect.String, reflect.Struct: // Nothing default: - return extendPropertyErrorf(propertyName, "pointer is a %s", ptrKind) + return extendPropertyErrorf(propertyName(srcField), "pointer is a %s", ptrKind) } default: - return extendPropertyErrorf(propertyName, "unsupported kind %s", + return extendPropertyErrorf(propertyName(srcField), "unsupported kind %s", srcFieldValue.Kind()) } - dstFieldInterface := dstFieldValue.Interface() - srcFieldInterface := srcFieldValue.Interface() - if filter != nil { - b, err := filter(propertyName, dstField, srcField, - dstFieldInterface, srcFieldInterface) + b, err := filter(dstField, srcField) if err != nil { return &ExtendPropertyError{ - Property: propertyName, + Property: propertyName(srcField), Err: err, } } @@ -408,11 +403,10 @@ func extendPropertiesRecursive(dstValues []reflect.Value, srcValue reflect.Value order := Append if orderFunc != nil { var err error - order, err = orderFunc(propertyName, dstField, srcField, - dstFieldInterface, srcFieldInterface) + order, err = orderFunc(dstField, srcField) if err != nil { return &ExtendPropertyError{ - Property: propertyName, + Property: propertyName(srcField), Err: err, } } @@ -423,12 +417,12 @@ func extendPropertiesRecursive(dstValues []reflect.Value, srcValue reflect.Value if len(recurse) > 0 { err := extendPropertiesRecursive(recurse, srcFieldValue, - propertyName+".", filter, sameTypes, orderFunc) + append(prefix, srcField.Name), filter, sameTypes, orderFunc) if err != nil { return err } } else if !found { - return extendPropertyErrorf(propertyName, "failed to find property to extend") + return extendPropertyErrorf(propertyName(srcField), "failed to find property to extend") } } diff --git a/proptools/extend_test.go b/proptools/extend_test.go index d2dac72..e562776 100644 --- a/proptools/extend_test.go +++ b/proptools/extend_test.go @@ -1168,9 +1168,7 @@ func appendPropertiesTestCases() []appendPropertyTestCase { out: &struct{ S string }{ S: "string1string2", }, - filter: func(property string, - dstField, srcField reflect.StructField, - dstValue, srcValue interface{}) (bool, error) { + filter: func(dstField, srcField reflect.StructField) (bool, error) { return true, nil }, }, @@ -1185,9 +1183,7 @@ func appendPropertiesTestCases() []appendPropertyTestCase { out: &struct{ S string }{ S: "string1", }, - filter: func(property string, - dstField, srcField reflect.StructField, - dstValue, srcValue interface{}) (bool, error) { + filter: func(dstField, srcField reflect.StructField) (bool, error) { return false, nil }, }, @@ -1202,12 +1198,8 @@ func appendPropertiesTestCases() []appendPropertyTestCase { out: &struct{ S string }{ S: "string1string2", }, - filter: func(property string, - dstField, srcField reflect.StructField, - dstValue, srcValue interface{}) (bool, error) { - return property == "s" && - dstField.Name == "S" && srcField.Name == "S" && - dstValue.(string) == "string1" && srcValue.(string) == "string2", nil + filter: func(dstField, srcField reflect.StructField) (bool, error) { + return dstField.Name == "S" && srcField.Name == "S", nil }, }, { @@ -1257,9 +1249,7 @@ func appendPropertiesTestCases() []appendPropertyTestCase { out: &struct{ S string }{ S: "string1", }, - filter: func(property string, - dstField, srcField reflect.StructField, - dstValue, srcValue interface{}) (bool, error) { + filter: func(dstField, srcField reflect.StructField) (bool, error) { return true, fmt.Errorf("filter error") }, err: extendPropertyErrorf("s", "filter error"), @@ -1300,9 +1290,7 @@ func TestExtendProperties(t *testing.T) { var err error var testType string - order := func(property string, - dstField, srcField reflect.StructField, - dstValue, srcValue interface{}) (Order, error) { + order := func(dstField, srcField reflect.StructField) (Order, error) { switch testCase.order { case Append: return Append, nil @@ -1764,9 +1752,7 @@ func TestExtendMatchingProperties(t *testing.T) { var err error var testType string - order := func(property string, - dstField, srcField reflect.StructField, - dstValue, srcValue interface{}) (Order, error) { + order := func(dstField, srcField reflect.StructField) (Order, error) { switch testCase.order { case Append: return Append, nil