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

cmd/go: consider making "tool pprof" work with the module cache #48601

Open
mvdan opened this issue Sep 24, 2021 · 4 comments
Open

cmd/go: consider making "tool pprof" work with the module cache #48601

mvdan opened this issue Sep 24, 2021 · 4 comments
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@mvdan
Copy link
Member

mvdan commented Sep 24, 2021

I was just debugging a server's large memory usage, so via its http pprof endpoints, I downloaded some profiles.

The binaries running on that server were built with -trimprefix, because it's a sane default for "production builds", and enables reproducible builds too.

Unfortunately that means that, if I grab the deployed binary and some of its profiles, trying to use go tool pprof mostly works, but trying to inspect any source code fails:

$ go tool pprof dvotenode heap
[...]
(pprof) list NewCache
Total: 2.20GB
ROUTINE ======================== github.com/dgraph-io/ristretto.NewCache in github.com/dgraph-io/ristretto@v0.1.0/cache.go
    2.41MB    21.05MB (flat, cum)  0.94% of Total
 Error: could not find file github.com/dgraph-io/ristretto@v0.1.0/cache.go on path /home/mvdan

Adding -source_path=$(go env GOMODCACHE) fixes those cases:

(pprof) list NewCache
Total: 2.20GB
ROUTINE ======================== github.com/dgraph-io/ristretto.NewCache in github.com/dgraph-io/ristretto@v0.1.0/cache.go
    2.41MB    21.05MB (flat, cum)  0.94% of Total
         .          .    164:	case config.MaxCost == 0:
         .          .    165:		return nil, errors.New("MaxCost can't be zero")
         .          .    166:	case config.BufferItems == 0:
         .          .    167:		return nil, errors.New("BufferItems can't be zero")
         .          .    168:	}
         .    18.15MB    169:	policy := newPolicy(config.NumCounters, config.MaxCost)
         .          .    170:	cache := &Cache{
[...]

This won't cover quite all cases, because the main module lacks a version, and might not even be in the module cache if I built from a git clone. Note the lack of @module-version in the error output, unlike my previous example involving a dependency:

(pprof) list LoadTree
Total: 2.20GB
ROUTINE ======================== go.vocdoni.io/dvote/census.(*Manager).LoadTree in go.vocdoni.io/dvote/census/census.go
         0     2.13GB (flat, cum) 97.15% of Total
 Error: could not find file go.vocdoni.io/dvote/census/census.go on path /home/mvdan/go/pkg/mod

I still think it would be neat for go tool pprof to come with a built-in -source_path=$(go env GOMODCACHE) default. As far as I can tell, it can't hurt. It won't be perfect with handling the main module either, but that's okay.

One alternative I do have, which will support listing all function sources, is to rebuild the same program with the same build flags but without -trimpath. Arguably that's the better solution if I want to be able to inspect all the source. However, that takes extra steps, and sometimes it might be difficult to figure out how to build a slight variant of exactly the same program at exactly the same version. So the sane(r) default that I propose here would probably be enough to perform a lot of list commands.

Yet another question is how I could tell go tool pprof that all the go.vocdoni.io/dvote module sources are in the directory /home/mvdan/src/dvote. I don't think -source_path is powerful enough for this right now, as I don't seem to be able to supply two paths. This also seems worth investigating in terms of UX, but it does seem like a separate idea.

cc @hyangah @aalexand @cherrymui

@hyangah
Copy link
Contributor

hyangah commented Sep 24, 2021

In module world, there can be multiple source roots where involved source files are. 1) module cache, 2) main module, and 3) local dependencies (replaced dependencies or picked by go.work). 2) and 3) can be outside of module cache, right?

If -source_path=$(go env GOMODCACHE) is default, that will always break source listing of the main module.
Many users use pprof during development and can be interested in optimizing code in their main project (main module, not in the module cache), I am afraid this default change would break many common cases. I may misunderstand the flag.

Delve addresses a similar issue with users source mapping configuration that allows multiple mapping entries.

https://github.com/go-delve/delve/tree/master/Documentation/cli#config

config substitute-path <from> <to>
config substitute-path <from>

Can pprof be extended to allow such customization? @aalexand

@mvdan
Copy link
Member Author

mvdan commented Sep 24, 2021

I do imagine this might require something more than the current -source_path. Perhaps the ability to append "prefix maps" so we can:

  • map / to / (absolute paths, on by default)
  • map path@version to go env GOMODCACHE (module cache, on by default for Go)

And the user could add extra maps, like github.com/current/module to $PWD (minus the prefix).

@aalexand
Copy link
Contributor

We resisted adding the path substitution in pprof, this adds complexity that is rarely needed.

There is google/pprof#611 in pprof (with google/pprof#612 that attempts to fix that), is this related?

@mvdan
Copy link
Member Author

mvdan commented Sep 25, 2021

That kind of fix would certainly work :) I just didn't expect it would be acceptable for pprof to learn Go specifics.

@mknyszek mknyszek added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Oct 4, 2021
@mknyszek mknyszek added this to the Backlog milestone Oct 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

4 participants