From bd0e4edd4fb57d0fe0c2a53559c6177be0d548f0 Mon Sep 17 00:00:00 2001 From: Clayton Coleman Date: Tue, 3 Jul 2018 12:13:09 -0400 Subject: [PATCH] Add a new conversion path to replace GenericConversionFunc reflect.Call is very expensive. We currently use a switch block as part of AddGenericConversionFunc to avoid the bulk of top level a->b conversion for our primary types. Instead of having these be handwritten, we should generate them. The pattern for generating them looks like: ``` scheme.AddConversionFunc(&v1.Type{}, &internal.Type{}, func(a, b interface{}, scope conversion.Scope) error { return Convert_v1_Type_to_internal_Type(a.(*v1.Type), b.(*internal.Type), scope) }) ``` which matches AddDefaultObjectFunc (which proved out the approach). The conversion machinery would then do a simple map lookup and invoke the function. This bypasses reflect.Call and in the future allows Golang mid-stack inlining to optimize this code. As a future step we can drop support for the reflection path and simply return a nice error "you must write a generator for your type". --- .../apimachinery/pkg/conversion/converter.go | 112 +++++++++++------- .../pkg/conversion/converter_test.go | 92 -------------- .../k8s.io/apimachinery/pkg/runtime/scheme.go | 23 ++-- 3 files changed, 82 insertions(+), 145 deletions(-) diff --git a/staging/src/k8s.io/apimachinery/pkg/conversion/converter.go b/staging/src/k8s.io/apimachinery/pkg/conversion/converter.go index 7854c207c72..0c384de4847 100644 --- a/staging/src/k8s.io/apimachinery/pkg/conversion/converter.go +++ b/staging/src/k8s.io/apimachinery/pkg/conversion/converter.go @@ -40,7 +40,11 @@ type NameFunc func(t reflect.Type) string var DefaultNameFunc = func(t reflect.Type) string { return t.Name() } -type GenericConversionFunc func(a, b interface{}, scope Scope) (bool, error) +// ConversionFunc converts the object a into the object b, reusing arrays or objects +// or pointers if necessary. It should return an error if the object cannot be converted +// or if some data is invalid. If you do not wish a and b to share fields or nested +// objects, you must copy a before calling this function. +type ConversionFunc func(a, b interface{}, scope Scope) error // Converter knows how to convert one type to another. type Converter struct { @@ -161,11 +165,15 @@ type Scope interface { type FieldMappingFunc func(key string, sourceTag, destTag reflect.StructTag) (source string, dest string) func NewConversionFuncs() ConversionFuncs { - return ConversionFuncs{fns: make(map[typePair]reflect.Value)} + return ConversionFuncs{ + fns: make(map[typePair]reflect.Value), + untyped: make(map[typePair]ConversionFunc), + } } type ConversionFuncs struct { - fns map[typePair]reflect.Value + fns map[typePair]reflect.Value + untyped map[typePair]ConversionFunc } // Add adds the provided conversion functions to the lookup table - they must have the signature @@ -183,6 +191,21 @@ func (c ConversionFuncs) Add(fns ...interface{}) error { return nil } +// AddUntyped adds the provided conversion function to the lookup table for the types that are +// supplied as a and b. a and b must be pointers or an error is returned. This method overwrites +// previously defined functions. +func (c ConversionFuncs) AddUntyped(a, b interface{}, fn ConversionFunc) error { + tA, tB := reflect.TypeOf(a), reflect.TypeOf(b) + if tA.Kind() != reflect.Ptr { + return fmt.Errorf("the type %T must be a pointer to register as an untyped conversion", a) + } + if tB.Kind() != reflect.Ptr { + return fmt.Errorf("the type %T must be a pointer to register as an untyped conversion", b) + } + c.untyped[typePair{tA, tB}] = fn + return nil +} + // Merge returns a new ConversionFuncs that contains all conversions from // both other and c, with other conversions taking precedence. func (c ConversionFuncs) Merge(other ConversionFuncs) ConversionFuncs { @@ -193,6 +216,12 @@ func (c ConversionFuncs) Merge(other ConversionFuncs) ConversionFuncs { for k, v := range other.fns { merged.fns[k] = v } + for k, v := range c.untyped { + merged.untyped[k] = v + } + for k, v := range other.untyped { + merged.untyped[k] = v + } return merged } @@ -355,16 +384,32 @@ func verifyConversionFunctionSignature(ft reflect.Type) error { // // conversion logic... // return nil // }) +// DEPRECATED: Will be removed in favor of RegisterUntypedConversionFunc func (c *Converter) RegisterConversionFunc(conversionFunc interface{}) error { return c.conversionFuncs.Add(conversionFunc) } // Similar to RegisterConversionFunc, but registers conversion function that were // automatically generated. +// DEPRECATED: Will be removed in favor of RegisterGeneratedUntypedConversionFunc func (c *Converter) RegisterGeneratedConversionFunc(conversionFunc interface{}) error { return c.generatedConversionFuncs.Add(conversionFunc) } +// RegisterUntypedConversionFunc registers a function that converts between a and b by passing objects of those +// types to the provided function. The function *must* accept objects of a and b - this machinery will not enforce +// any other guarantee. +func (c *Converter) RegisterUntypedConversionFunc(a, b interface{}, fn ConversionFunc) error { + return c.conversionFuncs.AddUntyped(a, b, fn) +} + +// RegisterGeneratedUntypedConversionFunc registers a function that converts between a and b by passing objects of those +// types to the provided function. The function *must* accept objects of a and b - this machinery will not enforce +// any other guarantee. +func (c *Converter) RegisterGeneratedUntypedConversionFunc(a, b interface{}, fn ConversionFunc) error { + return c.generatedConversionFuncs.AddUntyped(a, b, fn) +} + // RegisterIgnoredConversion registers a "no-op" for conversion, where any requested // conversion between from and to is ignored. func (c *Converter) RegisterIgnoredConversion(from, to interface{}) error { @@ -380,39 +425,6 @@ func (c *Converter) RegisterIgnoredConversion(from, to interface{}) error { return nil } -// IsConversionIgnored returns true if the specified objects should be dropped during -// conversion. -func (c *Converter) IsConversionIgnored(inType, outType reflect.Type) bool { - _, found := c.ignoredConversions[typePair{inType, outType}] - return found -} - -func (c *Converter) HasConversionFunc(inType, outType reflect.Type) bool { - _, found := c.conversionFuncs.fns[typePair{inType, outType}] - return found -} - -func (c *Converter) ConversionFuncValue(inType, outType reflect.Type) (reflect.Value, bool) { - value, found := c.conversionFuncs.fns[typePair{inType, outType}] - return value, found -} - -// SetStructFieldCopy registers a correspondence. Whenever a struct field is encountered -// which has a type and name matching srcFieldType and srcFieldName, it wil be copied -// into the field in the destination struct matching destFieldType & Name, if such a -// field exists. -// May be called multiple times, even for the same source field & type--all applicable -// copies will be performed. -func (c *Converter) SetStructFieldCopy(srcFieldType interface{}, srcFieldName string, destFieldType interface{}, destFieldName string) error { - st := reflect.TypeOf(srcFieldType) - dt := reflect.TypeOf(destFieldType) - srcKey := typeNamePair{st, srcFieldName} - destKey := typeNamePair{dt, destFieldName} - c.structFieldDests[srcKey] = append(c.structFieldDests[srcKey], destKey) - c.structFieldSources[destKey] = append(c.structFieldSources[destKey], srcKey) - return nil -} - // RegisterInputDefaults registers a field name mapping function, used when converting // from maps to structs. Inputs to the conversion methods are checked for this type and a mapping // applied automatically if the input matches in. A set of default flags for the input conversion @@ -468,8 +480,8 @@ func (f FieldMatchingFlags) IsSet(flag FieldMatchingFlags) bool { // it is not used by Convert() other than storing it in the scope. // Not safe for objects with cyclic references! func (c *Converter) Convert(src, dest interface{}, flags FieldMatchingFlags, meta *Meta) error { + // TODO: deprecated, will be removed in favor of untyped conversion if len(c.genericConversions) > 0 { - // TODO: avoid scope allocation s := &scope{converter: c, flags: flags, meta: meta} for _, fn := range c.genericConversions { if ok, err := fn(src, dest, s); ok { @@ -495,6 +507,21 @@ func (c *Converter) DefaultConvert(src, dest interface{}, flags FieldMatchingFla type conversionFunc func(sv, dv reflect.Value, scope *scope) error func (c *Converter) doConversion(src, dest interface{}, flags FieldMatchingFlags, meta *Meta, f conversionFunc) error { + pair := typePair{reflect.TypeOf(src), reflect.TypeOf(dest)} + scope := &scope{ + converter: c, + flags: flags, + meta: meta, + } + if fn, ok := c.conversionFuncs.untyped[pair]; ok { + return fn(src, dest, scope) + } + if fn, ok := c.generatedConversionFuncs.untyped[pair]; ok { + return fn(src, dest, scope) + } + // TODO: consider everything past this point deprecated - we want to support only point to point top level + // conversions + dv, err := EnforcePtr(dest) if err != nil { return err @@ -506,15 +533,10 @@ func (c *Converter) doConversion(src, dest interface{}, flags FieldMatchingFlags if err != nil { return err } - s := &scope{ - converter: c, - flags: flags, - meta: meta, - } // Leave something on the stack, so that calls to struct tag getters never fail. - s.srcStack.push(scopeStackElem{}) - s.destStack.push(scopeStackElem{}) - return f(sv, dv, s) + scope.srcStack.push(scopeStackElem{}) + scope.destStack.push(scopeStackElem{}) + return f(sv, dv, scope) } // callCustom calls 'custom' with sv & dv. custom must be a conversion function. diff --git a/staging/src/k8s.io/apimachinery/pkg/conversion/converter_test.go b/staging/src/k8s.io/apimachinery/pkg/conversion/converter_test.go index 5373c80983a..924f699135a 100644 --- a/staging/src/k8s.io/apimachinery/pkg/conversion/converter_test.go +++ b/staging/src/k8s.io/apimachinery/pkg/conversion/converter_test.go @@ -732,95 +732,3 @@ func TestConverter_flags(t *testing.T) { } } } - -func TestConverter_FieldRename(t *testing.T) { - type WeirdMeta struct { - Name string - Type string - } - type NameMeta struct { - Name string - } - type TypeMeta struct { - Type string - } - type A struct { - WeirdMeta - } - type B struct { - TypeMeta - NameMeta - } - - c := NewConverter(DefaultNameFunc) - err := c.SetStructFieldCopy(WeirdMeta{}, "WeirdMeta", TypeMeta{}, "TypeMeta") - if err != nil { - t.Fatalf("unexpected error %v", err) - } - err = c.SetStructFieldCopy(WeirdMeta{}, "WeirdMeta", NameMeta{}, "NameMeta") - if err != nil { - t.Fatalf("unexpected error %v", err) - } - err = c.SetStructFieldCopy(TypeMeta{}, "TypeMeta", WeirdMeta{}, "WeirdMeta") - if err != nil { - t.Fatalf("unexpected error %v", err) - } - err = c.SetStructFieldCopy(NameMeta{}, "NameMeta", WeirdMeta{}, "WeirdMeta") - if err != nil { - t.Fatalf("unexpected error %v", err) - } - c.Debug = testLogger(t) - - aVal := &A{ - WeirdMeta: WeirdMeta{ - Name: "Foo", - Type: "Bar", - }, - } - - bVal := &B{ - TypeMeta: TypeMeta{"Bar"}, - NameMeta: NameMeta{"Foo"}, - } - - table := map[string]struct { - from, to, expect interface{} - flags FieldMatchingFlags - }{ - "to": { - aVal, - &B{}, - bVal, - AllowDifferentFieldTypeNames | SourceToDest | IgnoreMissingFields, - }, - "from": { - bVal, - &A{}, - aVal, - AllowDifferentFieldTypeNames | SourceToDest, - }, - "toDestFirst": { - aVal, - &B{}, - bVal, - AllowDifferentFieldTypeNames, - }, - "fromDestFirst": { - bVal, - &A{}, - aVal, - AllowDifferentFieldTypeNames | IgnoreMissingFields, - }, - } - - for name, item := range table { - err := c.Convert(item.from, item.to, item.flags, nil) - if err != nil { - t.Errorf("%v: unexpected error: %v", name, err) - continue - } - if e, a := item.expect, item.to; !reflect.DeepEqual(e, a) { - t.Errorf("%v: unexpected diff: %v", name, diff.ObjectDiff(e, a)) - } - } -} diff --git a/staging/src/k8s.io/apimachinery/pkg/runtime/scheme.go b/staging/src/k8s.io/apimachinery/pkg/runtime/scheme.go index 66431e75937..d188588163e 100644 --- a/staging/src/k8s.io/apimachinery/pkg/runtime/scheme.go +++ b/staging/src/k8s.io/apimachinery/pkg/runtime/scheme.go @@ -300,6 +300,7 @@ func (s *Scheme) New(kind schema.GroupVersionKind) (Object, error) { // (for two conversion types) to the converter. These functions are checked first during // a normal conversion, but are otherwise not called. Use AddConversionFuncs when registering // typed conversions. +// DEPRECATED: Use AddConversionFunc func (s *Scheme) AddGenericConversionFunc(fn conversion.GenericConversionFunc) { s.converter.AddGenericConversionFunc(fn) } @@ -366,6 +367,20 @@ func (s *Scheme) AddGeneratedConversionFuncs(conversionFuncs ...interface{}) err return nil } +// AddConversionFunc registers a function that converts between a and b by passing objects of those +// types to the provided function. The function *must* accept objects of a and b - this machinery will not enforce +// any other guarantee. +func (s *Scheme) AddConversionFunc(a, b interface{}, fn conversion.ConversionFunc) error { + return s.converter.RegisterUntypedConversionFunc(a, b, fn) +} + +// AddGeneratedConversionFunc registers a function that converts between a and b by passing objects of those +// types to the provided function. The function *must* accept objects of a and b - this machinery will not enforce +// any other guarantee. +func (s *Scheme) AddGeneratedConversionFunc(a, b interface{}, fn conversion.ConversionFunc) error { + return s.converter.RegisterGeneratedUntypedConversionFunc(a, b, fn) +} + // AddFieldLabelConversionFunc adds a conversion function to convert field selectors // of the given kind from the given version to internal version representation. func (s *Scheme) AddFieldLabelConversionFunc(gvk schema.GroupVersionKind, conversionFunc FieldLabelConversionFunc) error { @@ -373,14 +388,6 @@ func (s *Scheme) AddFieldLabelConversionFunc(gvk schema.GroupVersionKind, conver return nil } -// AddStructFieldConversion allows you to specify a mechanical copy for a moved -// or renamed struct field without writing an entire conversion function. See -// the comment in conversion.Converter.SetStructFieldCopy for parameter details. -// Call as many times as needed, even on the same fields. -func (s *Scheme) AddStructFieldConversion(srcFieldType interface{}, srcFieldName string, destFieldType interface{}, destFieldName string) error { - return s.converter.SetStructFieldCopy(srcFieldType, srcFieldName, destFieldType, destFieldName) -} - // RegisterInputDefaults sets the provided field mapping function and field matching // as the defaults for the provided input type. The fn may be nil, in which case no // mapping will happen by default. Use this method to register a mechanism for handling