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.

cobra_test.go

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.

github-actions · 2025-01-25T16:11:17Z

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.

command.go

Signed-off-by: Marc Khouzam <marc.khouzam@gmail.com>

github-actions · 2025-01-25T16:46:49Z

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 · 2025-01-25T17:50:22Z

On MinGW were getting a CI error

START| DeadcodeElimination
| cobra_test.go:291: could not run go tool nm: exit status 1
FAIL | DeadcodeElimination (0.56s)

we could actually just turn that test off for windows.

Signed-off-by: Marc Khouzam <marc.khouzam@gmail.com>

command.go

github-actions · 2025-01-25T21:34:55Z

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 · 2025-01-26T08:12:36Z

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 · 2025-01-26T11:34: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.

Signed-off-by: Marc Khouzam <marc.khouzam@gmail.com>

github-actions · 2025-01-26T12:28:25Z

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

This is what I feel is the correct final version for this PR. @aarzilli can you please look at the changes I pushed on top of yours and confirm you agree?

aarzilli · 2025-01-27T13:42:26Z

The changes look good to me.

artem-anisimov-0x7f · 2025-01-27T14:48:41Z

This is a very good improvement. @marckhouzam, could you please make a new release of cobra?

marckhouzam · 2025-01-27T14:49:14Z

@aarzilli thank you for your patience. This took a very long time but you remained very responsive. Your nice attitude (and great technical expertise) made it possible to get this merged. This should benefit many programs!

P.S. I wouldn't be against adding a section to the Cobra documentation teaching people what they should do to try to get dead-code-elimination working. Although this is more of a general Go concept, it might help people understand it if they have a small section in the Cobra docs. But I leave that up to you, if you still have energy after this looooong review.

marckhouzam · 2025-01-27T14:57:27Z

This is a very good improvement. @marckhouzam, could you please make a new release of cobra?

Nice to see immediate interest 😄 .
I'll work on a release this week.

aarzilli · 2025-01-27T18:03:32Z

Thanks for getting this merged!

sylr · 2025-01-29T16:56:32Z

@jpmcb Could we get a cobra release soon to be able to enjoy this change ?

Follow-up to spf13#1956. This commit allows a program to reset any of the tree templates to their default behaviour, as it was possible to do before the change of spf13#1956. Signed-off-by: Marc Khouzam <marc.khouzam@gmail.com>

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

cobra_test.go Outdated Show resolved Hide resolved

cobra_test.go Outdated Show resolved Hide resolved

cobra_test.go Outdated Show resolved Hide resolved

cobra_test.go Show resolved Hide resolved

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

aarzilli added 3 commits January 25, 2025 08:16

addressing review comments

bed6de4

make string "help" a constant

beec329

make string "help" a constant more

6585f26

marckhouzam force-pushed the dereflect branch from e7c6c98 to a671776 Compare January 25, 2025 16:11

marckhouzam reviewed Jan 25, 2025

View reviewed changes

command.go Outdated Show resolved Hide resolved

Some minor cleanup

5d464d1

Signed-off-by: Marc Khouzam <marc.khouzam@gmail.com>

marckhouzam force-pushed the dereflect branch from a671776 to 5d464d1 Compare January 25, 2025 16:46

Add test

082a365

Signed-off-by: Marc Khouzam <marc.khouzam@gmail.com>

marckhouzam reviewed Jan 25, 2025

View reviewed changes

command.go Outdated Show resolved Hide resolved

disable TestDeadcodeElimination on windows

0f94115

Fix missing recursion

dbc168c

Signed-off-by: Marc Khouzam <marc.khouzam@gmail.com>

marckhouzam force-pushed the dereflect branch from 8fa4656 to dbc168c Compare January 26, 2025 12:28

marckhouzam approved these changes Jan 26, 2025

View reviewed changes

marckhouzam added this to the 1.9.0 milestone Jan 26, 2025

marckhouzam merged commit 611e16c into spf13:main Jan 27, 2025
20 checks passed

marckhouzam mentioned this pull request Feb 2, 2025

Allow to reset the templates to the default #2229

Open

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

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

marckhouzam commented Oct 15, 2024

github-actions bot commented Oct 15, 2024

aarzilli commented Oct 15, 2024

github-actions bot commented Oct 15, 2024

github-actions bot commented Jan 25, 2025

github-actions bot commented Jan 25, 2025

marckhouzam commented Jan 25, 2025

github-actions bot commented Jan 25, 2025

github-actions bot commented Jan 26, 2025

github-actions bot commented Jan 26, 2025

github-actions bot commented Jan 26, 2025

marckhouzam left a comment

Choose a reason for hiding this comment

aarzilli commented Jan 27, 2025

artem-anisimov-0x7f commented Jan 27, 2025

marckhouzam commented Jan 27, 2025

marckhouzam commented Jan 27, 2025

aarzilli commented Jan 27, 2025

sylr commented Jan 29, 2025

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