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: sync with upstream https://github.com/google/pprof ? #16184

Closed
dgryski opened this issue Jun 26, 2016 · 20 comments
Closed

cmd/pprof: sync with upstream https://github.com/google/pprof ? #16184

dgryski opened this issue Jun 26, 2016 · 20 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@dgryski
Copy link
Contributor

dgryski commented Jun 26, 2016

The pprof tool was initially an import of Google's internal version, which is now on GitHub. Can we just switch to that new repo? Are there patches in Go's copy that need to be pushed upstream to make google/pprof work with current Go binaries?

Maybe this ends up just being a doc fix saying "you can also use this other pprof tool over here".

@ianlancetaylor ianlancetaylor added this to the Go1.8 milestone Jun 26, 2016
@hyangah
Copy link
Contributor

hyangah commented Jun 29, 2016

I ask @rauls5382 most pprof questions

@rauls5382
Copy link
Contributor

I am not aware of any changes to pprof that are missing on the github version.

There are some UI differences that I plan to smooth out, but other than that it should be fully functional with Go programs. Please open issues at github.com/google/pprof/issues if you encounter any problems.

I think it would be great if we could sync up the two versions at some point.

@bradfitz
Copy link
Contributor

I am not aware of any changes to pprof that are missing on the github version.

Have you been merging all Go's changes? There have been a handful.

@rauls5382
Copy link
Contributor

I have merged a few in the past but admittedly haven't checked recently. I will take a new look.

@quentinmit quentinmit added the NeedsFix The path to resolution is known, but the work has not been done. label Oct 6, 2016
@quentinmit
Copy link
Contributor

@rauls5382 Have you had a chance to compare-and-merge pprof?

@rauls5382
Copy link
Contributor

I finally spent some time on this. There's about a dozen small CLs that should be upstreamed.
Most of them are very small. I'll be upstreaming them shortly.

@rsc
Copy link
Contributor

rsc commented Oct 25, 2016

@rauls5382, it sounds like you found changes in Go that need to be merged into google/pprof.

What about the other direction? Are there patches in google/pprof that need to be merged into Go?

@rauls5382
Copy link
Contributor

Yes, google/pprof has had many modifications after the golang fork.
There was a significant reorganization of the packages before they were published into golang/pprof. The process of syncing up with upstream will require some manual changes as the package structure is different. I'll put a proposal together.

@quentinmit
Copy link
Contributor

Too late for Go 1.8. Please try to have this ready for Go1.9. Our pprof is falling behind and I would love to have the improvements in Go.

@quentinmit quentinmit modified the milestones: Go1.9Early, Go1.8 Nov 1, 2016
@gopherbot
Copy link

CL https://golang.org/cl/33212 mentions this issue.

@hyangah
Copy link
Contributor

hyangah commented Feb 7, 2017

@rauls5382 We are now in early stage of go1.9 cycle. Can we merge into it?

I checked the abandoned cl/33212 and it seems that it doesn't include some patches added in Go side. So need refresh.

@hyangah
Copy link
Contributor

hyangah commented Feb 7, 2017

For long term maintenance, wouldn't it be easier to use vendoring if the canonical source remains in github.com/google/pprof, and ask developers to update the upstream directly (then update the vendorred version in go repo)?

@rauls5382
Copy link
Contributor

rauls5382 commented Feb 8, 2017 via email

@dgryski
Copy link
Contributor Author

dgryski commented Feb 8, 2017

Would it be possible to use Go's build tags features to toggle C++ demangling, binutils usage, etc so that the google/pprof repo can be more canonically upstream with no changes when it's imported to the Go repo?

@davecheney
Copy link
Contributor

davecheney commented Feb 8, 2017 via email

@hyangah
Copy link
Contributor

hyangah commented Feb 8, 2017

I tried to compare the package and source code structure of Go's pprof and github.com/google/pprof.

Go --> github.com/google/pprof
src/internal/pprof/* --> profile/*
cmd/pprof/internal/* --> internal/* except binutil

Hopefully just import path change would be sufficient.

cmd/pprof/*.go files (package main) contain custom driver.Fetcher, driver.ObjTool implementations and go tool compatible documentation. So these need to be committed directly to the go repo(?) or as @dgryski proposed, let google/pprof repo host them in a special directory.

@rsc
Copy link
Contributor

rsc commented Feb 8, 2017

The cmd/pprof directory should probably just be left in the main repo. It's only the helper libraries (subdirectories and src/internal/pprof) that should be pulled down from github.com/google/pprof.

@rauls5382
Copy link
Contributor

rauls5382 commented Feb 8, 2017 via email

@rsc
Copy link
Contributor

rsc commented Feb 8, 2017

For cmd/pprof, I think the way forward is to copy the relevant directories into go/src/cmd/vendor/github.com/google/pprof/xxx, with the files unmodified. Update cmd/vendor/vendor.json when you do. Then cmd/pprof becomes just a single directory with a few files, that imports paths like "github.com/google/pprof/xxx", which are resolved into the cmd/vendor directory. It is fine to vendor iant's demangle too.

For the purposes of updating cmd/pprof, let's forget that go/src/internal/pprof/profile exists. Vendor github.com/google/pprof/profile into go/src/cmd/vendor/github.com/google/pprof/profile like all the other directories, and let pprof use it.

We'll maintain go/src/internal/pprof/profile as a separate fork; we only use it for writing profiles, so we can cut out even more and just live with the minimal maintenance burden. This code is linked into essentially every Go program, so we want to be able to make it as small as possible and not have it be subject to unexpected dependencies.

Thanks.

@bradfitz
Copy link
Contributor

bradfitz commented May 3, 2017

I believe this happened (a047f72, etc). Closing.

@rauls5382, correct me if I'm wrong.

@bradfitz bradfitz closed this as completed May 3, 2017
@golang golang locked and limited conversation to collaborators May 3, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

9 participants