Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
nunofgs opened this issue Apr 16, 2024 · 2 comments
Closed

Comments

@nunofgs
Copy link

nunofgs commented Apr 16, 2024

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 grpc-ecosystem/go-grpc-middleware#704.

@cretz
Copy link
Member

cretz commented Apr 17, 2024

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.

@Quinn-With-Two-Ns
Copy link
Contributor

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

@Quinn-With-Two-Ns Quinn-With-Two-Ns closed this as not planned Won't fix, can't repro, duplicate, stale Jun 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants