Skip to content

Commit

Permalink
WIP: Use MethodByName only with constants
Browse files Browse the repository at this point in the history
We would like to enable dead-code-elimination, as unblocked by https://go-review.googlesource.com/c/go/+/522436
  • Loading branch information
justinsb committed Aug 4, 2024
1 parent 1dc08db commit 931d094
Show file tree
Hide file tree
Showing 2 changed files with 64 additions and 36 deletions.
74 changes: 44 additions & 30 deletions upup/pkg/fi/context.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ import (
"k8s.io/klog/v2"
"k8s.io/kops/pkg/apis/kops"
"k8s.io/kops/pkg/apis/nodeup"
"k8s.io/kops/util/pkg/reflectutils"
"k8s.io/kops/util/pkg/vfs"
)

Expand Down Expand Up @@ -136,7 +137,7 @@ func (c *Context[T]) RunTasks(options RunTasksOptions) error {
// it is typically called after we have checked the existing state of the Task and determined that is different
// from the desired state.
func (c *Context[T]) Render(a, e, changes Task[T]) error {
typeContextPtr := reflect.TypeOf((*Context[T])(nil))
// typeContextPtr := reflect.TypeOf((*Context[T])(nil))
var lifecycle Lifecycle
if hl, ok := e.(HasLifecycle); ok {
lifecycle = hl.GetLifecycle()
Expand Down Expand Up @@ -192,60 +193,73 @@ func (c *Context[T]) Render(a, e, changes Task[T]) error {
}

v := reflect.ValueOf(e)
vType := v.Type()
// vType := v.Type()

targetType := reflect.ValueOf(c.Target).Type()
// targetType := reflect.ValueOf(c.Target).Type()

var renderer *reflect.Method
var renderer *reflect.Value
var rendererArgs []reflect.Value
rendererName := ""

for i := 0; i < vType.NumMethod(); i++ {
method := vType.Method(i)
if !strings.HasPrefix(method.Name, "Render") {
renderMethodNames := []string{"Render"}

targetTypeName := fmt.Sprintf("%T", c.Target)
switch targetTypeName {
case "Foo":

default:
panic(fmt.Sprintf("targetType %q is not recognized", targetTypeName))
}
for _, methodName := range renderMethodNames {
method := reflectutils.GetMethodByName(v, methodName)
if method.IsZero() {
continue
}
match := true

var args []reflect.Value
for j := 0; j < method.Type.NumIn(); j++ {
arg := method.Type.In(j)
if arg.ConvertibleTo(vType) {
continue
}
if arg.ConvertibleTo(typeContextPtr) {
args = append(args, reflect.ValueOf(c))
continue
}
if arg.ConvertibleTo(targetType) {
args = append(args, reflect.ValueOf(c.Target))
continue
}
match = false
break
}
// var args []reflect.Value
// for j := 0; j < method.Type.NumIn(); j++ {
// arg := method.Type.In(j)
// if arg.ConvertibleTo(vType) {
// continue
// }
// if arg.ConvertibleTo(typeContextPtr) {
// args = append(args, reflect.ValueOf(c))
// continue
// }
// if arg.ConvertibleTo(targetType) {
// args = append(args, reflect.ValueOf(c.Target))
// continue
// }
// match = false
// break
// }
if match {
if renderer != nil {
if method.Name == "Render" {
if methodName == "Render" {
continue
}
if renderer.Name != "Render" {
if rendererName != "Render" {
return fmt.Errorf("found multiple Render methods that could be involved on %T", e)
}
}
renderer = &method
rendererArgs = args
rendererName = methodName
// rendererArgs = args
}

}
if renderer == nil {
return fmt.Errorf("could not find Render method on type %T (target %T)", e, c.Target)
}

rendererArgs = append(rendererArgs, reflect.ValueOf(c))
rendererArgs = append(rendererArgs, reflect.ValueOf(a))
rendererArgs = append(rendererArgs, reflect.ValueOf(e))
rendererArgs = append(rendererArgs, reflect.ValueOf(changes))
klog.V(11).Infof("Calling method %s on %T", renderer.Name, e)
m := v.MethodByName(renderer.Name)
rv := m.Call(rendererArgs)
klog.V(11).Infof("Calling method %s on %T", rendererName, e)
// m := v.MethodByName(renderer.Name)
rv := (*renderer).Call(rendererArgs)
var rvErr error
if !rv[0].IsNil() {
rvErr = rv[0].Interface().(error)
Expand Down
26 changes: 20 additions & 6 deletions util/pkg/reflectutils/walk.go
Original file line number Diff line number Diff line change
Expand Up @@ -57,14 +57,29 @@ func JSONMergeStruct(dest, src interface{}) {
}
}

// GetMethodByName implements a switch statement so that we can help the compiler with dead-code-elimination.
// For background: https://github.com/golang/protobuf/issues/1561
func GetMethodByName(reflectValue reflect.Value, methodName string) reflect.Value {
switch methodName {
case "CheckChanges":
return reflectValue.MethodByName("CheckChanges")
case "Find":
return reflectValue.MethodByName("Find")
case "ShouldCreate":
return reflectValue.MethodByName("ShouldCreate")
default:
panic(fmt.Sprintf("need to add %q to getMethodByName helper", methodName))
}
}

// InvokeMethod calls the specified method by reflection
func InvokeMethod(target interface{}, name string, args ...interface{}) ([]reflect.Value, error) {
func InvokeMethod(target interface{}, methodName string, args ...interface{}) ([]reflect.Value, error) {
v := reflect.ValueOf(target)

method, found := v.Type().MethodByName(name)
if !found {
m := GetMethodByName(v, methodName)
if m.IsZero() {
return nil, &MethodNotFoundError{
Name: name,
Name: methodName,
Target: target,
}
}
Expand All @@ -73,8 +88,7 @@ func InvokeMethod(target interface{}, name string, args ...interface{}) ([]refle
for _, a := range args {
argValues = append(argValues, reflect.ValueOf(a))
}
klog.V(12).Infof("Calling method %s on %T", method.Name, target)
m := v.MethodByName(method.Name)
klog.V(12).Infof("Calling method %s on %T", methodName, target)
rv := m.Call(argValues)
return rv, nil
}
Expand Down

0 comments on commit 931d094

Please sign in to comment.