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

text/template: builtins global map impacts dead code elimination #36021

Open
rojer opened this issue Dec 6, 2019 · 17 comments
Open

text/template: builtins global map impacts dead code elimination #36021

rojer opened this issue Dec 6, 2019 · 17 comments
Milestone

Comments

@rojer
Copy link

@rojer rojer commented Dec 6, 2019

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

1.13.5

Does this issue reproduce with the latest release?

yes

The map of builtin functions that can be invoked from templates is defined as a global variable here.
It references the call function, which uses reflect.Call to invoke a user function. This is one of the conditions that disables dead code elimination in the linker.
At the moment Go is unable to elide unused global maps - see #31704 and long discussion on #2559.

As a result, if anything imports text/template, even transitively, dead code elimination is disabled for entire program.
Consider this simple program: https://github.com/rojer/go-dce-bug
pkg1.MyStruct.Junk method is unused and should be eliminated. However, as described here, DCE is disabled and it is not eliminated:

$ go build && go tool nm go-dce-bug | grep -E '(Junk|unused)'
  4c5710 T github.com/rojer/go-dce-bug/pkg1.(*MyStruct).Junk
  524d20 R github.com/rojer/go-dce-bug/pkg1.(*MyStruct).Junk.stkobj

The reason is, pkg2 imports text/template. the function that references text/template is unused, and even if it were, it does nit use templates as such, just a simple exported function. But since compiler cannot properly elide global map initializers, reflect.Call ends up being marked reachable via text/template.builtins and text/template.call.

Bottom line is, the text/template.builtins map has very high cost to the program and until Go compiler gets better at eliminating unused global variables, its initalization should be removed from package scope and the map should be initialized on first use.

@bradfitz

This comment has been minimized.

Copy link
Contributor

@bradfitz bradfitz commented Dec 6, 2019

How does lazy initialization help?

The linker doesn't care that it's lazily initialized. The link only cares that reflect.Value.Call (or Value.Method) is used anywhere, even lazily, IIRC.

As a result, if anything imports text/template, even transitively, dead code elimination is disabled for entire program.

That's not true.

Only dead code elimination of exported methods.

@rojer

This comment has been minimized.

Copy link
Author

@rojer rojer commented Dec 6, 2019

@bradfitz if the map is only initalized when it's first used, DCE will only be disabled if the program uses templating functions. the problem that currently using any function from text/template is enough, and the code that uses it does not even need to be alive by itself - pkg2.unused in the example project is completely unused, yet it causes text/template.init to be retained, and method DCE to be disabled.
if you want real example of this, see my comment on the issue in u-root - u-root/u-root#1477

Only dead code elimination of exported methods.

this is true, but it's still a lot. for u-root, it's 15% of binary size.

@cagedmantis cagedmantis changed the title text/template: The builtins global map is poisonous text/template: builtins global map is poisonous Dec 6, 2019
@bcmills

This comment has been minimized.

Copy link
Member

@bcmills bcmills commented Dec 6, 2019

This is arguably a defect in the DCE analysis: if the code to initialize the map only writes to that map (and does not mutate other global state as a side-effect), and no live function reads from the map, then both the map itself and the code to initialize it should be considered dead and eliminated.

However, that defect is admittedly not easy to fix in general: it would require DCE to do something akin to escape analysis and/or pure-function detection, and the calls to reflect.ValueOf within addValueFuncs are likely not free of side-effects.

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Dec 6, 2019

Change https://golang.org/cl/210284 mentions this issue: text/template: avoid a global map to help the linker deadcode eliminate

@toothrot toothrot added this to the Backlog milestone Dec 9, 2019
@toothrot toothrot changed the title text/template: builtins global map is poisonous text/template: builtins global map impacts dead code elimination Dec 9, 2019
@rojer

This comment has been minimized.

Copy link
Author

@rojer rojer commented Dec 14, 2019

@bradfitz can you please clarify what release your pending CL will be committed to? i see that it didn't make 1.14, so which will it be?

@bradfitz

This comment has been minimized.

Copy link
Contributor

@bradfitz bradfitz commented Dec 14, 2019

Go 1.15, August 2020.

@rojer

This comment has been minimized.

Copy link
Author

@rojer rojer commented Dec 14, 2019

ugh, that's bad news

@bradfitz

This comment has been minimized.

Copy link
Contributor

@bradfitz bradfitz commented Dec 14, 2019

Just fork the package and fix meanwhile if you need it. That's an easy workaround which is why we don't rush this in.

@rojer

This comment has been minimized.

Copy link
Author

@rojer rojer commented Dec 14, 2019

how do you suggest we do that? i tried the most obvious way - via vendor/text/template, but it won;t build due to use of internal packages:

vendor/text/template/exec.go:10:2: use of internal package internal/fmtsort not allowed
@bradfitz

This comment has been minimized.

Copy link
Contributor

@bradfitz bradfitz commented Dec 14, 2019

Vendor that too.

@rojer

This comment has been minimized.

Copy link
Author

@rojer rojer commented Dec 14, 2019

ok, that works, thanks!

@rojer

This comment has been minimized.

Copy link
Author

@rojer rojer commented Dec 14, 2019

@bradfitz, actually, it doesn't. or rather, no always - it's weird.

  • when i use text/template directly from my package, i.e. import "text/template", then my packae's vendor directory is used
  • when text/template is used indirectly through a standard library package, i.e. via import of go/doc, then my vendored package is not used.

simplest test case, main.go:

package main

import (
// _ "go/doc"
// _ "text/template"
)

func main() {
}

i have test/template and internal/fmtsort copied into y local vendor directory.
i moved aside $GOROOT/src/text/template to make the failure obvious.
so when i directly import text/template, it works and my package builds:

[rojer@nbd ~/go/src/github.com/rojer/foo]$ cat main.go 
package main

import (
        //      "go/doc"
        _ "text/template"
)

func main() {
}
[rojer@nbd ~/go/src/github.com/rojer/foo]$ GOROOT=/home/rojer/goroot GOPATH=/home/rojer/go /home/rojer/goroot/bin/go build -v
github.com/rojer/foo/vendor/internal/fmtsort
github.com/rojer/foo/vendor/text/template/parse
github.com/rojer/foo/vendor/text/template
github.com/rojer/foo

however, if i import go/doc, it no longer does:

rojer@nbd ~/go/src/github.com/rojer/foo]$ cat main.go 
package main

import (
        "go/doc"
        // _ "text/template"
)

func main() {
}
[rojer@nbd ~/go/src/github.com/rojer/foo]$ GOROOT=/home/rojer/goroot GOPATH=/home/rojer/go /home/rojer/goroot/bin/go build -v
/home/rojer/goroot/src/go/doc/comment.go:14:2: cannot find package "text/template" in any of:
        /home/rojer/goroot/src/vendor/text/template (vendor tree)
        /home/rojer/goroot/src/text/template (from $GOROOT)
        /home/rojer/go/src/text/template (from $GOPATH)

looks like when importing from standard library a different vendor tree is used.

@bcmills

This comment has been minimized.

Copy link
Member

@bcmills bcmills commented Dec 16, 2019

Yes, the standard library uses the vendor tree in GOROOT/src/vendor.

If you need to patch the standard library, patch the standard library itself (in GOROOT/src/text/template).

Or, if you want to substitute text/template in a way that can be built with a stock go toolchain and standard library, give the fork a unique import path and update the import statements within your program to refer to that path.

@rojer

This comment has been minimized.

Copy link
Author

@rojer rojer commented Dec 17, 2019

@bcmills patching standard library is what we're trying to avoid, of course, since it's an inconvenience to users.

changing imports is not an option since what gets us to text/template is the indirect dependency via go/build and then go/doc.
trying to vendor go/build, go/doc, text/template and all their dependencies brings too many internals with it. i can get it to build with 1.13, but it goes up in flames if compiled with 1.12.

there just isn't a good way to override even small parts of stdlib, it seems.
is there a chance we could get CL 210284 in sooner? this is all that we need to realize significant size gains for our binary.
alternatively, perhaps adding a mechanism for adding a higher priority vendoring path for stdlib could be considered, that way we could vendor just the text/template package and not the other stuff.

rojer pushed a commit to rojer/u-root that referenced this issue Dec 17, 2019
In order for DCE to work we need this patch for text/template:
https://go-review.googlesource.com/c/go/+/210284/

It is already approved and will go into Go 1.15.
Until ten we will have this nasty hack.

Our dependency on text/template actually goes via `go/build` and `go/doc`,
so those need to be vendored too because vendor lookup directory
is different when importing from stdlib (golang/go#36021 (comment)).

Files in this commit come from Go 1.13.5, latest stable release at the time.

`vendor_stdlib.sh` is the script that should make refreshing this easy.

Signed-off-by: Deomid "rojer" Ryabkov <rojer9@fb.com>
@rojer

This comment has been minimized.

Copy link
Author

@rojer rojer commented Dec 17, 2019

fwiw, rojer/u-root@e84d5ba is what i got working on 1.13 but it completely explodes on earlier versions and will probably do so with 1.14 as well, making is a nightmare to maintain.

@bcmills

This comment has been minimized.

Copy link
Member

@bcmills bcmills commented Dec 17, 2019

trying to vendor go/build, go/doc, text/template and all their dependencies brings too many internals with it.

Wait, how are you ending up with go/build and go/doc as dependencies of a size-sensitive binary? I would be surprised if that were at all viable even after text/template is resolved.

@rojer

This comment has been minimized.

Copy link
Author

@rojer rojer commented Dec 17, 2019

Wait, how are you ending up with go/build and go/doc as dependencies of a size-sensitive binary?

installcommand

I would be surprised if that were at all viable even after text/template is resolved.

after a bunch of other changes, this really is the last thing that is keeping us from saving 3M out of 18.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants
You can’t perform that action at this time.