cmd/compile/internal/pgo: fix hard-coded PGO sample data position #58294

brancz · 2023-02-03T15:14:45Z

This patch detects at which index position profiling samples that have the value-type samples count are, instead of the previously hard-coded position of index 1. Runtime generated profiles always generate CPU profiling data with the 0 index being CPU nanoseconds, and samples count at index 1, which is why this previously hasn't come up.

Fixes #58292

gopherbot · 2023-02-03T15:22:41Z

This PR (HEAD: 2cb3ce0) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/465135 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

brancz · 2023-02-03T15:22:46Z

Happy to add a test of course, but wanted to discuss what you would prefer that to look like. Should I edit the existing test and modify the generated pprof and put the samples count on index 0? An additional test? Or is it small enough that you don't care?

gopherbot · 2023-02-03T15:29:17Z

Message from Gopher Robot:

Patch Set 1:

Congratulations on opening your first change. Thank you for your contribution!

Next steps:
A maintainer will review your change and provide feedback. See
https://go.dev/doc/contribute#review for more info and tips to get your
patch through code review.

Most changes in the Go project go through a few rounds of revision. This can be
surprising to people new to the project. The careful, iterative review process
is our way of helping mentor contributors and ensuring that their contributions
have a lasting impact.

During May-July and Nov-Jan the Go project is in a code freeze, during which
little code gets reviewed or merged. If a reviewer responds with a comment like
R=go1.11 or adds a tag like "wait-release", it means that this CL will be
reviewed as part of the next development cycle. See https://go.dev/s/release
for more details.

Please don’t reply on this GitHub thread. Visit golang.org/cl/465135.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2023-02-03T15:48:25Z

Message from Michael Pratt:

Patch Set 1: Run-TryBot+1

Please don’t reply on this GitHub thread. Visit golang.org/cl/465135.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2023-02-03T15:55:05Z

Message from Gopher Robot:

Patch Set 1:

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/465135.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2023-02-03T16:01:27Z

Message from Gopher Robot:

Patch Set 1:

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/465135.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2023-02-03T16:07:54Z

Message from Gopher Robot:

Patch Set 1: TryBot-Result-1

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/465135.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2023-02-03T16:07:55Z

Message from Michael Pratt:

Patch Set 1: Code-Review+1

(2 comments)

Please don’t reply on this GitHub thread. Visit golang.org/cl/465135.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2023-02-03T16:59:35Z

This PR (HEAD: 9bf83e6) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/465135 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

gopherbot · 2023-02-03T17:12:33Z

Message from Frederic Branczyk:

Patch Set 1:

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/465135.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2023-02-03T17:12:34Z

Message from Cherry Mui:

Patch Set 1: Code-Review+2

(2 comments)

Please don’t reply on this GitHub thread. Visit golang.org/cl/465135.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2023-02-03T21:07:31Z

Message from Michael Pratt:

Patch Set 2: Code-Review+1

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/465135.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2023-02-04T09:45:20Z

This PR (HEAD: 302d47f) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/465135 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

gopherbot · 2023-02-04T09:56:04Z

Message from Frederic Branczyk:

Patch Set 3:

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/465135.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2023-02-07T17:30:13Z

Message from Austin Clements:

Patch Set 3:

(2 comments)

Please don’t reply on this GitHub thread. Visit golang.org/cl/465135.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2023-02-08T02:15:46Z

This PR (HEAD: 157ffb9) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/465135 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

gopherbot · 2023-02-08T02:27:45Z

Message from Frederic Branczyk:

Patch Set 4:

(2 comments)

Please don’t reply on this GitHub thread. Visit golang.org/cl/465135.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2023-02-08T17:11:23Z

Message from Michael Pratt:

Patch Set 4: Code-Review+2 Run-TryBot+1

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/465135.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2023-02-08T17:11:23Z

Message from Gopher Robot:

Patch Set 4:

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/465135.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2023-02-08T17:23:15Z

Message from Gopher Robot:

Patch Set 4: TryBot-Result+1

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/465135.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2023-02-08T17:53:27Z

Message from David Chase:

Patch Set 4:

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/465135.
After addressing review feedback, remember to publish your drafts!

This patch detects at which index position profiling samples that have the value-type samples count are, instead of the previously hard-coded position of index 1. Runtime generated profiles always generate CPU profiling data with the 0 index being CPU nanoseconds, and samples count at index 1, which is why this previously hasn't come up. Fixes golang#58292

gopherbot · 2023-02-08T17:59:31Z

This PR (HEAD: dabbf9f) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/465135 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

gopherbot · 2023-02-08T18:12:05Z

Message from Frederic Branczyk:

Patch Set 5:

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/465135.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2023-02-08T18:12:06Z

Message from Michael Pratt:

Patch Set 5: Auto-Submit+1 Code-Review+2 Run-TryBot+1

Please don’t reply on this GitHub thread. Visit golang.org/cl/465135.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2023-02-08T18:18:09Z

Message from Gopher Robot:

Patch Set 5:

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/465135.
After addressing review feedback, remember to publish your drafts!

This patch detects at which index position profiling samples that have the value-type samples count are, instead of the previously hard-coded position of index 1. Runtime generated profiles always generate CPU profiling data with the 0 index being CPU nanoseconds, and samples count at index 1, which is why this previously hasn't come up. Fixes #58292 Change-Id: Idde761d53b02259f3076c4e5dcb4a96a9d53df0e GitHub-Last-Rev: dabbf9f GitHub-Pull-Request: #58294 Reviewed-on: https://go-review.googlesource.com/c/go/+/465135 Auto-Submit: Michael Pratt <[email protected]> TryBot-Result: Gopher Robot <[email protected]> Reviewed-by: Michael Pratt <[email protected]> Run-TryBot: Michael Pratt <[email protected]> Reviewed-by: Cherry Mui <[email protected]>

gopherbot · 2023-02-08T18:24:32Z

This PR is being closed because golang.org/cl/465135 has been merged.

This patch detects at which index position profiling samples that have the value-type samples count are, instead of the previously hard-coded position of index 1. Runtime generated profiles always generate CPU profiling data with the 0 index being CPU nanoseconds, and samples count at index 1, which is why this previously hasn't come up. Fixes golang#58292 Change-Id: Idde761d53b02259f3076c4e5dcb4a96a9d53df0e GitHub-Last-Rev: dabbf9f GitHub-Pull-Request: golang#58294 Reviewed-on: https://go-review.googlesource.com/c/go/+/465135 Auto-Submit: Michael Pratt <[email protected]> TryBot-Result: Gopher Robot <[email protected]> Reviewed-by: Michael Pratt <[email protected]> Run-TryBot: Michael Pratt <[email protected]> Reviewed-by: Cherry Mui <[email protected]>

brancz force-pushed the fix-samples-count-index branch from 3b6e84e to 2cb3ce0 Compare February 3, 2023 15:17

brancz force-pushed the fix-samples-count-index branch 2 times, most recently from 0e406c8 to 9bf83e6 Compare February 3, 2023 16:57

brancz force-pushed the fix-samples-count-index branch from 9bf83e6 to 302d47f Compare February 4, 2023 09:42

brancz force-pushed the fix-samples-count-index branch from 302d47f to 157ffb9 Compare February 8, 2023 02:08

brancz force-pushed the fix-samples-count-index branch from 157ffb9 to dabbf9f Compare February 8, 2023 17:58

gopherbot closed this Feb 8, 2023

cmd/compile/internal/pgo: fix hard-coded PGO sample data position #58294

cmd/compile/internal/pgo: fix hard-coded PGO sample data position #58294

Uh oh!

Conversation

brancz commented Feb 3, 2023

Uh oh!

gopherbot commented Feb 3, 2023

Uh oh!

brancz commented Feb 3, 2023

Uh oh!

gopherbot commented Feb 3, 2023

Uh oh!

gopherbot commented Feb 3, 2023

Uh oh!

gopherbot commented Feb 3, 2023

Uh oh!

gopherbot commented Feb 3, 2023

Uh oh!

gopherbot commented Feb 3, 2023

Uh oh!

gopherbot commented Feb 3, 2023

Uh oh!

gopherbot commented Feb 3, 2023

Uh oh!

gopherbot commented Feb 3, 2023

Uh oh!

gopherbot commented Feb 3, 2023

Uh oh!

gopherbot commented Feb 3, 2023

Uh oh!

gopherbot commented Feb 4, 2023

Uh oh!

gopherbot commented Feb 4, 2023

Uh oh!

gopherbot commented Feb 7, 2023

Uh oh!

gopherbot commented Feb 8, 2023

Uh oh!

gopherbot commented Feb 8, 2023

Uh oh!

gopherbot commented Feb 8, 2023

Uh oh!

gopherbot commented Feb 8, 2023

Uh oh!

gopherbot commented Feb 8, 2023

Uh oh!

gopherbot commented Feb 8, 2023

Uh oh!

gopherbot commented Feb 8, 2023

Uh oh!

gopherbot commented Feb 8, 2023

Uh oh!

gopherbot commented Feb 8, 2023

Uh oh!

gopherbot commented Feb 8, 2023

Uh oh!

gopherbot commented Feb 8, 2023

Uh oh!

Uh oh!