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

Open
bradfitz opened this issue Jul 28, 2022 · 9 comments
Open

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

bradfitz opened this issue Jul 28, 2022 · 9 comments
Assignees
Labels
compiler/runtime NeedsFix
Milestone

Comments

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Jul 28, 2022

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 label Jul 28, 2022
@prattmic
Copy link
Member

@prattmic prattmic commented Jul 28, 2022

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

@prattmic prattmic commented Jul 28, 2022

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

@prattmic prattmic commented Jul 28, 2022

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 label Jul 28, 2022
@prattmic prattmic added this to the Backlog milestone Jul 28, 2022
@prattmic prattmic added NeedsFix and removed NeedsInvestigation labels Jul 28, 2022
@prattmic prattmic self-assigned this Jul 28, 2022
@prattmic
Copy link
Member

@prattmic prattmic commented Jul 28, 2022

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

@gopherbot gopherbot commented Jul 30, 2022

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

@odeke-em odeke-em commented Jul 30, 2022

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

@prattmic prattmic commented Jul 30, 2022

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 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 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler/runtime NeedsFix
Projects
Status: In Progress
Development

No branches or pull requests

6 participants