fix deadcode disabling in Go by tonistiigi · Pull Request #30 · modern-go/reflect2

tonistiigi · 2024-07-09T01:16:03Z

Avoid embedding reflect.Type and its Method() method because exposing it disables deadcode removal in linker.

https://tip.golang.org/src/cmd/link/internal/ld/deadcode.go#L395

Avoid embedding reflect.Type and its Method() method because exposing it disables deadcode removal in linker. Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>

thaJeztah · 2025-03-21T19:06:57Z

@taowen PTAL 🙏

ikonst · 2025-03-22T23:51:15Z

@tonistiigi Are you sure it's meaningful? Can you suggest a simple sample main.go importing this package such that go build -ldflags=-dumpdep main.go |& whydeadcode produces output?

tonistiigi · 2025-03-23T02:20:36Z

@ikonst

» cat go.mod| grep reflect2
	github.com/modern-go/reflect2 v1.0.2 // indirect
» go build -ldflags=-dumpdep ./cmd/buildx 2>&1 | ~/go/bin/whydeadcode                                                                                                                                
github.com/modern-go/reflect2.UnsafeSliceType.Method reachable from:
	 type:github.com/modern-go/reflect2.UnsafeSliceType
	 github.com/modern-go/reflect2.newUnsafeSliceType
	 github.com/modern-go/reflect2.(*frozenConfig).wrapType
	 github.com/modern-go/reflect2.(*frozenConfig).Type2
	 github.com/modern-go/reflect2.(*frozenConfig).TypeOf
	 github.com/json-iterator/go.init
	 github.com/json-iterator/go..inittask
	 go:main.inittasks
	 _

alternatively sometimes

github.com/modern-go/reflect2.(*UnsafeSliceType).MethodByName reachable from:
	 type:*github.com/modern-go/reflect2.UnsafeSliceType
	 type:github.com/json-iterator/go.sliceEncoder
	 github.com/json-iterator/go.encoderOfSlice
	 github.com/json-iterator/go._createEncoderOfType
	 github.com/json-iterator/go.createEncoderOfType
	 github.com/json-iterator/go.encoderOfType
	 github.com/json-iterator/go.describeStruct
	 github.com/json-iterator/go.decoderOfStruct
	 github.com/json-iterator/go._createDecoderOfType
	 github.com/json-iterator/go.createDecoderOfType
	 github.com/json-iterator/go.decoderOfType
	 github.com/json-iterator/go.(*frozenConfig).DecoderOf
	 github.com/json-iterator/go.(*Iterator).ReadVal
	 github.com/json-iterator/go.(*Iterator).Read.func2
	 github.com/json-iterator/go.(*Iterator).Read
	 github.com/json-iterator/go.(*frozenConfig).validateJsonRawMessage.func1
	 github.com/json-iterator/go.(*frozenConfig).validateJsonRawMessage
	 github.com/json-iterator/go.Config.Froze
	 github.com/json-iterator/go.init
	 github.com/json-iterator/go..inittask
	 go:main.inittasks
	 _

» cat go.mod| grep reflect2
require github.com/modern-go/reflect2 v1.0.3-0.20250322232337-35a7c28c31ee // indirect

» go build -ldflags=-dumpdep ./cmd/buildx 2>&1 | ~/go/bin/whydeadcode
(no traces via reflect2)

ikonst · 2025-03-23T07:34:34Z

@tonistiigi yes, but can you provide a minimal main.go?

From whydeadcode README:

Because of how -dumpdep works only the first result output by whydeadcode is real. Because of how -dumpdep works anything beyond the first result can be a false positive (i.e. things that look like they will affect deadcode elimination but won't if the first result is taken care of) and it can also have false negatives (i.e. things that will continue to keep deadcode elimination disabled if the first result is taken care of).

I saw a similar "problem" in another package but when I tried to reproduce it in isolation, it wasn't a problem anymore.

tonistiigi · 2025-03-24T01:01:12Z

yes, but can you provide a minimal main.go?

I don't have time to work on this. What I can show you is that this does have a significant effect on the build above.

» ls -l buildx                                                                                                                                                                                       
-rwxr-xr-x  1 tonistiigi  wheel  95985362 Mar 23 00:56 buildx
» ls -l buildx                                                                                                                                                                                      
-rwxr-xr-x  1 tonistiigi  wheel  65406946 Mar 23 00:55 buildx

The difference of these builds is only the patch in this PR (you can't repro this as more fixes are needed, most notably awful text/template but other patches were already part of the first build).

Because of how -dumpdep works anything beyond the first result can be a false positive (i.e. things that look like they will affect deadcode elimination but won't if the first result is taken care of) and it can also have false negatives (i.e. things that will continue to keep deadcode elimination disabled if the first result is taken care of).

This is true, but the current case is not a false positive. It is entirely possible that there are other cases that mess up deadcode in this package that this PR does not fix. I only followed the call path in buildx. The output of whydeadcode is also more complex than defined here. It can show call paths where MethodByName() is called with the constant argument as the first item but these are really not that problematic. In debugging you would need to look at the first codepath that doesn't use a constant argument.

pgimalac · 2025-03-24T09:41:42Z

Definitely not a minimal example, but I can confirm updating to the merged commit fixes it for the https://github.com/DataDog/datadog-agent binaries, see DataDog/datadog-agent#32527 (comment) for the size diff (note that the PR is a hacky one where I just comment out the uses of template 😄)

Without the update, the output of whydeadcode is

Details

github.com/modern-go/reflect2.(*safeType).MethodByName reachable from:
	 type:*github.com/modern-go/reflect2.safeType
	 type:github.com/modern-go/reflect2.safeType
	 github.com/modern-go/reflect2.(*frozenConfig).wrapType
	 github.com/modern-go/reflect2.(*frozenConfig).Type2
	 github.com/modern-go/reflect2.(*frozenConfig).TypeOf
	 github.com/json-iterator/go.(*frozenConfig).validateJsonRawMessage
	 github.com/json-iterator/go.Config.Froze
	 github.com/DataDog/datadog-agent/pkg/util/kubernetes/kubelet.newPodUnmarshaller
	 github.com/DataDog/datadog-agent/pkg/util/kubernetes/kubelet.NewKubeUtil
	 github.com/DataDog/datadog-agent/pkg/util/kubernetes/kubelet.GetKubeUtilWithRetrier
	 github.com/DataDog/datadog-agent/pkg/util/kubernetes/kubelet.GetKubeUtil
	 github.com/DataDog/datadog-agent/pkg/util/kubernetes/hostinfo.NewNodeInfo
	 github.com/DataDog/datadog-agent/pkg/util/kubernetes/clustername.getClusterName
	 github.com/DataDog/datadog-agent/pkg/util/kubernetes/clustername.GetClusterNameTagValue
	 github.com/DataDog/datadog-agent/comp/metadata/host/hostimpl/hosttags.Get
	 github.com/DataDog/datadog-agent/pkg/aggregator.newHostTagProviderWithClock
	 github.com/DataDog/datadog-agent/pkg/aggregator.initAgentDemultiplexer
	 github.com/DataDog/datadog-agent/pkg/aggregator.InitAndStartAgentDemultiplexer
	 github.com/DataDog/datadog-agent/comp/aggregator/demultiplexer/demultiplexerimpl.newDemultiplexer
	 github.com/DataDog/datadog-agent/comp/aggregator/demultiplexer/demultiplexerimpl.newDemultiplexer·f
	 github.com/DataDog/datadog-agent/comp/aggregator/demultiplexer/demultiplexerimpl.Module
	 github.com/DataDog/datadog-agent/cmd/dogstatsd/subcommands/start.RunDogstatsdFct
	 github.com/DataDog/datadog-agent/cmd/dogstatsd/subcommands/start.MakeCommand.func1
	 github.com/DataDog/datadog-agent/cmd/dogstatsd/subcommands/start.MakeCommand
	 github.com/DataDog/datadog-agent/cmd/dogstatsd/command.makeCommands
	 github.com/DataDog/datadog-agent/cmd/dogstatsd/command.MakeRootCommand
	 main.main
	 runtime.main_main·f
	 runtime.main
	 runtime.mainPC
	 runtime.rt0_go
	 main
	 _

The output of whydeadcode is also more complex than defined here. It can show call paths where MethodByName() is called with the constant argument as the first item but these are really not that problematic.

Actually this is something I fixed in aarzilli/whydeadcode#3, if you use the latest it should be correct.

ikonst · 2025-03-24T10:09:35Z

@pgimalac To complement that, can you comment out the use of text/template but undo this change and see if the binary size remains large? I want to make sure this has an actual effect.

I've made a similar PR in jszwec/csvutil#73 but then I couldn't reproduce the effect.

ikonst · 2025-03-24T10:25:27Z

For example, given this program:

package main

import (
	"fmt"
	"reflect"
)

type embedding struct {
	reflect.Type
	foo string
}

type s struct {
}

func (s) Foo() {}

func main() {
	t := reflect.TypeOf(s{})
	a := embedding{Type: t, foo: "bar"}
	fmt.Println(a)
	//m := a.Method(0)
	//fmt.Println(m)
}

I'm only whydeadcode output if I uncomment the last 2 lines.

Am I missing something?

pgimalac · 2025-03-24T11:06:51Z

That's actually what I did above, if I just comment out the update of reflect2 then whydeadcode gives the output from my previous comment

I tried to make a minimal example a couple months ago but wasn't able to, I think it has to be complex enough (multi module at least) so that the linker can't figure out which methods are used...

fix deadcode disabling in Go

4a6a3c9

Avoid embedding reflect.Type and its Method() method because exposing it disables deadcode removal in linker. Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>

ikonst approved these changes Mar 15, 2025

View reviewed changes

taowen merged commit 35a7c28 into modern-go:master Mar 22, 2025

pgimalac mentioned this pull request Apr 14, 2025

[AGENTRUN-217] Update reflect2 to avoid disabling dead code elimination DataDog/datadog-agent#36092

Merged

pgimalac mentioned this pull request Jun 8, 2025

Remove use of html/template in apiserver to avoid disabling dead code elimination kubernetes/kubernetes#132177

Merged

liggitt mentioned this pull request Jun 10, 2025

WIP: fork jsoniter kubernetes-sigs/structured-merge-diff#289

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix deadcode disabling in Go#30

fix deadcode disabling in Go#30
taowen merged 1 commit intomodern-go:masterfrom
tonistiigi:deadcode-fix

tonistiigi commented Jul 9, 2024

Uh oh!

thaJeztah commented Mar 21, 2025

Uh oh!

ikonst commented Mar 22, 2025

Uh oh!

tonistiigi commented Mar 23, 2025

Uh oh!

ikonst commented Mar 23, 2025 •

edited

Loading

Uh oh!

tonistiigi commented Mar 24, 2025 •

edited

Loading

Uh oh!

pgimalac commented Mar 24, 2025

Uh oh!

ikonst commented Mar 24, 2025 •

edited

Loading

Uh oh!

ikonst commented Mar 24, 2025

Uh oh!

pgimalac commented Mar 24, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

tonistiigi commented Jul 9, 2024

Uh oh!

thaJeztah commented Mar 21, 2025

Uh oh!

ikonst commented Mar 22, 2025

Uh oh!

tonistiigi commented Mar 23, 2025

Uh oh!

ikonst commented Mar 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tonistiigi commented Mar 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pgimalac commented Mar 24, 2025

Uh oh!

ikonst commented Mar 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ikonst commented Mar 24, 2025

Uh oh!

pgimalac commented Mar 24, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ikonst commented Mar 23, 2025 •

edited

Loading

tonistiigi commented Mar 24, 2025 •

edited

Loading

ikonst commented Mar 24, 2025 •

edited

Loading