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

runtime: cpu sample has wrong descripting comment in the code #58327

Closed
subtle-byte opened this issue Feb 5, 2023 · 4 comments
Closed

runtime: cpu sample has wrong descripting comment in the code #58327

subtle-byte opened this issue Feb 5, 2023 · 4 comments
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.

Comments

@subtle-byte
Copy link
Contributor

subtle-byte commented Feb 5, 2023

What version of Go are you using (go version)?

$ go version
go version go1.19.5 linux/amd64

Does this issue reproduce with the latest release?

Yes, I got the same result with go 1.20.

What did you do?

I have program:

package main

import (
	"os"
	"runtime"
	"runtime/trace"
)

func main() {
	runtime.SetCPUProfileRate(100)
	err := trace.Start(os.Stdout)
	if err != nil {
		panic(err)
	}
	for i := 0; i < 10000; i++ {
		for j := 0; j < 10000; j++ {
			_ = i + j
		}
	}
	defer trace.Stop()
}

The program provides a trace file as output.

What did you expect to see?

I expect Cpu samples in the trace file will be in the format (as in the source code of trace tool):

EvCPUSample         = 49 // CPU profiling sample [timestamp, stack, real timestamp, real P id (-1 when absent), goroutine id]

I.e. stack is second, real timestamp is third.

What did you see instead?

Instead, I see the following events in the trace file:

event type: 49, args: [11 1755437435756 0 1 15]
event type: 49, args: [10 1755437928681 0 1 15]
event type: 49, args: [4 1755438251486 0 1 15]

So seems like real timestamp is on the second position, but more essential there is no stack id!

@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Feb 5, 2023
@subtle-byte
Copy link
Contributor Author

Hi @rhysh, seems like you added cpu samples to traces, could you take a look at this issue?

@subtle-byte
Copy link
Contributor Author

After some investigation, looks like we have the stack id in the cpu sample, but the comment to the mentioned line is invalid - actually stack id is last, so the comment should look like:

EvCPUSample         = 49 // CPU profiling sample [timestamp, real timestamp, real P id (-1 when absent), goroutine id, stack]

@subtle-byte subtle-byte changed the title runtime/trace: cpu samples don't contain stack id cmd/trace: cpu sample has wrong descripting comment in the code Feb 5, 2023
@subtle-byte subtle-byte changed the title cmd/trace: cpu sample has wrong descripting comment in the code runtime: cpu sample has wrong descripting comment in the code Feb 5, 2023
@gopherbot
Copy link

Change https://go.dev/cl/465335 mentions this issue: runtime: fix cpu sample comment

@dr2chase dr2chase added the NeedsFix The path to resolution is known, but the work has not been done. label Feb 6, 2023
@mknyszek
Copy link
Contributor

mknyszek commented Feb 6, 2023

Thanks for sending a patch. In the future you don't need to file an issue about internal code comments, feel free to just send the PR. If you want to double-check your understanding, we'll hash it out in the review. If you have questions about the implementation in general, a good place to send a message https://groups.google.com/g/golang-dev.

(There's nothing inherently wrong with filing an issue, but I think it ends up being more work for you in the end and doesn't really serve the Go maintainers either.)

johanbrandhorst pushed a commit to Pryz/go that referenced this issue Feb 12, 2023
Fixes golang#58327

Change-Id: I15593e6ee42e04f2de13804ef26e0e66a2307db0
GitHub-Last-Rev: 7e0d04b
GitHub-Pull-Request: golang#58338
Reviewed-on: https://go-review.googlesource.com/c/go/+/465335
Reviewed-by: Michael Knyszek <mknyszek@google.com>
Run-TryBot: Michael Knyszek <mknyszek@google.com>
Auto-Submit: Michael Knyszek <mknyszek@google.com>
Reviewed-by: David Chase <drchase@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
@golang golang locked and limited conversation to collaborators Feb 6, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants