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

Allow linker to perform dead code elimination for programs using go-cmp #373

Open
pgimalac opened this issue Feb 18, 2025 · 4 comments · May be fixed by #374 or #375
Open

Allow linker to perform dead code elimination for programs using go-cmp #373

pgimalac opened this issue Feb 18, 2025 · 4 comments · May be fixed by #374 or #375

Comments

@pgimalac
Copy link

pgimalac commented Feb 18, 2025

Describe the bug
When reflect's Method is used with a non-constant argument, the linker has to keep every public method of reachable types because it can't statically determine whether that method will be called through reflect. This makes binaries significantly bigger (in my experience, around 30%).
https://github.com/golang/go/blob/go1.23.6/src/cmd/link/internal/ld/deadcode.go#L420-L433

github.com/google/go-cmp uses reflect.Type.Method to generate a string representation of an unnamed interface type (source), which disables this dead code elimination.
Since go-cmp is used by various k8s packages, you can't avoid it when using those dependencies.

To Reproduce
Consider this simple example:

package main

import (
	_ "github.com/aws/aws-sdk-go-v2/service/ec2"
	"github.com/google/go-cmp/cmp"
)

func main() {
	_ = cmp.Equal(1, 2)
    _ = cmd.Diff(1, 2)
}

When building with upstream go-cmp, the binary is 6.6MB.
When using the build tag added on my branch, the binary is 5.1MB.

The binary is 30% bigger when DCE is disabled.

Fixing
I propose two ways of fixing this issue.

  1. The Equal function can be fixed by moving code around, so that the linker can statically determine that the problematic piece of code is not reachable. Avoid disabling dead code elimination when using Equal #374

  2. Adding a build tag to optionally replace the logic of generating a string representation of an unnamed interface, from using reflect.Type.Method() to using reflect.Type.String(). The main issue is that it changes the generated string (in particular it wouldn't follow the qualified argument), and I'm not sure how much of an issue that would be.
    Add a build tag to avoid disabling dead code elimination #375

I think 1. should be done as it fixes the issue by default for users of Equal without any functional change (except an extra space character), and 2. if you're fine with it would allow users who care to also fix Diff (assuming the proposed change is safe and wouldn't break any functionality).

Additional notes
This is somewhat similar to spf13/cobra#2015 for cobra.

@neild
Copy link
Collaborator

neild commented Feb 18, 2025

I'm not sure that we'd want to do anything here. The go-cmp/cmp package is intended for use in tests, and the package documentation is clear on this:

It is intended to only be used in tests, as performance is not a goal [...]

If some other package is depending on it in a non-test context, then that's unfortunate and probably an issue worth raising with that package's maintainers.

Given that performance is explicitly not a goal of this package, adding complexity to improve performance doesn't seem like something we should do.

@pgimalac
Copy link
Author

I actually didn't realize go-cmp was only supposed to be used in tests, I opened #376 to make that clearer in the README of the repo (not just the documentation).

Now to be fair it is clearly not used just for tests, a quick Github search shows that it is used in thousands of non-test files... (including some from Google 😄)

I'm trying to remove the problematic uses of go-cmp in kubernetes so that my binaries can get proper dead code elimination, so hopefully I can fix that without any change in this repo, but I still consider that if you can contribute to making many binaries using go-cmp 30% smaller simply by moving code around and / or adding a build tag to enable it, then it's worth it.
In particular the build tag solution is trivial, it has no change of behavior by default, it just allows anyone who cares to fix this issue...

@dsnet
Copy link
Collaborator

dsnet commented Feb 22, 2025

I'm not fond of introducing any build tags to separate behavior. But I'm more sympathetic to a clean refactor that makes it such that use of Equal-only can exercise more DCE. That said, this assumes that there are many usages of Equal only, which I don't know if we have numbers for.

@pgimalac
Copy link
Author

@dsnet very fair comment, I have no idea how many users just use Equal and not Diff... (and I have no idea how to find out !)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants