From 236925be877453618a0ccd078922c31aa5fdba1f Mon Sep 17 00:00:00 2001 From: Clayton Coleman Date: Sun, 24 Apr 2016 23:38:50 -0400 Subject: [PATCH] Avoid allocations and a reflect.Call in conversion reflect.Call is fairly expensive, performing 8 allocations and having to set up a call stack. Using a fairly straightforward to generate switch statement, we can bypass that early in conversion (as long as the function takes responsibility for invocation). We may also be able to avoid an allocation for the conversion scope, but not positive yet. ``` benchmark old ns/op new ns/op delta BenchmarkPodConversion-8 14713 12173 -17.26% benchmark old allocs new allocs delta BenchmarkPodConversion-8 80 72 -10.00% benchmark old bytes new bytes delta BenchmarkPodConversion-8 9133 8712 -4.61% ``` --- pkg/api/install/install.go | 86 +++++++++++++++++++++++++++ pkg/conversion/converter.go | 24 ++++++++ pkg/conversion/deep_copy_generated.go | 13 ++++ pkg/runtime/scheme.go | 8 +++ 4 files changed, 131 insertions(+) diff --git a/pkg/api/install/install.go b/pkg/api/install/install.go index 58d121ba86..05433889c4 100644 --- a/pkg/api/install/install.go +++ b/pkg/api/install/install.go @@ -29,6 +29,7 @@ import ( "k8s.io/kubernetes/pkg/api/v1" "k8s.io/kubernetes/pkg/apimachinery" "k8s.io/kubernetes/pkg/apimachinery/registered" + "k8s.io/kubernetes/pkg/conversion" "k8s.io/kubernetes/pkg/runtime" "k8s.io/kubernetes/pkg/util/sets" ) @@ -150,4 +151,89 @@ func addVersionsToScheme(externalVersions ...unversioned.GroupVersion) { v1.AddToScheme(api.Scheme) } } + + // This is a "fast-path" that avoids reflection for common types. It focuses on the objects that are + // converted the most in the cluster. + // TODO: generate one of these for every external API group - this is to prove the impact + api.Scheme.AddGenericConversionFunc(func(objA, objB interface{}, s conversion.Scope) (bool, error) { + switch a := objA.(type) { + case *v1.Pod: + switch b := objB.(type) { + case *api.Pod: + return true, v1.Convert_v1_Pod_To_api_Pod(a, b, s) + } + case *api.Pod: + switch b := objB.(type) { + case *v1.Pod: + return true, v1.Convert_api_Pod_To_v1_Pod(a, b, s) + } + + case *v1.Event: + switch b := objB.(type) { + case *api.Event: + return true, v1.Convert_v1_Event_To_api_Event(a, b, s) + } + case *api.Event: + switch b := objB.(type) { + case *v1.Event: + return true, v1.Convert_api_Event_To_v1_Event(a, b, s) + } + + case *v1.ReplicationController: + switch b := objB.(type) { + case *api.ReplicationController: + return true, v1.Convert_v1_ReplicationController_To_api_ReplicationController(a, b, s) + } + case *api.ReplicationController: + switch b := objB.(type) { + case *v1.ReplicationController: + return true, v1.Convert_api_ReplicationController_To_v1_ReplicationController(a, b, s) + } + + case *v1.Node: + switch b := objB.(type) { + case *api.Node: + return true, v1.Convert_v1_Node_To_api_Node(a, b, s) + } + case *api.Node: + switch b := objB.(type) { + case *v1.Node: + return true, v1.Convert_api_Node_To_v1_Node(a, b, s) + } + + case *v1.Namespace: + switch b := objB.(type) { + case *api.Namespace: + return true, v1.Convert_v1_Namespace_To_api_Namespace(a, b, s) + } + case *api.Namespace: + switch b := objB.(type) { + case *v1.Namespace: + return true, v1.Convert_api_Namespace_To_v1_Namespace(a, b, s) + } + + case *v1.Service: + switch b := objB.(type) { + case *api.Service: + return true, v1.Convert_v1_Service_To_api_Service(a, b, s) + } + case *api.Service: + switch b := objB.(type) { + case *v1.Service: + return true, v1.Convert_api_Service_To_v1_Service(a, b, s) + } + + case *v1.Endpoints: + switch b := objB.(type) { + case *api.Endpoints: + return true, v1.Convert_v1_Endpoints_To_api_Endpoints(a, b, s) + } + case *api.Endpoints: + switch b := objB.(type) { + case *v1.Endpoints: + return true, v1.Convert_api_Endpoints_To_v1_Endpoints(a, b, s) + } + } + return false, nil + }) } diff --git a/pkg/conversion/converter.go b/pkg/conversion/converter.go index 1aeb7a0460..51394564ce 100644 --- a/pkg/conversion/converter.go +++ b/pkg/conversion/converter.go @@ -40,6 +40,8 @@ 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) + // Converter knows how to convert one type to another. type Converter struct { // Map from the conversion pair to a function which can @@ -47,6 +49,11 @@ type Converter struct { conversionFuncs ConversionFuncs generatedConversionFuncs ConversionFuncs + // genericConversions are called during normal conversion to offer a "fast-path" + // that avoids all reflection. These methods are not called outside of the .Convert() + // method. + genericConversions []GenericConversionFunc + // Set of conversions that should be treated as a no-op ignoredConversions map[typePair]struct{} @@ -99,6 +106,14 @@ func NewConverter(nameFn NameFunc) *Converter { return c } +// AddGenericConversionFunc adds a function that accepts the ConversionFunc call pattern +// (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. +func (c *Converter) AddGenericConversionFunc(fn GenericConversionFunc) { + c.genericConversions = append(c.genericConversions, fn) +} + // WithConversions returns a Converter that is a copy of c but with the additional // fns merged on top. func (c *Converter) WithConversions(fns ConversionFuncs) *Converter { @@ -500,6 +515,15 @@ 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 { + 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 { + return err + } + } + } return c.doConversion(src, dest, flags, meta, c.convert) } diff --git a/pkg/conversion/deep_copy_generated.go b/pkg/conversion/deep_copy_generated.go index 84e653019b..c8de3276df 100644 --- a/pkg/conversion/deep_copy_generated.go +++ b/pkg/conversion/deep_copy_generated.go @@ -67,6 +67,19 @@ func DeepCopy_conversion_Converter(in Converter, out *Converter, c *Cloner) erro if err := DeepCopy_conversion_ConversionFuncs(in.generatedConversionFuncs, &out.generatedConversionFuncs, c); err != nil { return err } + if in.genericConversions != nil { + in, out := in.genericConversions, &out.genericConversions + *out = make([]GenericConversionFunc, len(in)) + for i := range in { + if newVal, err := c.DeepCopy(in[i]); err != nil { + return err + } else { + (*out)[i] = newVal.(GenericConversionFunc) + } + } + } else { + out.genericConversions = nil + } if in.ignoredConversions != nil { in, out := in.ignoredConversions, &out.ignoredConversions *out = make(map[typePair]struct{}) diff --git a/pkg/runtime/scheme.go b/pkg/runtime/scheme.go index 9a4a708963..842e8c67ad 100644 --- a/pkg/runtime/scheme.go +++ b/pkg/runtime/scheme.go @@ -275,6 +275,14 @@ func (s *Scheme) New(kind unversioned.GroupVersionKind) (Object, error) { return nil, ¬RegisteredErr{gvk: kind} } +// AddGenericConversionFunc adds a function that accepts the ConversionFunc call pattern +// (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. +func (s *Scheme) AddGenericConversionFunc(fn conversion.GenericConversionFunc) { + s.converter.AddGenericConversionFunc(fn) +} + // Log sets a logger on the scheme. For test purposes only func (s *Scheme) Log(l conversion.DebugLogger) { s.converter.Debug = l