-
Notifications
You must be signed in to change notification settings - Fork 209
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
Do not inhibit the dead code elimination. #341
base: master
Are you sure you want to change the base?
Do not inhibit the dead code elimination. #341
Conversation
Avoid calls to value.TypeString() that do not request fully qualified type names. The implementation of TypeString() depends on getting methods by index which disables the DCE in the compiler. k8s.io/kube-openapi and k8s.io/apiserver happen to depend on go-cmp outside tests so it is important that go-cmp does not inhibit the DCE inadvertently.
Hey, anybody out here? @dsnet could you please look at the patch? |
The I'm not sure I see how this helps since |
|
Can you point at concrete code? It's a bit esoteric to me that code would depend only on |
GRPC turns out to have an accidental use of go-cmp outside tests, too. It uses After this PR, GRPC no longer imports |
These both seem like cases that are better addressed by the place misusing In the case of In the gRPC case, the use of |
These are very reasonable arguments. The use in kube-openapi looks like a bug to me and I've filed them a PR. The use in gRPC, although inefficient, looks legit to me. What bothers me more is that we are talking about a whack-a-mole solution. You've implemented some really useful features, and people will use them without thinking twice about go-cmp being mostly a package for tests. Kube-openapi, kube-apiserver and gRPC are just examples that were really trivial to find. There certainly are more. I think it is important to minimise the damage that go-cmp causes when used less than prudently. Especially given that disabled DCE seriously pessimises the code generation. |
The
That line undersells "performance is not a goal", however. For example, that gRPC use of This is quite useful for catching inadvertent errors in tests, but is really not suitable behavior for any production context. |
If there were a build tag to isolate tests, I'd be more open to something like this, but as it stands we're going to prioritize good reporting of types in tests over reduced binary bloat. |
Avoid calls to value.TypeString() that do not request fully qualified type names. The implementation of TypeString() depends on getting methods by index which disables the DCE in the compiler.
k8s.io/kube-openapi and k8s.io/apiserver happen to depend on go-cmp outside tests so it is important that go-cmp does not inhibit the DCE inadvertently.