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

Jump to bottom

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

Closed
aarzilli opened this issue May 16, 2023 · 8 comments
Closed

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

aarzilli opened this issue May 16, 2023 · 8 comments
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. FeatureRequest NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Milestone
Backlog

Comments

@aarzilli
Copy link
Contributor

aarzilli commented May 16, 2023 •
edited

In the compiler/runtime office hours of January the problem of determining why deadcode elimination is not performed by the linker was brought up (by Daniel Martí @mvdan) and the -c option of the linker was offered as a solution, however it does not work for two reason: (1) it prints a call graph, not a reachability graph and (2) a reachability graph wouldn't be enough to determine why deadcode elimination was disabled anyway.
To put it in more practical terms:

  1. In this example deadcode elimination is disabled but you won't find any path to reflect.Value.MethodByName in the call graph

  2. The call graph is printed after deadcode elimination does (not) run. When it contains a call path starting in a public method and ending on reflect.Value.MethodByName it's impossible to tell whether the public method at the start of the path would be reachable (were it not for deadcode elimination being disabled) or not. Example where it is reachable and example where it isn't.

  3. If deadcode elimination is disabled by reflect.Type.Method it won't show up in the call graph, and deadcode elimination seems to use a symbol attribute to determine this (IsReflectMethod call in deadcodePass.flood), example of this.

There are several possible ways to do this, I patched the linker to hijack the -k option, which I do not advocate for but it shows that 90% of the work for this feature already exists in the linker.

Related #6853
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label May 16, 2023
@mvdan
Copy link
Member

mvdan commented May 16, 2023

As an end user with little knowledge of how the linker works, I think it would be useful to only get basic high-level information, like:

  • Is dead code elimination being disabled? Is it disabling for all packages, or just some?
  • Which packages are causing it to be disabled?
  • Out of those packages, which bits of code are the cause?

Essentially, allowing me to look at a 20MiB Go binary, and see how much effort it would be to re-enable dead code elimination, perhaps shaving off two or three megabytes.

Debug outputs like call graphs can always be helpful, but they're quite verbose and require knowledge of linker internals to understand them. For example, is it only MethodByName which is problematic, or are there other funcs/methods that I need to watch out for? I also imagine that the heuristic might change over time, so a higher-level diagnostic would be more stable.

@aarzilli
Copy link
Contributor Author

@mvdan: Serious answer: if there was a linker flag we could use that did something like my patch (or to print a reachability graph with all the necessary information) the rest of the gap could be filled by blog posts/conference talks/community supported tools/etc.

Joke answer: deadcode elimination is disabled because you are using cobra.

@thanm
Copy link
Contributor

thanm commented May 16, 2023

Just for the record, the reachability of reflect.Value.MethodByName and related does not completely turn off linker deadcode, it just restricts which methods can be eliminated. Linker deadcode still gets rid of a loads of code/data even in the "reflectSeen" case.

My recollection of the meeting we talked about the linker's -dumpdep flag, not -c? I may be mistaken here.

With that said, it does seem like it would be nice to have a more user-friendly option that would give reliable info in this realm.

@heschi heschi added this to the Backlog milestone May 16, 2023
@heschi
Copy link
Contributor

heschi commented May 16, 2023

cc @golang/compiler

@heschi heschi added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label May 16, 2023
@cherrymui
Copy link
Member

Yes, -dumpdep is the right flag to emit the reachability graph. -c is not what we want.

-v=2 may provide some additional internal details. But it is not user friendly, as it prints A LOT (many information about other passes as well), and for the deadcode pass it is mainly for linker developers (me) to debug it.

If there is anything useful but missing from -dumpdep, we could consider add it.

@aarzilli
Copy link
Contributor Author

If there is anything useful but missing from -dumpdep, we could consider add it.

I think what's missing is a way to print which symobls have the ReflectMethod flag set.

@gopherbot
Copy link

Change https://go.dev/cl/495715 mentions this issue: cmd/link: add ReflectMethod flag to -dumpdep output

@steiza steiza mentioned this issue Nov 3, 2023
Open
@mvdan
Copy link
Member

mvdan commented Nov 21, 2023

@aarzilli just gave a talk at https://golab.io on this topic, and mentioned this new tool of his: https://github.com/aarzilli/whydeadcode

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. FeatureRequest NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Projects
None yet
Milestone
Backlog
Development

No branches or pull requests
7 participants
@aarzilli @mknyszek @mvdan @gopherbot @thanm @cherrymui @heschi