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

The flame graph is incorrect when using go 1.23. #33

Closed
manchurio opened this issue Aug 20, 2024 · 2 comments
Closed

The flame graph is incorrect when using go 1.23. #33

manchurio opened this issue Aug 20, 2024 · 2 comments

Comments

@manchurio
Copy link

manchurio commented Aug 20, 2024

package main

import (
	"fmt"
	"github.com/felixge/fgprof"
	"net/http"
	"os"
	"time"
)

func main() {
	cpuf, err := os.Create("fgprof.out")
	if err != nil {
		return
	}
	stop := fgprof.Start(cpuf, fgprof.FormatPprof)
	defer stop()
	for range 30 {
		networkTask()
		cpuTask()
	}
}

func networkTask() {
	resp, err := http.Get("http://google.com")
	if err != nil {
		fmt.Println(err)
		return
	}
	defer resp.Body.Close()
}

func cpuTask() {
	start := time.Now()
	for time.Since(start) < 100*time.Millisecond {
		for i := 0; i < 10000; i++ {
			_ = i
		}
	}
}

As shown in the image, cputask and networkTask should be at the same level, but now cpuTask is below networkTask.

image

As a comparison, this is for Go 1.22.

image

@felixge
Copy link
Owner

felixge commented Aug 23, 2024

Thanks for reporting this. This might be an upstream regression, potentially in some patches submitted by my colleague and me for the go1.23 release. I'll investigate.

@felixge
Copy link
Owner

felixge commented Aug 30, 2024

I can confirm that this is a regression I introduced upstream, sorry about that.

I've submitted a fix and will ask for getting it back-ported to a go1.23.x release.

Meanwhile I've created a workaround and tagged a new v0.9.5 release that contains it. Upgrading should fix the issue on your end.

gopherbot pushed a commit to golang/go that referenced this issue Sep 25, 2024
Fix a regression introduced in CL 572396 causing goroutine stacks not
getting null terminated.

This bug impacts callers that reuse the []StackRecord slice for multiple
calls to GoroutineProfile. See felixge/fgprof#33
for an example of the problem.

Add a test case to prevent similar regressions in the future. Use null
padding instead of null termination to be consistent with other profile
types and because it's less code to implement. Also fix the
ThreadCreateProfile code path.

Fixes #69243

Change-Id: I0b9414f6c694c304bc03a5682586f619e9bf0588
Reviewed-on: https://go-review.googlesource.com/c/go/+/609815
Reviewed-by: Tim King <[email protected]>
LUCI-TryBot-Result: Go LUCI <[email protected]>
Reviewed-by: Michael Pratt <[email protected]>
gopherbot pushed a commit to golang/go that referenced this issue Oct 21, 2024
…ing null terminated

Fix a regression introduced in CL 572396 causing goroutine stacks not
getting null terminated.

This bug impacts callers that reuse the []StackRecord slice for multiple
calls to GoroutineProfile. See felixge/fgprof#33
for an example of the problem.

Add a test case to prevent similar regressions in the future. Use null
padding instead of null termination to be consistent with other profile
types and because it's less code to implement. Also fix the
ThreadCreateProfile code path.

Fixes #69258

Change-Id: I0b9414f6c694c304bc03a5682586f619e9bf0588
Reviewed-on: https://go-review.googlesource.com/c/go/+/609815
Reviewed-by: Tim King <[email protected]>
LUCI-TryBot-Result: Go LUCI <[email protected]>
Reviewed-by: Michael Pratt <[email protected]>
(cherry picked from commit 49e542a)
Reviewed-on: https://go-review.googlesource.com/c/go/+/621277
Reviewed-by: Cherry Mui <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants