diff --git a/cmd/libs/go2idl/conversion-gen/generators/conversion.go b/cmd/libs/go2idl/conversion-gen/generators/conversion.go index e1c62c247b0..61f89b8fbe7 100644 --- a/cmd/libs/go2idl/conversion-gen/generators/conversion.go +++ b/cmd/libs/go2idl/conversion-gen/generators/conversion.go @@ -21,6 +21,7 @@ import ( "fmt" "io" "path/filepath" + "sort" "strings" "k8s.io/gengo/args" @@ -36,10 +37,11 @@ import ( // generator. type CustomArgs struct { ExtraPeerDirs []string // Always consider these as last-ditch possibilities for conversions. - // SkipDefaulters indicates whether defaulter functions should be a part of conversion - // This field was introduced to ease the transition to removing defaulters from conversion. - // It will be removed in 1.6. - SkipDefaulters bool + // Skipunsafe indicates whether to generate unsafe conversions to improve the efficiency + // of these operations. The unsafe operation is a direct pointer assignment via unsafe + // (within the allowed uses of unsafe) and is equivalent to a proposed Golang change to + // allow structs that are identical to be assigned to each other. + SkipUnsafe bool } // This is the comment tag that carries parameters for conversion generation. @@ -49,6 +51,16 @@ func extractTag(comments []string) []string { return types.ExtractCommentTags("+", comments)[tagName] } +func isCopyOnly(comments []string) bool { + values := types.ExtractCommentTags("+", comments)["k8s:conversion-fn"] + return len(values) == 1 && values[0] == "copy-only" +} + +func isDrop(comments []string) bool { + values := types.ExtractCommentTags("+", comments)["k8s:conversion-fn"] + return len(values) == 1 && values[0] == "drop" +} + // TODO: This is created only to reduce number of changes in a single PR. // Remove it and use PublicNamer instead. func conversionNamer() *namer.NameStrategy { @@ -156,56 +168,6 @@ func getManualConversionFunctions(context *generator.Context, pkg *types.Package } } -// All of the types in conversions map are of type "DeclarationOf" with -// the underlying type being "Func". -type defaulterFuncMap map[*types.Type]*types.Type - -// Returns all manually-defined defaulting functions in the package. -func getManualDefaultingFunctions(context *generator.Context, pkg *types.Package, manualMap defaulterFuncMap) { - buffer := &bytes.Buffer{} - sw := generator.NewSnippetWriter(buffer, context, "$", "$") - - for _, f := range pkg.Functions { - if f.Underlying == nil || f.Underlying.Kind != types.Func { - glog.Errorf("Malformed function: %#v", f) - continue - } - if f.Underlying.Signature == nil { - glog.Errorf("Function without signature: %#v", f) - continue - } - signature := f.Underlying.Signature - // Check whether the function is conversion function. - // Note that all of them have signature: - // func Convert_inType_To_outType(inType, outType, conversion.Scope) error - if signature.Receiver != nil { - continue - } - if len(signature.Parameters) != 1 { - continue - } - if len(signature.Results) != 0 { - continue - } - inType := signature.Parameters[0] - if inType.Kind != types.Pointer { - continue - } - // Now check if the name satisfies the convention. - args := defaultingArgsFromType(inType.Elem) - sw.Do("$.inType|defaultfn$", args) - if f.Name.Name == buffer.String() { - key := inType.Elem - // We might scan the same package twice, and that's OK. - if v, ok := manualMap[key]; ok && v != nil && v.Name.Package != pkg.Path { - panic(fmt.Sprintf("duplicate static defaulter defined: %#v", key)) - } - manualMap[key] = f - } - buffer.Reset() - } -} - func Packages(context *generator.Context, arguments *args.GeneratorArgs) generator.Packages { boilerplate, err := arguments.LoadGoBoilerplate() if err != nil { @@ -217,10 +179,17 @@ func Packages(context *generator.Context, arguments *args.GeneratorArgs) generat header := append([]byte(fmt.Sprintf("// +build !%s\n\n", arguments.GeneratedBuildTag)), boilerplate...) header = append(header, []byte("\n// This file was autogenerated by conversion-gen. Do not edit it manually!\n\n")...) - // Accumulate pre-existing conversion and default functions. + // Accumulate pre-existing conversion functions. // TODO: This is too ad-hoc. We need a better way. manualConversions := conversionFuncMap{} - manualDefaults := defaulterFuncMap{} + + // Record types that are memory equivalent. A type is memory equivalent + // if it has the same memory layout and no nested manual conversion is + // defined. + // TODO: in the future, relax the nested manual conversion requirement + // if we can show that a large enough types are memory identical but + // have non-trivial conversion + memoryEquivalentTypes := equalMemoryTypes{} // We are generating conversions only for packages that are explicitly // passed as InputDir. @@ -234,7 +203,6 @@ func Packages(context *generator.Context, arguments *args.GeneratorArgs) generat // Add conversion and defaulting functions. getManualConversionFunctions(context, pkg, manualConversions) - getManualDefaultingFunctions(context, pkg, manualDefaults) // Only generate conversions for packages which explicitly request it // by specifying one or more "+k8s:conversion-gen=" @@ -246,18 +214,23 @@ func Packages(context *generator.Context, arguments *args.GeneratorArgs) generat glog.V(5).Infof(" no tag") continue } - skipDefaulters := false + skipUnsafe := false if customArgs, ok := arguments.CustomArgs.(*CustomArgs); ok { if len(customArgs.ExtraPeerDirs) > 0 { peerPkgs = append(peerPkgs, customArgs.ExtraPeerDirs...) } - skipDefaulters = customArgs.SkipDefaulters + skipUnsafe = customArgs.SkipUnsafe } + // Make sure our peer-packages are added and fully parsed. for _, pp := range peerPkgs { context.AddDir(pp) getManualConversionFunctions(context, context.Universe[pp], manualConversions) - getManualDefaultingFunctions(context, context.Universe[pp], manualDefaults) + } + + unsafeEquality := TypesEqual(memoryEquivalentTypes) + if skipUnsafe { + unsafeEquality = noEquality{} } packages = append(packages, @@ -267,7 +240,7 @@ func Packages(context *generator.Context, arguments *args.GeneratorArgs) generat HeaderText: header, GeneratorFunc: func(c *generator.Context) (generators []generator.Generator) { return []generator.Generator{ - NewGenConversion(arguments.OutputFileBaseName, pkg.Path, manualConversions, manualDefaults, peerPkgs, !skipDefaulters), + NewGenConversion(arguments.OutputFileBaseName, pkg.Path, manualConversions, peerPkgs, unsafeEquality), } }, FilterFunc: func(c *generator.Context, t *types.Type) bool { @@ -275,9 +248,79 @@ func Packages(context *generator.Context, arguments *args.GeneratorArgs) generat }, }) } + + // If there is a manual conversion defined between two types, exclude it + // from being a candidate for unsafe conversion + for k, v := range manualConversions { + if isCopyOnly(v.CommentLines) { + glog.V(5).Infof("Conversion function %s will not block memory copy because it is copy-only", v.Name) + continue + } + // this type should be excluded from all equivalence, because the converter must be called. + memoryEquivalentTypes.Skip(k.inType, k.outType) + } + return packages } +type equalMemoryTypes map[conversionPair]bool + +func (e equalMemoryTypes) Skip(a, b *types.Type) { + e[conversionPair{a, b}] = false + e[conversionPair{b, a}] = false +} + +func (e equalMemoryTypes) Equal(a, b *types.Type) bool { + if a == b { + return true + } + if equal, ok := e[conversionPair{a, b}]; ok { + return equal + } + if equal, ok := e[conversionPair{b, a}]; ok { + return equal + } + result := e.equal(a, b) + e[conversionPair{a, b}] = result + e[conversionPair{b, a}] = result + return result +} + +func (e equalMemoryTypes) equal(a, b *types.Type) bool { + in, out := unwrapAlias(a), unwrapAlias(b) + switch { + case in == out: + return true + case in.Kind == out.Kind: + switch in.Kind { + case types.Struct: + if len(in.Members) != len(out.Members) { + return false + } + for i, inMember := range in.Members { + outMember := out.Members[i] + if !e.Equal(inMember.Type, outMember.Type) { + return false + } + } + return true + case types.Pointer: + return e.Equal(in.Elem, out.Elem) + case types.Map: + return e.Equal(in.Key, out.Key) && e.Equal(in.Elem, out.Elem) + case types.Slice: + return e.Equal(in.Elem, out.Elem) + case types.Interface: + // TODO: determine whether the interfaces are actually equivalent - for now, they must have the + // same type. + return false + case types.Builtin: + return in.Name.Name == out.Name.Name + } + } + return false +} + func findMember(t *types.Type, name string) (types.Member, bool) { if t.Kind != types.Struct { return types.Member{}, false @@ -303,20 +346,27 @@ const ( conversionPackagePath = "k8s.io/kubernetes/pkg/conversion" ) +type noEquality struct{} + +func (noEquality) Equal(_, _ *types.Type) bool { return false } + +type TypesEqual interface { + Equal(a, b *types.Type) bool +} + // genConversion produces a file with a autogenerated conversions. type genConversion struct { generator.DefaultGen targetPackage string peerPackages []string manualConversions conversionFuncMap - manualDefaulters defaulterFuncMap imports namer.ImportTracker types []*types.Type skippedFields map[*types.Type][]string - includeDefaulters bool + useUnsafe TypesEqual } -func NewGenConversion(sanitizedName, targetPackage string, manualConversions conversionFuncMap, manualDefaulters defaulterFuncMap, peerPkgs []string, includeDefaulters bool) generator.Generator { +func NewGenConversion(sanitizedName, targetPackage string, manualConversions conversionFuncMap, peerPkgs []string, useUnsafe TypesEqual) generator.Generator { return &genConversion{ DefaultGen: generator.DefaultGen{ OptionalName: sanitizedName, @@ -324,11 +374,10 @@ func NewGenConversion(sanitizedName, targetPackage string, manualConversions con targetPackage: targetPackage, peerPackages: peerPkgs, manualConversions: manualConversions, - manualDefaulters: manualDefaulters, imports: generator.NewImportTracker(), types: []*types.Type{}, skippedFields: map[*types.Type][]string{}, - includeDefaulters: includeDefaulters, + useUnsafe: useUnsafe, } } @@ -439,6 +488,22 @@ func (g *genConversion) preexists(inType, outType *types.Type) (*types.Type, boo } func (g *genConversion) Init(c *generator.Context, w io.Writer) error { + if glog.V(5) { + if m, ok := g.useUnsafe.(equalMemoryTypes); ok { + var result []string + glog.Infof("All objects without identical memory layout:") + for k, v := range m { + if v { + continue + } + result = append(result, fmt.Sprintf(" %s -> %s = %t", k.inType, k.outType, v)) + } + sort.Strings(result) + for _, s := range result { + glog.Infof(s) + } + } + } sw := generator.NewSnippetWriter(w, c, "$", "$") sw.Do("func init() {\n", nil) sw.Do("SchemeBuilder.Register(RegisterConversions)\n", nil) @@ -477,10 +542,6 @@ func (g *genConversion) generateConversion(inType, outType *types.Type, sw *gene With("Scope", types.Ref(conversionPackagePath, "Scope")) sw.Do("func auto"+nameTmpl+"(in *$.inType|raw$, out *$.outType|raw$, s $.Scope|raw$) error {\n", args) - // if no defaulter of form SetDefaults_XXX is defined, do not inline a check for defaulting. - if function, ok := g.manualDefaulters[inType]; ok && g.includeDefaulters { - sw.Do("$.|raw$(in)\n", function) - } g.generateFor(inType, outType, sw) sw.Do("return nil\n", nil) sw.Do("}\n\n", nil) @@ -508,6 +569,7 @@ func (g *genConversion) generateConversion(inType, outType *types.Type, sw *gene func (g *genConversion) generateFor(inType, outType *types.Type, sw *generator.SnippetWriter) { glog.V(5).Infof("generating %v -> %v", inType, outType) var f func(*types.Type, *types.Type, *generator.SnippetWriter) + switch inType.Kind { case types.Builtin: f = g.doBuiltin @@ -524,6 +586,7 @@ func (g *genConversion) generateFor(inType, outType *types.Type, sw *generator.S default: f = g.doUnknown } + f(inType, outType, sw) } @@ -638,18 +701,51 @@ func (g *genConversion) doStruct(inType, outType *types.Type, sw *generator.Snip outMemberType = &copied } - args := map[string]interface{}{ - "inType": inMemberType, - "outType": outMemberType, - "name": inMember.Name, + args := argsFromType(inMemberType, outMemberType).With("name", inMember.Name) + + // try a direct memory copy for any type that has exactly equivalent values + if g.useUnsafe.Equal(inMemberType, outMemberType) { + args = args. + With("Pointer", types.Ref("unsafe", "Pointer")). + With("SliceHeader", types.Ref("reflect", "SliceHeader")) + switch inMemberType.Kind { + case types.Pointer: + sw.Do("out.$.name$ = ($.outType|raw$)($.Pointer|raw$(in.$.name$))\n", args) + continue + case types.Map: + sw.Do("{\n", nil) + sw.Do("m := (*$.outType|raw$)($.Pointer|raw$(&in.$.name$))\n", args) + sw.Do("out.$.name$ = *m\n", args) + sw.Do("}\n", nil) + continue + case types.Slice: + sw.Do("{\n", nil) + sw.Do("outHdr := (*$.SliceHeader|raw$)($.Pointer|raw$(&out.$.name$))\n", args) + sw.Do("inHdr := (*$.SliceHeader|raw$)($.Pointer|raw$(&in.$.name$))\n", args) + sw.Do("*outHdr = *inHdr\n", nil) + sw.Do("}\n", nil) + continue + } } + // check based on the top level name, not the underlying names if function, ok := g.preexists(inMember.Type, outMember.Type); ok { - args["function"] = function - sw.Do("if err := $.function|raw$(&in.$.name$, &out.$.name$, s); err != nil {\n", args) - sw.Do("return err\n", nil) - sw.Do("}\n", nil) - continue + if isDrop(function.CommentLines) { + continue + } + // copy-only functions that are directly assignable can be inlined instead of invoked. + // As an example, conversion functions exist that allow types with private fields to be + // correctly copied between types. These functions are equivalent to a memory assignment, + // and are necessary for the reflection path, but should not block memory conversion. + // Convert_unversioned_Time_to_unversioned_Time is an example of this logic. + if !isCopyOnly(function.CommentLines) || !g.isFastConversion(inMemberType, outMemberType) { + args["function"] = function + sw.Do("if err := $.function|raw$(&in.$.name$, &out.$.name$, s); err != nil {\n", args) + sw.Do("return err\n", nil) + sw.Do("}\n", nil) + continue + } + glog.V(5).Infof("Skipped function %s because it is copy-only and we can use direct assignment", function.Name) } // If we can't auto-convert, punt before we emit any code. @@ -722,6 +818,17 @@ func (g *genConversion) doStruct(inType, outType *types.Type, sw *generator.Snip } } +func (g *genConversion) isFastConversion(inType, outType *types.Type) bool { + switch inType.Kind { + case types.Builtin: + return true + case types.Map, types.Slice, types.Pointer, types.Struct, types.Alias: + return g.isDirectlyAssignable(inType, outType) + default: + return false + } +} + func (g *genConversion) isDirectlyAssignable(inType, outType *types.Type) bool { return unwrapAlias(inType) == unwrapAlias(outType) } diff --git a/cmd/libs/go2idl/conversion-gen/main.go b/cmd/libs/go2idl/conversion-gen/main.go index 097c0ea8dba..79f515e5ad0 100644 --- a/cmd/libs/go2idl/conversion-gen/main.go +++ b/cmd/libs/go2idl/conversion-gen/main.go @@ -60,7 +60,6 @@ func main() { "k8s.io/kubernetes/pkg/conversion", "k8s.io/kubernetes/pkg/runtime", }, - SkipDefaulters: true, } pflag.CommandLine.StringSliceVar(&customArgs.ExtraPeerDirs, "extra-peer-dirs", customArgs.ExtraPeerDirs, "Comma-separated list of import paths which are considered, after tag-specified peers, for conversions.")