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: potential deadcode elimination opportunity of unused methods #38685

Closed
typeless opened this issue Apr 27, 2020 · 10 comments
Closed

cmd/link: potential deadcode elimination opportunity of unused methods #38685

typeless opened this issue Apr 27, 2020 · 10 comments
Labels
binary-size FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@typeless
Copy link

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

$ go version
go version go1.14.2 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
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/user/.cache/go-build"
GOENV="/home/user/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/user"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/home/user/src/golang.org/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/home/user/src/golang.org/go/pkg/tool/linux_amd64"
GCCGO="/usr/bin/gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD=""
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-build436707887=/tmp/go-build -gno-record-gcc-switches"

What did you do?

https://play.golang.org/p/lNId1c5z_Dk

cat <<EOF > hello.go
package main

import (
        "fmt"
        "io"
)

type Bar interface {
        Bark()
        Meow()
}

type Foo struct {
}

func (*Foo) Bark() {
        fmt.Println("Bark")
}
func (*Foo) Meow() {
        fmt.Println("Meow")
}

func baz(v Bar) {
        v.Bark()
}

func main() {
        x := &Foo{}
        baz(x)
}
EOF
$ go build hello.go
$ nm hello | grep Meow
00000000004915c0 T main.(*Foo).Meow
00000000004da690 R main.(*Foo).Meow.stkobj

What did you expect to see?

nm hello | grep Meow

should output nothing.

What did you see instead?

$ nm hello | grep Meow
00000000004915c0 T main.(*Foo).Meow
00000000004da690 R main.(*Foo).Meow.stkobj

Gccgo has the remaining Meow too. But tinygo successfully removes the method.

Related to #6853 and #36313

Also found https://aykevl.nl/2018/12/tinygo-interface, which might be of interest.

@cherrymui
Copy link
Member

We probably could do that. I'm thinking on reworking the linker's live method analysis. I'll keep this in mind.

I think the compiler needs to emit some kind of marker at Bar.Bark interface call, so the linker knows Bark is used. The linker then could track only marked interface methods, instead of all of them as we are doing today.

The question is how frequently this will fire, i.e. how many methods we can eliminate by doing this.

But, most importantly, as a cat person, I oppose removing the Meow method regardless.

@cherrymui cherrymui changed the title potential deadcode elimination opportunity of unused methods cmd/link: potential deadcode elimination opportunity of unused methods Apr 27, 2020
@cherrymui cherrymui added this to the Unplanned milestone Apr 27, 2020
@andybons andybons added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Apr 27, 2020
@typeless
Copy link
Author

Also for the following example, I expect the Fish's Meow and Bark should be removed.

package main

import (
        "fmt"
)

type Bar interface {
        Bark()
        Meow()
}

type Foo struct {
}

func (*Foo) Bar() {
        fmt.Println("Bar")
}

//go:noinline
func (f *Foo) Bark() {
        fmt.Println("Meow")
}

//go:noinline
func (*Foo) Meow() {
        fmt.Println("Meow")
}

type Fish struct {
}

//go:noinline
func (*Fish) Bark() {
        fmt.Println("Fish Bark")
}

//go:noinline
func (*Fish) Meow() {
        fmt.Println("Fish Meow")
}

func baz(v Bar) {
        v.Bark()
}

func main() {
        x := &Foo{}
        y := &Fish{}
        baz(x)
        fmt.Println(x)
        fmt.Println(y)
}
$ nm hello | grep Fish
0000000000491650 T main.(*Fish).Bark
00000000004da760 R main.(*Fish).Bark.stkobj
00000000004916e0 T main.(*Fish).Meow
00000000004da780 R main.(*Fish).Meow.stkobj

@rojer
Copy link

rojer commented Aug 19, 2020

We at u-root are very interested in this.
DCE that eliminates exported non-interface methods saves about 12% for us (and when it stops working our binaries no longer fit into flash chips which kinda sucks).
I expect eliminating unused interface methods to bring similar improvement.
@cherrymui If you can put together a patch I will be happy to test it to give you some numbers.

@cherrymui
Copy link
Member

@rojer Have you tried the tip version of Go? I made some improvements in the dev.link branch which is merged a few days ago. Does that make any difference? Thanks.

@rojer
Copy link

rojer commented Aug 19, 2020

@cherrymui looks like there's been a minor improvement since 1.15:

[rojer@nbd ~/go/src/github.com/u-root/u-root master]$ for GO in ~/go/go1.14.7/bin/go ~/go/go1.15/bin/go ~/go/go-git/bin/go; do $GO version; $GO run github.com/u-root/u-root/tools/makebb -o /tmp/bb cmds/*/* > /dev/null 2>&1; ls -la /tmp/bb; done
go version go1.14.7 linux/amd64
-rwxrwxr-x 1 rojer rojer 17956864 Aug 19 20:11 /tmp/bb
go version go1.15 linux/amd64
-rwxrwxr-x 1 rojer rojer 14299136 Aug 19 20:11 /tmp/bb
go version devel +64350f1eab Wed Aug 19 17:54:18 2020 +0000 linux/amd64
-rwxrwxr-x 1 rojer rojer 14127104 Aug 19 20:11 /tmp/bb

big step down from 1.14 is the unused methods DCE: 1.15 shipped with the fix for #36021

@thanm
Copy link
Contributor

thanm commented Nov 5, 2020

This looks to be fixed on tip:

$ go build barkmeow.go
$ nm barkmeow | fgrep Meow
$ 

Marking this bug closed -- please reopen if you disagree. Thanks.

@thanm thanm closed this as completed Nov 5, 2020
@rojer
Copy link

rojer commented Nov 6, 2020

doesn't seem to work across package boundaries, UnusedInterfaceMethod doesn't get eliminated:

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

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")
}

@rojer
Copy link

rojer commented Nov 6, 2020

@thanm should i file a new issue for this?

@thanm
Copy link
Contributor

thanm commented Nov 6, 2020

Yes, please file a new issue. There are many different flavors/permutations/combinations when it comes to unused method removal; the details matter.

@rojer
Copy link

rojer commented Nov 6, 2020

@thanm #42421

@golang golang locked and limited conversation to collaborators Nov 6, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
binary-size FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

7 participants