godeltaprof tests are failing on gotip

[korniltsev]

Failing tests:
https://github.com/grafana/pyroscope-go/actions/runs/9072542438/job/24932183708

This bot PR #102 suggests this may be caused by this upstream change https://go-review.googlesource.com/c/go/+/533258

[korniltsev]

this may be caused by this upstream change https://go-review.googlesource.com/c/go/+/533258

Indeed, this commit is the reason.

The actual reason is that before the change this line of code https://github.com/golang/go/blob/22344e11f27d9667e7bbb6209df59e9a9e976d91/src/runtime/mprof.go#L776
used stack from callers / gcallers which does skip logical frames and performs inline unwinding and looks like this

[
   0x48f931, //  sync.(*Mutex).Unlock
   0x48f918,  // main.main.func1
   0x46ba81,  // runtime.goexit
]

And after the change the same line uses stack from fpTracebackPCs
which looks like this:

[
   0x4,      // skip value
   0x467405, // sync.event 
   0x44ad65, // runtime.semrelease1
   0x468533, // sync.runtime_Semrelease 
   0x476b5b, // sync.(*Mutex).unlockSlow << this function has a branch, so value here depends on Mutex state
   0x48f931, // main.main.func1
   0x46ba81, // runtime.goexit
]

Because of that, runtime.stkbucket allocates two bucket s for the same logical stack and therefore later we get two BlockProfileRecord with equal stack (and godeltaprof assumed all BlockProfileRecords are unique)

Here is a short program to reproduce:

https://go.dev/play/p/YD8N3UVACna?v=gotip

gotip:

=========================
443e01  runtime._LostContendedRuntimeLock
=========================
48f931  sync.(*Mutex).Unlock
48f918  main.main.func1
46ba81  runtime.goexit
=========================
48f931  sync.(*Mutex).Unlock
48f918  main.main.func1
46ba81  runtime.goexit

1.22:

=========================
4401a1  runtime._LostContendedRuntimeLock
=========================
48f931  sync.(*Mutex).Unlock
48f918  main.main.func1
46ba81  runtime.goexit

[korniltsev]

This is not necessary a runtime bug, but I've submitted a golang issue anyway just to make sure they are aware of this (implicit?) change. golang/go#67548

I think it make sense to defend godeltaprof against this change even if the change is fixed upstream.

[korniltsev]

There is a similar issue with the heap profile, where map internals are elided from the end of call stacks and we can get multiple entries with the same stack in the pprof.

We should have a test for this case if this is the case

[stefanb]

Probably a release should be tagged, so that it can be referenced from dependants (including grafana) to be buildable with upcoming Go 1.23

See build attempt in homebrew:

• go 1.23, go@1.22 1.22.6 (new formula) Homebrew/homebrew-core#175310

https://github.com/Homebrew/homebrew-core/actions/runs/9617673150/job/26535459459#step:3:10143

  go: downloading github.com/grafana/pyroscope-go/godeltaprof v0.1.6
  go: downloading github.com/grafana/authlib v0.0.0-20240319083410-9d4a6e3861e5
  # github.com/grafana/pyroscope-go/godeltaprof/internal/pprof
  /Users/brew/Library/Caches/Homebrew/go_mod_cache/pkg/mod/github.com/grafana/pyroscope-go/godeltaprof@v0.1.6/internal/pprof/delta_mutex.go:30:20: undefined: runtime_cyclesPerSecond
  /Users/brew/Library/Caches/Homebrew/go_mod_cache/pkg/mod/github.com/grafana/pyroscope-go/godeltaprof@v0.1.6/internal/pprof/proto.go:320:8: undefined: runtime_expandFinalInlineFrame

[nsrip-dd]

👋 I'm working on an upstream fix for this. What I have so far is here: https://go-review.googlesource.com/c/go/+/595966. Is it alright if we use the reproducer you created as a regression test case? If so, I can of course add any attribution you think is appropriate to the CL :)

[korniltsev]

sure, feel free to use the reproducer if it suits your needs