Dead code elimination partially disabled

[seanamos]

urfave/cli directly uses text/template, and unfortunately this partially disables dead code elimination. However, "partial" in this context can result in significantly larger binaries depending on how many other dependencies you have. Full DCE can result in significant binary size reductions (50%+-).

Cobra recently added a fix/workaround for a similar issue:
spf13/cobra#1956

I appreciate there may not be a non-breaking way to solve this in v3.

[seanamos]

See also the relevant discussion here: golang/go#72895

[seanamos]

I did a quick experiment (really hacked together) to demonstrate how significant this is.

In the empty hello world example, the binary size dropped to 1.3mb from 2.0mb

Usin my local fork in a more real world project with a binary size of 33mb, the size dropped to 8mb.

The approach I used? Swap out the usage of text/template for Datadog's fork: https://github.com/DataDog/datadog-agent/tree/main/pkg/template/text

The Datadog fork is a drop in replacement for stdlib, with one major difference, it does not use reflect.MethodByName to avoid disabling DCE. Datadog have run into the exact same problem: https://www.datadoghq.com/blog/engineering/agent-go-binaries/

This means you cannot call methods from within templates, FullName(), VisibleFlags() etc. .
I copied the necessary data from the Command into another struct, with no methods, and pass that as the template data.

	tdata = templateCommand{
		ArgsUsage:              cmd.ArgsUsage,
		Authors:                cmd.Authors,
		Category:               cmd.Category,
		Copyright:              cmd.Copyright,
		Description:            cmd.Description,
		Flags:                  cmd.Flags,
		FullName:               cmd.FullName(),
		HideVersion:            cmd.HideVersion,
		Name:                   cmd.Name,
		Names:                  cmd.Names(),
		Usage:                  cmd.Usage,
		UsageText:              cmd.UsageText,
		Version:                cmd.Version,
		VisibleCategories:      tvcats,
		VisibleCommands:        tvcmds,
		VisibleFlags:           cmd.VisibleFlags(),
		VisibleFlagCategories:  tvflagcats,
		VisiblePersistentFlags: cmd.VisiblePersistentFlags(),
		Arguments:              targs,
	}

	tracef("executing template")
	handleTemplateError(t.Execute(w, tdata))

There are some other changes, but that was the general idea.

[abitrolly]

It looks like this should be in Go stdlib.

[abitrolly]

Could this help with eliminating completions code that is not used? #2291

[seanamos]

It looks like this should be in Go stdlib.

Ideally yes. Currently the discussions there revolve around:

1. Introducing a new template function, ExecuteLite, that would not do reflection.
2. A new build tag to disable reflection in templating (runtime breakage likely).
3. Try do some compiler wizardry with templating to keep DCE but ensure template method calls still work.

I speculate 1 is most likely since it is the most practical and backwards compatible way to go about it. That said, any change on the stdlib/compiler level is likely to be stuck in limbo for a long time, probably years, while a large part of the Go ecosystem suffers from DCE being disabled with common imports.

[abitrolly]

Introducing a new template function, ExecuteLite, that would not do reflection.

I would just call it Render. ExecuteLite doesn't really show what does it do. If a breakage is expected, maybe it is possible to introduce a stripped down alternative from most popular libs like https://github.com/mitsuhiko/minijinja/tree/main/minijinja-go

EDIT: Link to Go project.

[seanamos]

This is a well-known Go binary size issue. The text/template and html/template packages import a lot of functionality for safety, which pulls in significant code.

It's not the size of the packages, but the use of reflect.MethodByName in those packages which is the issue. This disables method dead code elimination. This means instead of only used methods being compiled into your binary, all methods are. This is especially bad if you have quite a few dependencies.

If you use reflect.MethodByName directly in your code without using text/template or html/template, you get the same result. text/template and html/template are just a common and unexpected way that reflect.MethodByName usage finds its way into your code.

For a workaround in the meantime, you can use ldflags to strip unused code during build:

go build -ldflags="-s -w"

The -s flag removes the symbol table and -w removes DWARF debugging info. This typically reduces binary size by 20-30%.

It's a good tip in general for binary size reduction, but my example with one of our binaries shrinking from 33mb to 8mb was already using the -s -w ldflags.

[ldez]

We can think the problem differently.

The problem is related to DefaultPrintHelpCustom because of the usage of text/template.

But in fact DefaultPrintHelpCustom is problematic because it is used by the global variable HelpPrinterCustom.
A global variable cannot be "bypassed" by the lib user even with an init function.
The global variables are always initiated first (global variables are 99% of time a problem inside a lib).

If you modify the global variable HelpPrinterCustom, inside urfave/cli, with the following code, the problem is gone.

var HelpPrinterCustom HelpPrinterCustomFunc = func(w io.Writer, templ string, data any, customFunc map[string]any) {}

This change is, obviously breaking, and doesn't offer alternative by default for generating the help.
A user should set up the global var with DefaultPrintHelpCustom, but it this case the DCE problem is coming back.

We can think to a way to provide the help statically, but the current signature of HelpPrinterCustomFunc is too "low level" to be used for that.

The problem is not simple (IMHO impossible) to solve without breaking changes because of the usage of a global variable.

[seanamos]

@ldez I agree, I don't see a way to fix this without a breaking change.
Even with my approach, which doesn't change the public surface area of the lib, it would still be a behavior breaking change.

[ldez]

I think there is maybe a way by using a build tag with my idea: using a build tag notemplate to isolate the global variable HelpPrinterCustom can work.

Based on that you can use your approach inside your project and creating a custom HelpPrinterCustomFunc.

This is non-breaking, but this is not really a proper way to handle the problem, this is more a temporarily fix before a v4.

[ldez]

🤔 still with a build tag notemplate where HelpPrinterCustom is an empty function: the global variables ShowCommandHelp and ShowRootCommandHelp can be overridden.

These 2 global variables are functions:

• ShowRootCommandHelp is dedicated for the root command, so it can be replace with static help
• ShowCommandHelp takes the parent command and the name of the current command, so it also can be replaced with static help

So this approach can work by using your approach based on a fork of text/template inside your project and with my idea of generating static content for the help.

WDYT?

[ldez]

Edit: we can ignore it, the problem was related to something else, so my proposal based on build tag still works.

Sadly, there is another usage of text/template inside fish.go

The solution can still works but the fish completion must be rewritten without a template or with a lax approach.