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.

Sign up for GitHub

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/compile: pgo weighted merge support #64487

Open
zamazan4ik opened this issue Dec 1, 2023 · 5 comments
Open

cmd/compile: pgo weighted merge support #64487

zamazan4ik opened this issue Dec 1, 2023 · 5 comments
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Backlog

Comments

@zamazan4ik
Copy link

zamazan4ik commented Dec 1, 2023 •
edited

Go PGO documentation shows support for merging multiple PGO profiles. However, as far as I see, there is no support for adding a weight to each profile. I quickly checked the pprof merge implementation and also didn't find a way to add weights to profiles for the merge operation.

In the current guideline already there is a note about merging multiple profiles into one:

Merge profiles across workloads: take profiles from each workload (weighted by total footprint) and merge them into a single “fleet-wide” profile used to build a single common profile used to build. This likely provides modest performance improvements for all workloads.

But it also says nothing about adding weights to the profiles.

The use case for such functionality is the following. We gathered N profiles from different workloads. According to our needs, one workload is our primary workload, and others are secondaries. So we want to add some multiplier to optimize as much as possible for our main workload, and optimize for our secondary workloads too but with a lower priority.

llvm-profdata tool supports weighted merge (here are some invocation examples).

Right now I see the following ways, how it can be mitigated:

  • (an easy way) If we want to perform merging for profiles A and B with weights 2 and 1 correspondingly, we need to copy A profile, then merge 3 profiles: A, A_copy, B. In theory, it would be equal to the weight 2 near the A profile. However, not the best UX.
  • (a hard way) Parse PGO profiles somehow and tweak internal counters with some weight multiplier. Doable but hard to implement.

If you think having such functionality is important for PGO users in Go, I kindly ask implement it. Before the weight implementation, we can put the temporal mitigations to the documentation (of course if they work - I didn't test them yet). Probably it will require some changes on the pprof side.
@seankhliao seankhliao changed the title PGO profiles: weighted merge support cmd/compile: pgo weighted merge support Dec 1, 2023
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Dec 1, 2023
@dmitshur
Copy link
Contributor

dmitshur commented Dec 1, 2023

CC @golang/compiler.

@dmitshur dmitshur added this to the Backlog milestone Dec 1, 2023
@dmitshur dmitshur added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Dec 1, 2023
@prattmic
Copy link
Member

prattmic commented Dec 6, 2023 •
edited

It is true that there is no existing tooling to perform a weighted profile merge. That would be nice to have.

In the current guideline already there is a note about merging multiple profiles into one:

Merge profiles across workloads: take profiles from each workload (weighted by total footprint) and merge them into a single “fleet-wide” profile used to build a single common profile used to build. This likely provides modest performance improvements for all workloads.

But it also says nothing about adding weights to the profiles.

https://go.dev/doc/pgo#merging-profiles does allude to this limitation: "you likely want to ensure that all profiles have the same wall duration (i.e., all profiles are collected for 30s). Otherwise, profiles with longer wall duration will be overrepresented in the merged profile." Perhaps it isn't explicit enough, though.

(a hard way) Parse PGO profiles somehow and tweak internal counters with some weight multiplier. Doable but hard to implement.

I disagree that this is hard. The github.com/google/pprof/profile package is very useful and makes this kind of thing easy. I prototyped this in https://gist.github.com/prattmic/9818d05c6e6cf5c132e9105789f95e72, and it is quite simple. Half of this program is just flag parsing.

@mknyszek
Copy link
Contributor

mknyszek commented Dec 6, 2023

In compiler/runtime triage, we think that maybe this should be a feature request for the pprof tool instead (which the Go distribution vendors). As @prattmic this probably isn't too hard to implement, it just needs a CLI or something. We think that it would make more sense for some other tool than the compiler to do it, because that'll directly impact build times.

@mknyszek mknyszek closed this as not planned Won't fix, can't repro, duplicate, stale Dec 6, 2023
@zamazan4ik
Copy link
Author

I prototyped this in https://gist.github.com/prattmic/9818d05c6e6cf5c132e9105789f95e72, and it is quite simple. Half of this program is just flag parsing.

Thanks for sharing your prototype! It would be really valuable before we get a "mature" implementation.

In compiler/runtime triage, we think that maybe this should be a feature request for the pprof tool instead (which the Go distribution vendors). As @prattmic this probably isn't too hard to implement, it just needs a CLI or something. We think that it would make more sense for some other tool than the compiler to do it, because that'll directly impact build times.

Do I need to create this issue in pprof repo instead?

@prattmic
Copy link
Member

prattmic commented Dec 7, 2023

I'm less convinced that this should go straight to the pprof repo. While I think this would be a decent feature for the pprof CLI (perhaps -scale 1.0,2.0,etc, basically a custom version of -normalize), the pprof tool is fairly selective about adding features, so I don't want to dump everything on them.

Perhaps we should have a tool in x/tools for working with profiles, specifically for PGO that could provide extra functionality not available directly in pprof.

@prattmic prattmic reopened this Dec 7, 2023
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. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
Go Compiler / Runtime
Status: In Progress
Milestone
Backlog
Development

No branches or pull requests
5 participants
@mknyszek @prattmic @dmitshur @zamazan4ik @gopherbot