Allow linker to perform deadcode elimination for program using Cobra #1956

aarzilli · 2023-05-05T11:49:51Z

Fixes #2015

Cobra, in its default configuration, will execute a template to generate help, usage and version outputs. Text/template execution calls MethodByName and MethodByName disables dead code elimination in the Go linker, therefore all programs that make use of cobra will be linked with dead code elimination disabled, even if they end up replacing the default usage, help and version formatters with a custom function and no actual text/template evaluations are ever made at runtime.

Dead code elimination in the linker helps reduce disk space and memory utilization of programs. For example, for the simple example program used by TestDeadcodeElimination 40% of the final executable size is dead code. For a more realistic example, 12% of the size of Delve's executable is deadcode.

This PR changes Cobra so that, in its default configuration, it does not automatically inhibit deadcode elimination by:

changing Cobra's default behavior to emit output for usage and help using simple Go functions instead of template execution
quarantining all calls to template execution into SetUsageTemplate, SetHelpTemplate and SetVersionTemplate so that the linker can statically determine if they are reachable

CLAassistant · 2023-05-05T11:49:56Z

All committers have signed the CLA.

github-actions · 2023-05-05T11:50:10Z

This PR exceeds the recommended size of 200 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

aarzilli · 2023-05-05T14:53:00Z

The CLA thing isn't working for some reason.

github-actions · 2023-05-05T20:14:54Z

This PR exceeds the recommended size of 200 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

marckhouzam · 2023-06-12T12:39:11Z

Thanks @aarzilli !
I like the idea of optimizing things.
This will require some thought so I can understand clearly the behaviour change. Thanks for adding a test it should help with this.

aarzilli · 2023-06-12T12:51:37Z

Unless I made a mistake there shouldn't be any behavior changes (as in, observable from the outside). Happy to explain anything about this if needed.

aarzilli · 2023-07-28T07:01:10Z

Ping?

aarzilli · 2023-10-10T09:33:54Z

Ping?

github-actions · 2024-02-10T13:44:09Z

This PR exceeds the recommended size of 200 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

github-actions · 2024-04-10T08:01:12Z

This PR exceeds the recommended size of 200 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

github-actions · 2024-05-10T14:50:30Z

This PR exceeds the recommended size of 200 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

github-actions · 2024-06-13T10:01:15Z

This PR exceeds the recommended size of 200 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

github-actions · 2024-08-10T11:31:30Z

This PR exceeds the recommended size of 200 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

marckhouzam · 2024-10-14T01:58:22Z

@aarzilli I apologize for such a long delay, but I had misunderstood the value of this PR. I now realize that it may help all programs using Cobra, so I'm very interested.

I'm trying to convince myself that that this change actually has an impact.
Would you be able to explain how I can see the difference in a program that has dead code, before and after this PR?

Here is what I tried.

I used the program you have added to the tests in this PR
I added an "Unused()" function that I don't call in the program.
I ran go build -o myprog . (using cobra 1.8.1 without your PR)
I ran go tool nm ./myprog | grep Unused

I would have expected to see the Unused function but I don't.
I used go version go1.22.8 darwin/arm64

Could you clarify what I should expect?

This is the program:

package main

import (
	"fmt"
	"os"

	"github.com/spf13/cobra"
)

func Unused() {
	fmt.Println("not called")
}

var rootCmd = &cobra.Command{
	Version: "1.0",
	Use:     "example_program",
	Short:   "example_program - test fixture to check that deadcode elimination is allowed",
	Run: func(cmd *cobra.Command, args []string) {
		fmt.Println("hello world")
	},
	Aliases: []string{"alias1", "alias2"},
	Example: "stringer --help",
}

func main() {
	if err := rootCmd.Execute(); err != nil {
		fmt.Fprintf(os.Stderr, "Whoops. There was an error while executing your CLI '%s'", err)
		os.Exit(1)
	}
}

aarzilli · 2024-10-14T13:18:20Z

One easy way to see the impact of this change is to compile your example program with and without this PR:

$ ls -lh
total 8.4M
-rwxr-xr-x. 1 a a 5.2M Oct 14 14:45 example.withoutpr*
-rwxr-xr-x. 1 a a 3.2M Oct 14 14:45 example.withpr*
-rw-r--r--. 1 a a  596 Oct 14 14:44 main.go

This is an extreme example, where the size of the executable is reduced by 38%, but reductions of 10% are realistic.
I used go1.23 but you should get very similar results with go1.22.8.

As to your question, saying that deadcode elimination gets disabled is incorrect: in reality it continues to function but in a reduced capacity. Specifically if reflect.Value.MethodByName or reflect.Value.Method are reachable it can not always prove that exported methods are unreachable.

Your Unused function is not an exported method so, unless it is called by an exported method, it can always be deadcode eliminated.

If you want to see the Unused function make it all the way to the executable you have to do something like this:

package main

import (
	"fmt"
	"os"

	"github.com/spf13/cobra"
)

type Astruct struct {
	n int
}

//go:noinline
func Unused() {
	fmt.Println("not called")
}

//go:noinline
func (a *Astruct) Unused() {
	fmt.Println("not called", a.n)
	Unused()
}

//go:noinline
func (a *Astruct) Used() {
	fmt.Println("used", a.n)
}

var rootCmd = &cobra.Command{
	Version: "1.0",
	Use:     "example_program",
	Short:   "example_program - test fixture to check that deadcode elimination is allowed",
	Run: func(cmd *cobra.Command, args []string) {
		fmt.Println("hello world")
	},
	Aliases: []string{"alias1", "alias2"},
	Example: "stringer --help",
}

func main() {
	if err := rootCmd.Execute(); err != nil {
		fmt.Fprintf(os.Stderr, "Whoops. There was an error while executing your CLI '%s'", err)
		os.Exit(1)
	}
	var a Astruct
	fmt.Println(a)
	a.Used()
}

All of the noinline directives are needed because small functions like this would be removed by the inliner and the fmt.Println call is needed to make the Astruct type itself reachable (if all calls to the methods of Astruct are static the linker can still prove that Unused is unreachable).

With these changes you get:

$ go build main2.go && go tool nm ./main2 | grep 'Unused'
  589c00 T main.(*Astruct).Unused
  589ba0 T main.Unused
  765570 D runtime.adviseUnused
  4179e0 T runtime.sysUnusedOS

without the PR and:

$ go build main2.go && go tool nm ./main2 | grep 'Unused'
  602564 D runtime.adviseUnused
  4144a0 T runtime.sysUnusedOS

with the PR.

marckhouzam · 2024-10-15T03:28:15Z

Thanks for the explanation @aarzilli, I can now see the benefits.

So, with this PR, programs that don't set new templates (usage, help or version) will be able to do dead code elimination fully.

What about programs that do modify those templates? If they want to get full usage of dead code elimination they should convert their template use to a go function like you have done, IIUC? My next step for this review is to try to override the help/usage/version in that fashion and see if it works as expected.

I believe that using SetHelpFunc() and SetUsageFunc() will allow programs to do this, but I don't think we have a way to do this for the version template. But it is ok, it is still better than not being able to do it at all. Once this is merged, we can discuss adding a SetVersionFunc().

marckhouzam

This looks great.
I still want to do some testing, but I don't expect any big surprises.
Here are some comments for the PR.
The PR also needs to be rebased.

Thanks again for your patience, I think we can get this in soon.

marckhouzam · 2024-10-14T01:18:19Z

cobra_test.go

+		dirname  = "test_deadcode"
+		progname = "test_deadcode_elimination"
+	)
+	os.Mkdir(dirname, 0770)


I'm getting a warning here.
Can you change line to _ = os.Mkdir(dirname, 0770)

marckhouzam · 2024-10-14T01:18:41Z

cobra_test.go

+		os.Exit(1)
+	}
+}
+`), 0660)


I'm getting a warning here.
Can you change the permissions to 0600

marckhouzam · 2024-10-15T02:40:12Z

cobra_test.go

+	if err != nil {
+		t.Fatalf("could not write test program: %v", err)
+	}
+	defer os.RemoveAll(dirname)


Can you move this defer to right after the os.Mkdir call to guarantee it gets called even if the t.Fatalf above gets triggered.

marckhouzam · 2024-10-15T02:54:00Z

cobra_test.go

@@ -222,3 +226,57 @@ func TestRpad(t *testing.T) {
 		})
 	}
 }
+


Can you add a comment above this test explaining that deadcode elimination is reduced when MethodByName is present which will affect all programs using cobra; so this test make sure we don't introduce code that depends on MethodByName().

Also please put a comment with the URL of this PR.

See if you like what I wrote.

marckhouzam · 2024-10-15T03:34:55Z

What do you think about updating the documentation in the three sections starting here https://github.com/spf13/cobra/blob/main/site/content/user_guide.md#defining-your-own-help to explain the side-effect of overriding the template, and therefore that it is recommended to set a function instead?

github-actions · 2024-10-15T08:19:54Z

This PR exceeds the recommended size of 200 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

aarzilli · 2024-10-15T08:24:21Z

Thanks for the explanation @aarzilli, I can now see the benefits.

So, with this PR, programs that don't set new templates (usage, help or version) will be able to do dead code elimination fully.

What about programs that do modify those templates? If they want to get full usage of dead code elimination they should convert their template use to a go function like you have done, IIUC?

Yes, this is correct.

I believe that using SetHelpFunc() and SetUsageFunc() will allow programs to do this, but I don't think we have a way to do this for the version template. But it is ok, it is still better than not being able to do it at all. Once this is merged, we can discuss adding a SetVersionFunc().

It imagine should be trivial.

github-actions · 2024-10-15T08:33:47Z

This PR exceeds the recommended size of 200 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

marckhouzam · 2024-11-24T12:41:02Z

The linter has failed

I looked in detail into this and it makes sense.
The problem is that in reality, so many projects use templates, that I wonder if any real program will benefit from this improvement?

For example, programs that use some k8s libraries will still end up using templates. I’ve tested this with both tanzu and helm and the binary size didn’t change.

So I was just hesitant in accepting this because it complicates the Cobra code base and prevents any future use of templates. But it also feels wrong to force any program using cobra to have a dependency on templates.

@aarzilli maybe you can convince me. Did you say Delve would be able to reduce its size with this change?

dominiquelefevre · 2024-12-05T20:43:40Z

The problem is that in reality, so many projects use templates, that I wonder if any real program will benefit from this improvement?

At my company, most of services see a reduction of their size by 40% to 45% when DCE is enabled.

For example, programs that use some k8s libraries will still end up using templates

As for k8s, most of it depends on text/template only indirectly via x/net/trace. Its package init func registers some complex HTTP handlers that use templating. That causes other problems as well, and I think that fixing the init will fix the dependency for k8s, too. See golang/go#69951.

I think that k8s should not influence the design of other libs. There is a huge go ecosystem outside k8s.

Cobra, in its default configuration, will execute a template to generate help, usage and version outputs. Text/template execution calls MethodByName and MethodByName disables dead code elimination in the Go linker, therefore all programs that make use of cobra will be linked with dead code elimination disabled, even if they end up replacing the default usage, help and version formatters with a custom function and no actual text/template evaluations are ever made at runtime. Dead code elimination in the linker helps reduce disk space and memory utilization of programs. For example, for the simple example program used by TestDeadcodeElimination 40% of the final executable size is dead code. For a more realistic example, 12% of the size of Delve's executable is deadcode. This PR changes Cobra so that, in its default configuration, it does not automatically inhibit deadcode elimination by: 1. changing Cobra's default behavior to emit output for usage and help using simple Go functions instead of template execution 2. quarantining all calls to template execution into SetUsageTemplate, SetHelpTemplate and SetVersionTemplate so that the linker can statically determine if they are reachable

github-actions · 2024-12-06T16:02:29Z

This PR exceeds the recommended size of 200 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

aarzilli · 2024-12-06T16:24:37Z

@aarzilli maybe you can convince me. Did you say Delve would be able to reduce its size with this change?

Delve uses templates (and reflection) in various places but cobra is the only place where it couldn't be easily circumvented.

I think you are right that any given large Go program is likely to end up having multiple different dependencies on reflection but cobra itself is not large, nor is it only recommended for large programs.

You say "prevents any future use of templates" but I don't think it's true: first if they aren't statically reachable they are allowed, secondly this decision could be reversed if a compelling use case for templates was found.

aarzilli force-pushed the dereflect branch from 99d7f09 to de6570f Compare May 5, 2023 20:14

aarzilli mentioned this pull request May 16, 2023

cmd/link: there should be a way to determine why deadcode elimination was not performed golang/go#60221

Closed

aarzilli force-pushed the dereflect branch from de6570f to 8159848 Compare February 10, 2024 13:43

aarzilli force-pushed the dereflect branch from 8159848 to a9601b1 Compare April 10, 2024 08:00

aarzilli force-pushed the dereflect branch from a9601b1 to b06beaa Compare May 10, 2024 14:50

aarzilli force-pushed the dereflect branch from b06beaa to e94761a Compare June 13, 2024 10:00

aarzilli force-pushed the dereflect branch from e94761a to ee7aba8 Compare August 10, 2024 11:31

marckhouzam changed the title ~~Restructure code to let linker perform deadcode elimination step~~ Allow linker to perform deadcode elimination for program using Cobra Oct 14, 2024

marckhouzam reviewed Oct 15, 2024

View reviewed changes

aarzilli force-pushed the dereflect branch from ee7aba8 to 4da4d97 Compare October 15, 2024 08:19

marckhouzam mentioned this pull request Nov 24, 2024

The use of text/template disables dead code elimination in all users of cobra #2015

Open

aarzilli added 3 commits December 6, 2024 16:39

addressing review comments

5e44e3e

make string "help" a constant

45cac89

aarzilli force-pushed the dereflect branch from 84ffa30 to 45cac89 Compare December 6, 2024 16:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow linker to perform deadcode elimination for program using Cobra #1956

Allow linker to perform deadcode elimination for program using Cobra #1956

aarzilli commented May 5, 2023 •

edited by marckhouzam

Loading

CLAassistant commented May 5, 2023 •

edited

Loading

github-actions bot commented May 5, 2023

aarzilli commented May 5, 2023

github-actions bot commented May 5, 2023

marckhouzam commented Jun 12, 2023

aarzilli commented Jun 12, 2023

aarzilli commented Jul 28, 2023

aarzilli commented Oct 10, 2023

github-actions bot commented Feb 10, 2024

github-actions bot commented Apr 10, 2024

github-actions bot commented May 10, 2024

github-actions bot commented Jun 13, 2024

github-actions bot commented Aug 10, 2024

marckhouzam commented Oct 14, 2024 •

edited

Loading

aarzilli commented Oct 14, 2024 •

edited

Loading

marckhouzam commented Oct 15, 2024

marckhouzam left a comment

marckhouzam Oct 14, 2024

aarzilli Oct 15, 2024

marckhouzam Oct 14, 2024

aarzilli Oct 15, 2024

marckhouzam Oct 15, 2024

aarzilli Oct 15, 2024

marckhouzam Oct 15, 2024

aarzilli Oct 15, 2024

marckhouzam commented Oct 15, 2024

github-actions bot commented Oct 15, 2024

aarzilli commented Oct 15, 2024

github-actions bot commented Oct 15, 2024

marckhouzam commented Nov 24, 2024

dominiquelefevre commented Dec 5, 2024

github-actions bot commented Dec 6, 2024

aarzilli commented Dec 6, 2024

@@ @@ -222,3 +226,57 @@ func TestRpad(t *testing.T) { @@
               		})
               	}
               }

Allow linker to perform deadcode elimination for program using Cobra #1956

Are you sure you want to change the base?

Allow linker to perform deadcode elimination for program using Cobra #1956

Conversation

aarzilli commented May 5, 2023 • edited by marckhouzam Loading

CLAassistant commented May 5, 2023 • edited Loading

github-actions bot commented May 5, 2023

aarzilli commented May 5, 2023

github-actions bot commented May 5, 2023

marckhouzam commented Jun 12, 2023

aarzilli commented Jun 12, 2023

aarzilli commented Jul 28, 2023

aarzilli commented Oct 10, 2023

github-actions bot commented Feb 10, 2024

github-actions bot commented Apr 10, 2024

github-actions bot commented May 10, 2024

github-actions bot commented Jun 13, 2024

github-actions bot commented Aug 10, 2024

marckhouzam commented Oct 14, 2024 • edited Loading

aarzilli commented Oct 14, 2024 • edited Loading

marckhouzam commented Oct 15, 2024

marckhouzam left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marckhouzam commented Oct 15, 2024

github-actions bot commented Oct 15, 2024

aarzilli commented Oct 15, 2024

github-actions bot commented Oct 15, 2024

marckhouzam commented Nov 24, 2024

dominiquelefevre commented Dec 5, 2024

github-actions bot commented Dec 6, 2024

aarzilli commented Dec 6, 2024

aarzilli commented May 5, 2023 •

edited by marckhouzam

Loading

CLAassistant commented May 5, 2023 •

edited

Loading

marckhouzam commented Oct 14, 2024 •

edited

Loading

aarzilli commented Oct 14, 2024 •

edited

Loading