sdk-go icon indicating copy to clipboard operation
sdk-go copied to clipboard

Use of grpc-ecosystem/go-grpc-middleware/retry prevents dead code elimination

Open nunofgs opened this issue 2 years ago • 1 comments

The retry package in grpc-ecosystem/go-grpc-middleware makes use of golang.org/x/net/trace, which in turn calls text/template, which calls reflect.Value.MethodByName severely hampering dead code elimination.

You can verify by pointing this with whydeadcode:

❯ go build -ldflags=-dumpdep ./cmd |& whydeadcode
reflect.Value.MethodByName reachable from:
	 text/template.(*state).evalField
	 text/template.(*state).evalFieldChain
	 text/template.(*state).evalFieldNode
	 text/template.(*state).evalCommand
	 text/template.(*state).evalPipeline
	 text/template.(*state).walk
	 text/template.(*Template).execute
	 html/template.(*Template).Execute
	 golang.org/x/net/trace.RenderEvents
	 golang.org/x/net/trace.Events
	 golang.org/x/net/trace.Events·f
	 golang.org/x/net/trace.init.0
	 golang.org/x/net/trace..inittask
	 github.com/grpc-ecosystem/go-grpc-middleware/retry..inittask
	 go.temporal.io/sdk/internal..inittask
	 go.temporal.io/sdk/client..inittask
	 main..inittask
	 runtime.main
	 runtime.mainPC
	 runtime.rt0_go
	 _rt0_arm64_darwin
	 main
	 _

Is it possible to work around it?

Also reported in https://github.com/grpc-ecosystem/go-grpc-middleware/issues/704.

nunofgs avatar Apr 16 '24 21:04 nunofgs

which calls reflect.Value.MethodByName severely hampering dead code elimination.

How severely? And is it just this one reflection call or all of reflection? We also use reflection heavily, so unsure if removing that reflection call helps much.

Is it possible to work around it?

I am not sure it's reasonable to work around this issue with a commonly used dependency like go-grpc-middleware. Of course if solved upstream we'd gladly take the benefits.

cretz avatar Apr 17 '24 12:04 cretz

Nothing immediate we can do here go-grpc-middleware is a dependency of the SDK. if go-grpc-middleware resolves https://github.com/grpc-ecosystem/go-grpc-middleware/issues/704 then we would update to that version.

Quinn-With-Two-Ns avatar Jun 13 '24 14:06 Quinn-With-Two-Ns