Add a new conversion path to replace GenericConversionFunc #65771
Conversation
k8s-ci-robot
added
do-not-merge/release-note-label-needed
k8s-ci-robot
added
cncf-cla: yes
k8s-ci-robot
added
size/XL
k8s-ci-robot
added
release-note
|
No |
|
The release-note is very technical. This is optimization behind the scenes mostly, only interesting for us and aggregated apiserver builders. |
| type GenericConversionFunc func(a, b interface{}, scope Scope) (bool, error) | ||
|
|
||
| // ConversionFunction moves data from a into b. It should return an error if the data cannot | ||
| // be moved. | ||
| type ConversionFunc func(a, b interface{}, scope Scope) error |
There was a problem hiding this comment.
define "move". I know what you mean and it is actually a good term (= b might share data structures with a, but a is not mutated).
| @@ -40,8 +40,14 @@ type NameFunc func(t reflect.Type) string | |||
|
|
|||
| var DefaultNameFunc = func(t reflect.Type) string { return t.Name() } | |||
|
|
|||
| // GenericConversionFunction copies data from a into b. It should return an error if the | |||
| // data cannot be moved. It returns false if the provided objects are not recognized. | |||
| type GenericConversionFunc func(a, b interface{}, scope Scope) (bool, error) | |||
| 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 a generic conversion", a) |
| @@ -355,16 +386,32 @@ func verifyConversionFunctionSignature(ft reflect.Type) error { | |||
| // // conversion logic... | |||
| // return nil | |||
| // }) | |||
| // DEPRECATED: Will be removed in favor of RegisterGenericConversionFunc | |||
There was a problem hiding this comment.
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 RegisterGenericConversionFunc |
There was a problem hiding this comment.
RegisterGeneratedUntypedConversionFunc
| // 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 { |
There was a problem hiding this comment.
do we really need two register funcs? They do the same.
There was a problem hiding this comment.
a typo? The upper one should add to c.conversionFuncs, no?
There was a problem hiding this comment.
Upper one should have been conversionFuncs, fixed
| // 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 { |
There was a problem hiding this comment.
a bit confusing to have AddConversionFuncs for typed and AddConversionFunc for untyped.
| @@ -72,7 +72,7 @@ func Convert_v1beta1_ReplicaSet_to_api_ReplicationController(in *v1beta1.Replica | |||
| } | |||
|
|
|||
| intermediate2 := &v1.ReplicationController{} | |||
| if err := k8s_api_v1.Convert_extensions_ReplicaSet_to_v1_ReplicationController(intermediate1, intermediate2, s); err != nil { | |||
| if err := k8s_api_v1.Convert_extensions_ReplicaSet_To_v1_ReplicationController(intermediate1, intermediate2, s); err != nil { | |||
There was a problem hiding this comment.
ic, only a couple which were misnamed.
There was a problem hiding this comment.
Right, which means they were "accidentally" (not used) omitted from static analysis. I strengthened the static check so that you can't get into this scenario anymore without calling it something other than Convert_.
| return false, nil | ||
| }) | ||
| return nil | ||
| } |
|
I like the mechanism a lot! On the other hand, it reminds me of the old |
k8s-ci-robot
added
the
needs-rebase
k8s-ci-robot
removed
the
needs-rebase
|
/retest |
k8s-ci-robot
added
release-note-none
|
Split the second half of this into a separate linked PR after discussion with David - disabling the reflection path can happen more slowly. This PR removes the manual part of fast path. Will move the conversion parts into an upstream PR after review. |
k8s-ci-robot
added
the
needs-rebase
|
@sttts the second PR will be the more invasive one - up to you whether you want to do this in pieces or all at once. |
| // prevent user error when they don't get the correct conversion signature | ||
| if strings.HasPrefix(f.Name.Name, "Convert_") { | ||
| glog.Errorf("Rename function %s %s -> %s to match expected conversion signature", f.Name.Package, f.Name.Name, buffer.String()) | ||
| } |
There was a problem hiding this comment.
what's the goal here? Drop the commit? Or merge both outputs?
There was a problem hiding this comment.
nit: put the message below into the else branch.
There was a problem hiding this comment.
Him - I definitely wanted to be able to get a consistent error in the debug scenario (i.e., if you've jacked logging up to 8, I want to filter on "wrong name" via grep). So it was quasi intentional to display both at v8.
There was a problem hiding this comment.
Ok, no strong opinion here. Am ok with how it is.
I don't care much, both is fine. Can you clean up the DO NOT MERGE commits and rebase? |
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".
k8s-ci-robot
removed
the
needs-rebase
|
The DO NOT MERGES were conversion-gen changes - I didn't realize that was in staging, not vendor. So those are now updated with the correct commit title. |
|
/retest |
|
One nit left. Otherwise lgtm. |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: smarterclayton, sttts The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/retest Review the full test history for this PR. Silence the bot with an |
|
/test all [submit-queue is verifying that this PR is safe to merge] |
|
Automatic merge from submit-queue (batch tested with PRs 65771, 65849). If you want to cherry-pick this change to another branch, please follow the instructions here. |
|
@smarterclayton: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
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 which is hand-written. Instead of having these be handwritten, we should generate them.
The pattern for generating them looks like:
which matches AddDefaultObjectFunc (which proved out the approach last year). The
conversion machinery should then do a simple map lookup based on the incoming types and invoke the function. Like defaulting, it's up to the caller to match the types to arguments, which we do by generating this code. This bypasses reflect.Call and in the future allows Golang mid-stack inlining to optimize this code.
As part of this change I strengthened registration of custom functions to be generated instead of hand registered, and also strengthened error checking of the generator when it sees a manual conversion to error out. Since custom functions are automatically used by the generator, we don't really have a case for not registering the functions.
Once this is fully tested out, we can remove the reflection based path and the old registration methods, and all conversion will work from point to point methods (whether generated or custom).
Much of the need for the reflection path has been removed by changes to generation (to omit fields) and changes to Go (to make assigning equivalent structs easy).