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

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

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

rojer opened this issue Dec 6, 2019 · 17 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@rojer
Copy link

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
Copy link
Contributor

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
Copy link
Author

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
Copy link
Contributor

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
Copy link

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

@toothrot toothrot added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Dec 9, 2019
@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
Copy link
Author

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
Copy link
Contributor

Go 1.15, August 2020.

@rojer
Copy link
Author

rojer commented Dec 14, 2019

ugh, that's bad news

@bradfitz
Copy link
Contributor

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
Copy link
Author

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
Copy link
Contributor

Vendor that too.

@rojer
Copy link
Author

rojer commented Dec 14, 2019

ok, that works, thanks!

@rojer
Copy link
Author

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
Copy link
Contributor

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
Copy link
Author

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
Copy link
Author

rojer commented Dec 17, 2019

fwiw, https://github.com/rojer/u-root/commit/e84d5ba1d54aa62379c038ff9a153894f3db5b55 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
Copy link
Contributor

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
Copy link
Author

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.

bradfitz added a commit to tailscale/go that referenced this issue Apr 15, 2020
…nation

Fixes golang#36021
Updates golang#2559
Updates golang#26775

Change-Id: I2e6708691311035b63866f25d5b4b3977a118290
Reviewed-on: https://go-review.googlesource.com/c/go/+/210284
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Reviewed-by: Rob Pike <r@golang.org>
@golang golang locked and limited conversation to collaborators Apr 15, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
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

5 participants