x/net/trace: dependency on html/template inhibits dead code elimination

[artem-anisimov-0x7f]

Any use of reflect.Value.MethodByName() or reflect.Type.MethodByName() disables the DCE. The compiler assumes that these functions will look any method up, and cannot remove methods even if they are never called.

text/template and html/template are very notable offenders in this respect. They call MethodByName() on random strings extracted from template texts.

The package x/net/trace uses html/template, hence any user of x/net/trace compiles with DCE disabled. Worse yet, the package's init() references html/template which disables DCE even if x/net/trace is imported, but otherwise unused.

There is a very popular user of x/net/trace, namely google.golang.org/grpc. Essentially, this means that a lot of networked services in golang are compiled without DCE.

What did you expect to see?

I expect that a low-level library that is used by almost everyone does not pessimise the code generation by disabling the DCE. I'd avoid html/template altogether.

[rittneje]

text/template and html/template are very notable offenders in this respect. They call MethodByName() on random strings extracted from template texts.

This is also surprising and undesirable behavior from the perspective of a client of text/template and html/template, given that it disables all DCE.

On that note, it would probably be helpful if the docs for reflect.MethodByName warned about this reality.

[AlexanderYastrebov]

Looks like MethodByName is only used in text/template (imported by html/template)

$ grep -rnl MethodByName
reflect/type.go
reflect/all_test.go
reflect/abi_test.go
reflect/value.go
text/template/exec.go
cmd/cgo/internal/testplugin/testdata/method/main.go
cmd/link/internal/ld/deadcode.go
cmd/compile/internal/ir/func.go
cmd/compile/internal/walk/expr.go
cmd/internal/obj/link.go
cmd/internal/obj/textflag.go
go/internal/gccgoimporter/testdata/v1reflect.gox
go/internal/gccgoimporter/importer_test.go
runtime/textflag.h

I am wondering if a build tag like this

diff --git a/src/text/template/exec.go b/src/text/template/exec.go
index 2b778fff69..f11c0fc9c3 100644
--- a/src/text/template/exec.go
+++ b/src/text/template/exec.go
@@ -648,7 +648,7 @@ func (s *state) evalField(dot reflect.Value, fieldName string, node parse.Node,
        if ptr.Kind() != reflect.Interface && ptr.Kind() != reflect.Pointer && ptr.CanAddr() {
                ptr = ptr.Addr()
        }
-       if method := ptr.MethodByName(fieldName); method.IsValid() {
+       if method := methodByName(ptr, fieldName); method.IsValid() {
                return s.evalCall(dot, method, false, node, fieldName, args, final)
        }
        hasArgs := len(args) > 1 || !isMissing(final)
diff --git a/src/text/template/methodbyname_disabled.go b/src/text/template/methodbyname_disabled.go
new file mode 100644
index 0000000000..5947ee0fe3
--- /dev/null
+++ b/src/text/template/methodbyname_disabled.go
@@ -0,0 +1,9 @@
+//go:build text_template_methodbyname_disabled
+
+package template
+
+import "reflect"
+
+func methodByName(ptr reflect.Value, fieldName string) reflect.Value {
+       return reflect.Value{}
+}
diff --git a/src/text/template/methodbyname_enabled.go b/src/text/template/methodbyname_enabled.go
new file mode 100644
index 0000000000..8a65badd31
--- /dev/null
+++ b/src/text/template/methodbyname_enabled.go
@@ -0,0 +1,9 @@
+//go:build !text_template_methodbyname_disabled
+
+package template
+
+import "reflect"
+
+func methodByName(ptr reflect.Value, fieldName string) reflect.Value {
+       return ptr.MethodByName(fieldName)
+}

could be added for those who knows MethodByName is not used.

Otherwise text/template would need a new entry point (ExecuteWithoutMethodByName?) that does not invoke MethodByName.

[aarzilli]

given that it disables all DCE.

it doesn't disable all DCE, it disables it for public methods (which is a lot, but not all of it).

[AlexanderYastrebov]

Previous related #36021

[AlexanderYastrebov]

I am wondering if a build tag like this

This could be a Go experiment AlexanderYastrebov/go@go62024~1...AlexanderYastrebov:go:go62024

[artem-anisimov-0x7f]

@AlexanderYastrebov, the proposed patch will not work. It is mere presence of a call to MethodByName() in the code that disables the DCE. Calls to MethodByName() must be under a build tag as in

I am wondering if a build tag like this

[artem-anisimov-0x7f]

This could be a Go experiment AlexanderYastrebov/go@go62024~1...AlexanderYastrebov:go:go62024

I think that an experiment alone does not suffice. x/net/trace must be fixed without relying on the experiment.

First, a user of gRPC needs to know that they are affected by disabled DCE. I think it is wrong to assume that everyone verifies how the compiler does its job.

Second, I'd least expect gRPC to use text templating, so why would I bother with experiments related to text/template? And how would I know if it safe to enable the experiment when I use gRPC? It is certainly not safe for spf13/cobra.

Users of gRPC must be able to fix it with a plain go get -u golang.org/x/net.

[AlexanderYastrebov]

@alexander-hirth

the proposed patch will not work
I think that an experiment alone does not suffice

Both do the same, experiment is a standardized way to define a build tag.

x/net/trace must be fixed without relying on the experiment.

I agree, it could render html without using html/template that causes the trouble. But other code that uses text/template would still suffer.
I was thinking more about fixing the root cause - disabling use of MethodByName in text/template (specially due to a single use in the stdlib)

[mknyszek]

In triage.

Someone could rewrite this package to avoid html/template, but that would be complicated (and error-prone). It would be nice if Render wasn't part of this API and in a separate package, but unfortunately we can't change that now due to API compatibility guarantees. The root cause here though appears to be that an init function in x/net/trace (that installs a handler on the default mux in net/http) is reaching html/template. The compiler and/or linker are already smart enough to eliminate package dependencies if they're not reachable, but this case is too complex for it to do so. (It would have to prove that the default mux is not used.)

I think we're open to ideas to restructure this package in a fairly straightforward way and remove the dependency but I don't see one right now.

The bigger issue of MethodByName is another thing altogether. I don't think we have any good answers there yet.

EDIT: Updated to say what I meant in the first place.