Revert "runtime: remove GODEBUG=runtimecontentionstacks"

This reverts commit 87e930f7289136fad1310d4b63dd4127e409bac5 (CL 585639)

Reason for revert: This is part of a patch series that changed the
handling of contended lock2/unlock2 calls, reducing the maximum
throughput of contended runtime.mutex values, and causing a performance
regression on applications where that is (or became) the bottleneck.

Updates #66999
Updates #67585

Change-Id: I1e286d2a16d16e4af202cd5dc04b2d9c4ee71b32
Reviewed-on: https://go-review.googlesource.com/c/go/+/589097
Reviewed-by: Than McIntosh <thanm@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Michael Pratt <mpratt@google.com>
Auto-Submit: Rhys Hiltner <rhys.hiltner@gmail.com>

diff --git a/doc/godebug.md b/doc/godebug.md
index 649bcc598296821e573a3ae4c4b2f1e19736c21f..071acf98cfbe8775468aaae67bdd41df688cdfb7 100644 (file)
--- a/doc/godebug.md
+++ b/doc/godebug.md
@@ -176,9 +176,6 @@ This behavior is controlled by the `winreadlinkvolume` setting.
 For Go 1.23, it defaults to `winreadlinkvolume=1`.
 Previous versions default to `winreadlinkvolume=0`.
 
-Go 1.23 corrected the semantics of contention reports for runtime-internal locks,
-and so removed the [`runtimecontentionstacks` setting](/pkg/runtime#hdr-Environment_Variable).
-
 Go 1.23 enabled the experimental post-quantum key exchange mechanism
 X25519Kyber768Draft00 by default. The default can be reverted using the
 [`tlskyber` setting](/pkg/crypto/tls/#Config.CurvePreferences).

diff --git a/doc/next/6-stdlib/99-minor/runtime/pprof/66999.md b/doc/next/6-stdlib/99-minor/runtime/pprof/66999.md
deleted file mode 100644 (file)
index 7943b17..0000000
--- a/doc/next/6-stdlib/99-minor/runtime/pprof/66999.md
+++ /dev/null
@@ -1,5 +0,0 @@
-The mutex profile for contention on runtime-internal locks now correctly points
-to the end of the critical section that caused the delay. This matches the
-profile's behavior for contention on `sync.Mutex` values. The
-`runtimecontentionstacks` setting for `GODEBUG`, which allowed opting in to Go
-1.22's unusual behavior for this part of the profile, is now gone.

diff --git a/src/runtime/extern.go b/src/runtime/extern.go
index 96efcf3273f5c36e0dac7a4978b8817307218037..2019be4ddec4f3758507b069c5dd8a4a8eae164e 100644 (file)
--- a/src/runtime/extern.go
+++ b/src/runtime/extern.go
@@ -159,6 +159,15 @@ It is a comma-separated list of name=val pairs setting these named variables:
        panicnil: setting panicnil=1 disables the runtime error when calling panic with nil
        interface value or an untyped nil.
 
+       runtimecontentionstacks: setting runtimecontentionstacks=1 enables inclusion of call stacks
+       related to contention on runtime-internal locks in the "mutex" profile, subject to the
+       MutexProfileFraction setting. When runtimecontentionstacks=0, contention on
+       runtime-internal locks will report as "runtime._LostContendedRuntimeLock". When
+       runtimecontentionstacks=1, the call stacks will correspond to the unlock call that released
+       the lock. But instead of the value corresponding to the amount of contention that call
+       stack caused, it corresponds to the amount of time the caller of unlock had to wait in its
+       original call to lock. A future release is expected to align those and remove this setting.
+
        invalidptr: invalidptr=1 (the default) causes the garbage collector and stack
        copier to crash the program if an invalid pointer value (for example, 1)
        is found in a pointer-typed location. Setting invalidptr=0 disables this check.

diff --git a/src/runtime/metrics_test.go b/src/runtime/metrics_test.go
index fbf19e2b5e5a7582b28d1921a18cba06cf6a50e6..ebbf0e4fd07e6df20696ac561609beea8a50e463 100644 (file)
--- a/src/runtime/metrics_test.go
+++ b/src/runtime/metrics_test.go
@@ -6,6 +6,7 @@ package runtime_test
 
 import (
        "bytes"
+       "fmt"
        "internal/abi"
        "internal/goexperiment"
        "internal/profile"
@@ -954,6 +955,17 @@ func TestRuntimeLockMetricsAndProfile(t *testing.T) {
                t.Fatalf("need MutexProfileRate 0, got %d", old)
        }
 
+       {
+               before := os.Getenv("GODEBUG")
+               for _, s := range strings.Split(before, ",") {
+                       if strings.HasPrefix(s, "runtimecontentionstacks=") {
+                               t.Logf("GODEBUG includes explicit setting %q", s)
+                       }
+               }
+               defer func() { os.Setenv("GODEBUG", before) }()
+               os.Setenv("GODEBUG", fmt.Sprintf("%s,runtimecontentionstacks=1", before))
+       }
+
        t.Logf("NumCPU %d", runtime.NumCPU())
        t.Logf("GOMAXPROCS %d", runtime.GOMAXPROCS(0))
        if minCPU := 2; runtime.NumCPU() < minCPU {
@@ -1152,7 +1164,7 @@ func TestRuntimeLockMetricsAndProfile(t *testing.T) {
 
                stks := [][]string{{
                        "runtime.unlock",
-                       "runtime_test." + name + ".func4.1",
+                       "runtime_test." + name + ".func5.1",
                        "runtime_test.(*contentionWorker).run",
                }}
 
@@ -1258,14 +1270,14 @@ func TestRuntimeLockMetricsAndProfile(t *testing.T) {
                        {
                                "runtime.unlock",
                                "runtime.semrelease1",
-                               "runtime_test.TestRuntimeLockMetricsAndProfile.func5.1",
+                               "runtime_test.TestRuntimeLockMetricsAndProfile.func6.1",
                                "runtime_test.(*contentionWorker).run",
                        },
                        {
                                "runtime.unlock",
                                "runtime.semacquire1",
                                "runtime.semacquire",
-                               "runtime_test.TestRuntimeLockMetricsAndProfile.func5.1",
+                               "runtime_test.TestRuntimeLockMetricsAndProfile.func6.1",
                                "runtime_test.(*contentionWorker).run",
                        },
                }

diff --git a/src/runtime/mprof.go b/src/runtime/mprof.go
index fd0a0187245432b6561b9c0cc33cf1f281258469..93d49275c9760f1e9b0ed23a3833d4878daf8456 100644 (file)
--- a/src/runtime/mprof.go
+++ b/src/runtime/mprof.go
@@ -915,6 +915,11 @@ func (prof *mLockProfile) captureStack() {
        }
 
        prof.stack[0] = logicalStackSentinel
+       if debug.runtimeContentionStacks.Load() == 0 {
+               prof.stack[1] = abi.FuncPCABIInternal(_LostContendedRuntimeLock) + sys.PCQuantum
+               prof.stack[2] = 0
+               return
+       }
 
        var nstk int
        gp := getg()

diff --git a/src/runtime/pprof/pprof.go b/src/runtime/pprof/pprof.go
index 43ef66f0b03fe571c29737b475f654d0b8279ed6..be17e598754de1eaf7fddc876dfdd5180e5dfedf 100644 (file)
--- a/src/runtime/pprof/pprof.go
+++ b/src/runtime/pprof/pprof.go
@@ -166,6 +166,12 @@ import (
 // holds a lock for 1s while 5 other goroutines are waiting for the entire
 // second to acquire the lock, its unlock call stack will report 5s of
 // contention.
+//
+// Runtime-internal locks are always reported at the location
+// "runtime._LostContendedRuntimeLock". More detailed stack traces for
+// runtime-internal locks can be obtained by setting
+// `GODEBUG=runtimecontentionstacks=1` (see package [runtime] docs for
+// caveats).
 type Profile struct {
        name  string
        mu    sync.Mutex

diff --git a/src/runtime/runtime1.go b/src/runtime/runtime1.go
index c74f6d2c72a44375a5311715e693fa151846be71..03ef74b8dc4b54d860ba488fe1c4b71867a6ee33 100644 (file)
--- a/src/runtime/runtime1.go
+++ b/src/runtime/runtime1.go
@@ -319,6 +319,7 @@ var debug struct {
        gctrace                  int32
        invalidptr               int32
        madvdontneed             int32 // for Linux; issue 28466
+       runtimeContentionStacks  atomic.Int32
        scavtrace                int32
        scheddetail              int32
        schedtrace               int32
@@ -380,6 +381,7 @@ var dbgvars = []*dbgVar{
        {name: "madvdontneed", value: &debug.madvdontneed},
        {name: "panicnil", atomic: &debug.panicnil},
        {name: "profstackdepth", value: &debug.profstackdepth, def: 128},
+       {name: "runtimecontentionstacks", atomic: &debug.runtimeContentionStacks},
        {name: "sbrk", value: &debug.sbrk},
        {name: "scavtrace", value: &debug.scavtrace},
        {name: "scheddetail", value: &debug.scheddetail},