Avoid reflect.MethodByName

[vincentbernat]

Hey!

When using reflect.Method() or reflect.MethodByName() with a non-constant argument, dead code elimination is disabled for public functions. This makes the final binary bigger than it should. See for example kubernetes/kubernetes#132216 for an effort on the subject.

I am using the expr.Function() to declare function, so I think it should be possible to avoid calling reflect.MethodByName(). Would you consider a tag "exprdeclarative" or something like that to opt out reflect?

This would also disable some builtins, like get.

[vincentbernat]

FI, I have just remove code using the culprit methods from expr: master...vincentbernat:expr:feature/dce-elimination. My own tests still pass. This is to show what could be possible.

[antonmedv]

With expr.Function() no reflect.MethodByName() is needed. But reflect.MethodByName() is kept as a fallback for other calls. There is no way to tell linter what this code will not be reachable.

One of the solutions is to introduce build tag which will hide reflect.MethodByName() usage.

[vincentbernat]

Yes, that was my point. Would you be OK with such a tag?

[antonmedv]

Yes, we just need to find a good name (expr_no_method_by_name?) and documentation.

[vincentbernat]

OK, I can do that. My idea is to have reflect_on.go and reflectmethod_off.go, one with const disableReflectMethod := true and the other with const disableReflectMethod := false, and use this const in conditionals and rely on Go dead code elimination. Tag could be expr_no_reflectmethod (both .Method() and .MethodByName() should be removed).

[vincentbernat]

I think I may have been too optimistic about my ability to do that. For example, looking at:

func (c *compiler) CallNode(node *ast.CallNode) {
	fn := node.Callee.Type()
	if fn.Kind() == reflect.Func {
		fnInOffset := 0
		fnNumIn := fn.NumIn()
		switch callee := node.Callee.(type) {
		case *ast.MemberNode:
			if prop, ok := callee.Property.(*ast.StringNode); ok {
				if _, ok = callee.Node.Type().MethodByName(prop.Value); ok && callee.Node.Type().Kind() != reflect.Interface {
					fnInOffset = 1
					fnNumIn--
				}
			}
		case *ast.IdentifierNode:
			if t, ok := c.config.Env.MethodByName(c.ntCache, callee.Value); ok && t.Method {
				fnInOffset = 1
				fnNumIn--
			}
		}
		for i, arg := range node.Arguments {
			c.compile(arg)

			var in reflect.Type
			if fn.IsVariadic() && i >= fnNumIn-1 {
				in = fn.In(fn.NumIn() - 1).Elem()
			} else {
				in = fn.In(i + fnInOffset)
			}

			c.derefParam(in, arg)
		}
	} else {
		for _, arg := range node.Arguments {
			c.compile(arg)
		}
	}

	if ident, ok := node.Callee.(*ast.IdentifierNode); ok {
		if c.config != nil {
			if fn, ok := c.config.Functions[ident.Value]; ok {
				c.emitFunction(fn, len(node.Arguments))
				return
			}
		}
	}
	c.compile(node.Callee)

	if c.config != nil {
		isMethod, _, _ := checker.MethodIndex(c.ntCache, c.config.Env, node.Callee)
		if index, ok := checker.TypedFuncIndex(node.Callee.Type(), isMethod); ok {
			c.emit(OpCallTyped, index)
			return
		} else if checker.IsFastFunc(node.Callee.Type(), isMethod) {
			c.emit(OpCallFast, len(node.Arguments))
		} else {
			c.emit(OpCall, len(node.Arguments))
		}
	} else {
		c.emit(OpCall, len(node.Arguments))
	}
}

I know the bottom part of the function is using expr.Function() but for the upper part, I don't know exactly if the whole if/else should be removed, only the if part or only the switch/case. I think only the if part should be removed, but I can't be really sure. It seems that I will have to guess everything and this is unlikely to be correct.

I have updated the branch master...vincentbernat:expr:feature/dce-elimination with my current situation. Next steps would be to adapt the tests plus add a test to avoid a regression.

[antonmedv]

I see. I will try to work on this park as well.