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/compile: add test for PGO profiles merged from profiles with different source versions #58101

Open
prattmic opened this issue Jan 26, 2023 · 3 comments
Assignees
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. Testing An issue that has been verified to require only test changes, not just a test failure.
Milestone

Comments

@prattmic
Copy link
Member

Users may be running several slightly different versions of their service in production (e.g., due to staged rollouts).

Then when collecting profiles, those users may, intentionally or unintentionally, collect profiles from multiple different versions of the binary and merge them together for use as the PGO profile.

I believe we already handle these correctly, but it is not intuitive, so we should add tests to ensure this continues to work.

The interesting case are samples in functions that have moved (different StartLine) between profiles A and B:

  1. Profiling merging uses (StartLine, Name, SystemName, Filename) as the Function identity key. Thus there will be 2 Functions with the same Name but different StartLine. Samples referencing these functions won't merge because they reference different Function ids.

  2. When building the Node graph, we will end up with unique Nodes for each of the distinct Functions.

  3. When processing Nodes to compute weights, we will calculate the same NodeMapKey for Nodes from each of the unique Functions (assuming line offsets within the function are unchanged). Thus, we will accumulate weights from the different Functions into the same NodeMap entry.

  4. IRGraph construction then works as normal, finding merged weights from NodeMap entries.

This is quite fragile and we could easily break this without tests (or I missed something and it is already broken!)

cc @cherrymui @aclements

@prattmic prattmic added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Jan 26, 2023
@prattmic prattmic added this to the Go1.21 milestone Jan 26, 2023
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Jan 26, 2023
@bcmills bcmills added the Testing An issue that has been verified to require only test changes, not just a test failure. label Jan 26, 2023
@mknyszek
Copy link
Contributor

mknyszek commented Jun 9, 2023

Hey @prattmic, do you still plan to do this for Go 1.21? Should it go in Backlog? Thanks.

@prattmic
Copy link
Member Author

It's not critical, but it would still be nice to add the test coverage if I get around to it.

@mknyszek mknyszek modified the milestones: Go1.21, Go1.22 Jun 13, 2023
@mknyszek
Copy link
Contributor

I moved it to Go 1.22 just to get it out of the milestone. It sounds like it may or may not happen for Go 1.21, but since it's test-focused I guess it's not actually bound by the release?

@gopherbot gopherbot modified the milestones: Go1.22, Go1.23 Feb 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. Testing An issue that has been verified to require only test changes, not just a test failure.
Projects
Development

No branches or pull requests

4 participants