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

cmd/link: Unused interface methods don't get gc'd #42421

Closed
rojer opened this issue Nov 6, 2020 · 7 comments
Closed

cmd/link: Unused interface methods don't get gc'd #42421

rojer opened this issue Nov 6, 2020 · 7 comments
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Milestone

Comments

@rojer
Copy link

rojer commented Nov 6, 2020

Follow up to #38685 that seems to have solved a simple case but does not eliminate an unused interface method across packages

What version of Go are you using (go version)?

[rojer@nbd /tmp]$ GOROOT=/home/rojer/go/go-git /home/rojer/go/go-git/bin/go version
go version devel +b7e0adfee2 Fri Nov 6 07:55:52 2020 +0000 linux/amd64

Does this issue reproduce with the latest release?

yes

What operating system and processor architecture are you using (go env)?

go env Output
[rojer@nbd /tmp]$ GOROOT=/home/rojer/go/go-git /home/rojer/go/go-git/bin/go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/rojer/.cache/go-build"
GOENV="/home/rojer/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/home/rojer/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/rojer/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/home/rojer/go/go-git"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/home/rojer/go/go-git/pkg/tool/linux_amd64"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/dev/null"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build351127229=/tmp/go-build -gno-record-gcc-switches"

What did you do?

source:

[rojer@nbd /tmp]$ find foobar/
foobar/
foobar/go.mod
foobar/foo
foobar/foo/foo.go
foobar/bar
foobar/bar/bar.go
[rojer@nbd /tmp]$ cat foobar/foo/foo.go
package main

import (
        "foobar/bar"
)

func main() {
        // Test unused method elimination.
        var b bar.Interface
        b = bar.Bar{}
        b.UsedInterfaceMethod()
}
[rojer@nbd /tmp]$ cat foobar/bar/bar.go 
package bar

import "fmt"

type Interface interface {
        UsedInterfaceMethod()
        UnusedInterfaceMethod()
}

type Bar struct{}

func (Bar) UsedInterfaceMethod() {
        fmt.Println("one")
}

// This method is unused but cannot be eliminated yet.
// https://github.com/golang/go/issues/38685
func (Bar) UnusedInterfaceMethod() {
        fmt.Println("two")
}

// This method is unused and should be eliminated.
func (Bar) UnusedNonInterfaceMethod() {
        fmt.Println("three")
}

What did you expect to see?

foobar/bar.(*Bar).UnusedInterfaceMethod should not make it into the final binary.
in fact, neither should foobar/bar.(*Bar).UsedInterfaceMethod because pointer variant is not used.

in other words, all that should be left from bar.Bar should be bar.Bar.UsedInterfaceMethod.

interesting, that foobar/bar.Bar.UnusedInterfaceMethod does get eliminated but not the bar.*Bar variant.
why is it even generated in the first place?

What did you see instead?

[rojer@nbd /tmp/foobar/foo]$ GOROOT=/home/rojer/go/go-git /home/rojer/go/go-git/bin/go build 
[rojer@nbd /tmp/foobar/foo]$ GOROOT=/home/rojer/go/go-git /home/rojer/go/go-git/bin/go tool nm -size foo | grep foobar/bar
  497080        187 T foobar/bar.(*Bar).UnusedInterfaceMethod
  497140        187 T foobar/bar.(*Bar).UsedInterfaceMethod
  535e20         32 D foobar/bar..inittask
  496fe0        138 T foobar/bar.Bar.UsedInterfaceMethod
  4da288         40 R go.itab.foobar/bar.Bar,foobar/bar.Interface
@cherrymui
Copy link
Member

Thanks. Interesting example.

I think the unused method is linked in because it is referenced from the itab.

If you change foo.go to do

b = interface{}(bar.Bar{}).(bar.Interface)

then UnusedInterfaceMethod disappears.

I agree this is awkward, and we probably could fix the itab case. But it's too late for 1.16. Will try in 1.17. Thanks.

@toothrot toothrot added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Nov 6, 2020
@toothrot toothrot added this to the Backlog milestone Nov 6, 2020
@zephyrtronium
Copy link
Contributor

I'm surprised there is any case in which an exported method is eliminated from a compiled binary. Reflection can reach it without ever naming it: https://play.golang.org/p/zCIdBYzv8zo. I think that's the reason the (Bar) version is eliminated but the (*Bar) version isn't. The non-pointer method is never used, but the pointer-shaped wrapper could be needed through an interface, especially by reflection. (If I'm wrong, please correct me. I'm still learning the internals of cmd/compile.)

@rojer
Copy link
Author

rojer commented Nov 7, 2020

@zephyrtronium unused method elimination is disabled if use of reflection is detected. the approach to dead code elimination is described here.
projects that care about binary size have to avoid using reflection in that way (and we do).

@gopherbot
Copy link

Change https://golang.org/cl/268479 mentions this issue: cmd/compile, cmd/link: use weak reference in itab

@rojer9-fb
Copy link

@cherrymui how are we doing? will this still make 1.17? this is pretty important for size-conscious projects like ours.

@cherrymui
Copy link
Member

Yes, it will make into 1.17. I have CLs above already. I need to rebase and get them in. Thanks.

@rojer9-fb
Copy link

@cherrymui i see it's been approved, any reason it's not merged yet?

@golang golang locked and limited conversation to collaborators Mar 24, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Projects
None yet
Development

No branches or pull requests

6 participants