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/pprof: graphviz node names are funny with generics #54105

Closed
bradfitz opened this issue Jul 28, 2022 · 19 comments
Closed

cmd/pprof: graphviz node names are funny with generics #54105

bradfitz opened this issue Jul 28, 2022 · 19 comments
Assignees
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@bradfitz
Copy link
Contributor

Go 1.18.3.

Not sure what I expect when I use `go tool pprof's web mode to see the graphviz SVG output on a node using generics, but not this:

Screen Shot 2022-07-27 at 6 25 02 PM

Either without the newlines, or with the concrete types (if/when available)?

FWIW, that's from:

// Set populates an entry in a map, making the map if necessary.
//
// That is, it assigns (*m)[k] = v, making *m if it was nil.
func Set[K comparable, V any, T ~map[K]V](m *T, k K, v V) {
	if *m == nil {
		*m = make(map[K]V)
	}
	(*m)[k] = v
}
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Jul 28, 2022
@prattmic
Copy link
Member

I wonder if google/pprof#689 will address this. I’m skeptical though, as bad escaping should most likely cause parse errors.

@prattmic
Copy link
Member

No, this seems to be different.

In pprof, the type name is main.Set[...] in my test (your package is named mak, right?). pprof or dot (not sure where this lives) replaces . with a newline to separate the package and symbol name onto different lines. This seems to be getting caught up in that rule.

@prattmic
Copy link
Member

There is an even worse case with pkg.(*T[...]).f turning into pkg..f, even in the text view. Both of these are covered by google/pprof#705.

@cherrymui cherrymui added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jul 28, 2022
@prattmic prattmic added this to the Backlog milestone Jul 28, 2022
@prattmic prattmic added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Jul 28, 2022
@prattmic prattmic self-assigned this Jul 28, 2022
@prattmic
Copy link
Member

Fix in google/pprof#717

@cherrymui cherrymui changed the title runtime/pprof: graphviz node names are funny with generics cmd/pprof: graphviz node names are funny with generics Jul 28, 2022
@gopherbot
Copy link

Change https://go.dev/cl/420234 mentions this issue: cmd: vendor github.com/google/pprof to fix mangled type parameter symbol names

@odeke-em
Copy link
Member

Thank you for filing this bug @bradfitz and for the quick response and fix @prattmic! I think fixing this for Go1.19 is useful as it is just updating the dependency. I have sent a CL vendoring the fix in https://go-review.googlesource.com/c/go/+/420234. @ianlancetaylor @rsc can I kindly implore you to let this fly in the Go1.19 release :-) or perhaps should we wait until the next point release and also backport too?

@prattmic
Copy link
Member

cc @golang/release

This would be nice to have in 1.19 or 1.19.1 (arguably backported to 1.18 too), but I am not sure we want to pull this in so close to the release. FWIW, this does have a clear workaround: use upstream pprof.

@prattmic
Copy link
Member

prattmic commented Aug 1, 2022

This isn't going to make it into 1.19, but we can consider it for 1.19.1 (I'm personally still a bit unsure since there is a workaround).

@prattmic prattmic modified the milestones: Backlog, Go1.19.1 Aug 1, 2022
@dmitshur
Copy link
Contributor

dmitshur commented Aug 8, 2022

I'll move this issue to Go 1.20 milestone since the fix will need to land on master branch first. To address this issue for minor releases, we'd need to use the https://go.dev/wiki/MinorReleases process to create children backport issues.

@dmitshur dmitshur modified the milestones: Go1.19.1, Go1.20 Aug 8, 2022
jproberts pushed a commit to jproberts/go that referenced this issue Aug 10, 2022
…bol names

Updates github.com/google/pprof to bring in the commit from
google/pprof#717 which fixes mangled
symbol names for type parameters.

Fixes golang#54105

Change-Id: I01af9f780aba3338b960a03b30906a23642e4448
Reviewed-on: https://go-review.googlesource.com/c/go/+/420234
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
Run-TryBot: Emmanuel Odeke <emmanuel@orijtech.com>
Reviewed-by: Than McIntosh <thanm@google.com>
@prattmic
Copy link
Member

@gopherbot Please backport to 1.19. This fix just barely missed 1.19. While it does have a workaround (use upstream pprof instead of go tool pprof), that is unlikely to be clear to users, and the fix is simple.

@gopherbot
Copy link

Backport issue(s) opened: #54420 (for 1.19).

Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://go.dev/wiki/MinorReleases.

@gopherbot
Copy link

Change https://go.dev/cl/423356 mentions this issue: [release-branch.go1.19] cmd: vendor github.com/google/pprof to fix mangled type parameter symbol names

@dmitshur
Copy link
Contributor

@prattmic The backporting approach described in #34536 (comment) suggests we'll need to also backport the fix to 1.18, since the problem also exists in that version.

@prattmic
Copy link
Member

prattmic commented Aug 12, 2022

Sure, I can do so. Do we have a standard process for backporting one-off fixes to vendored packages, rather than wholesale updates? I can easily manually apply the appropriate changes to the vendored pprof files, but what should go.mod say? The vendored code will no longer be any valid upstream version of github.com/google/pprof.

@prattmic
Copy link
Member

I don't see any such similar cases on https://go-review.git.corp.google.com/q/project:go+is:merged+file:vendor+-branch:master+-branch:dev.link+-branch:dev.boringcrypto+-branch:dev.typeparams+-branch:dev.fuzz. :(

A full update of pprof on release-branch.go1.18 will pull in 20 commits:

$ git log --oneline f987b9c94b31..154dc81eb7b0
154dc81 all: update dependencies (#702)
59ca7ad Generalize the unit support in pprof a bit further. (#699)
83db2b7 Split monolithic webhtml.go into multiple files. (#695)
fdd30d9 Update minimum Go version to 1.17 in go.mod. (#693)
d8f96c0 Fix doc comments format to become compatible with tip gofmt. (#694)
b5a4dc8 allow rendering big flame graphs by avoiding stack overflow in JS parser (#684)
85950bb Fix tagroot to properly format unitless numeric tags. (#691)
c2158d7 Add Go 1.18 to testing, remove Go 1.16. (#692)
b2ab032 third_party: fix typo (#685)
5bba342 internal/graph: Support comments with double quotes (#688)
0368bd9 Handle either _text or _stext as the kernel relocation symbol. (#674)
43b8053 Parse and propagate the name of the kernel relocation symbol (#675)
513e8ac doc: clarify graph view docs to note negative values appear in profile comparison (#667)
8c355e5 internal/elfexec: Fix typos in elfexec.go (#681)
d25a53d Log build ID in local symbolization error messages. (#679)
6f57359 Update d3-flame-graph from 2.0.0-alpha to 4.1.3 (#677)
2007db6 Add badge link to Go API docs in pkg.go.dev (#676)
1daafda Update instructions to use "git clone" instead of "go get". (#673)
44fc4e8 proto/profile.proto: fix typo (#672)
e9b0287 Update mapassign regex to match both call variants (#668)

@dmitshur
Copy link
Contributor

dmitshur commented Aug 12, 2022

Do we have a standard process for backporting one-off fixes to vendored packages, rather than wholesale updates?

We have a standard process that applies to golang.org/x dependencies. It uses "internal-branch.go1.x-vendor" branches and is documented at https://go.dev/wiki/MinorReleases#cherry-pick-cls-for-vendored-golangorgx-packages.

This is the first backport request for a problem in the github.com/google/pprof dependency that I'm seeing, so not sure. Depending on how much the vendored github.com/google/pprof interacts with cmd/pprof and other parts of the standard library, it might be safest to update it to the same v0.0.0-20220729232143-a41b82acbcb1 version (that way, Go 1.19 and 1.18 will be using the same version). If that isn't safe, we'll need to figure out what to do instead.

@prattmic
Copy link
Member

I can take a closer look tomorrow, but I don't think there are any technical blockers (or any changes to non-vendored code required) to updating pprof to v0.0.0-20220729232143-a41b82acbcb1 in 1.18, it's just that pulling in a bunch of unrelated commits has higher risk of introducing other issues. Sounds like taking that risk is the best bet right now.

@prattmic
Copy link
Member

@gopherbot Please backport to 1.18. While it does have a workaround (use upstream pprof instead of go tool pprof), that is unlikely to be clear to users.

@gopherbot
Copy link

Change https://go.dev/cl/423576 mentions this issue: [release-branch.go1.18] cmd: upgrade github.com/google/pprof to v0.0.0-20220729232143-a41b82acbcb1

gopherbot pushed a commit that referenced this issue Aug 19, 2022
…0-20220729232143-a41b82acbcb1

Update the vendored copy of github.com/google/pprof to include the fix
for #54105.

pprof's go.mod specifies an upgrade to x/sys, thus we must also update
std's x/sys to match this version.

For #54105.
Fixes #54464.

Change-Id: I0ca4f338b3ec3e8c54a892eb684a5dd3af8d7f1b
Reviewed-on: https://go-review.googlesource.com/c/go/+/423576
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
Run-TryBot: Michael Pratt <mpratt@google.com>
gopherbot pushed a commit that referenced this issue Aug 19, 2022
…ngled type parameter symbol names

Updates github.com/google/pprof to bring in the commit from
google/pprof#717 which fixes mangled
symbol names for type parameters.

For #54105
Fixes #54420

Change-Id: I01af9f780aba3338b960a03b30906a23642e4448
Reviewed-on: https://go-review.googlesource.com/c/go/+/420234
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
Run-TryBot: Emmanuel Odeke <emmanuel@orijtech.com>
Reviewed-by: Than McIntosh <thanm@google.com>
(cherry picked from commit cd9cd92)
Reviewed-on: https://go-review.googlesource.com/c/go/+/423356
Run-TryBot: Michael Pratt <mpratt@google.com>
Reviewed-by: Emmanuel Odeke <emmanuel@orijtech.com>
bradfitz pushed a commit to tailscale/go that referenced this issue Sep 8, 2022
…ngled type parameter symbol names

Updates github.com/google/pprof to bring in the commit from
google/pprof#717 which fixes mangled
symbol names for type parameters.

For golang#54105
Fixes golang#54420

Change-Id: I01af9f780aba3338b960a03b30906a23642e4448
Reviewed-on: https://go-review.googlesource.com/c/go/+/420234
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
Run-TryBot: Emmanuel Odeke <emmanuel@orijtech.com>
Reviewed-by: Than McIntosh <thanm@google.com>
(cherry picked from commit cd9cd92)
Reviewed-on: https://go-review.googlesource.com/c/go/+/423356
Run-TryBot: Michael Pratt <mpratt@google.com>
Reviewed-by: Emmanuel Odeke <emmanuel@orijtech.com>
@golang golang locked and limited conversation to collaborators Aug 15, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

6 participants