Skip to content

Commit

Permalink
Revert "runtime: remove GODEBUG=runtimecontentionstacks"
Browse files Browse the repository at this point in the history
This reverts commit 87e930f (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>
  • Loading branch information
rhysh authored and gopherbot committed May 30, 2024
1 parent e6b8b2f commit ca7d300
Show file tree
Hide file tree
Showing 7 changed files with 37 additions and 11 deletions.
3 changes: 0 additions & 3 deletions doc/godebug.md
Original file line number Diff line number Diff line change
Expand Up @@ -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).
Expand Down
5 changes: 0 additions & 5 deletions doc/next/6-stdlib/99-minor/runtime/pprof/66999.md

This file was deleted.

9 changes: 9 additions & 0 deletions src/runtime/extern.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
18 changes: 15 additions & 3 deletions src/runtime/metrics_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ package runtime_test

import (
"bytes"
"fmt"
"internal/abi"
"internal/goexperiment"
"internal/profile"
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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",
}}

Expand Down Expand Up @@ -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",
},
}
Expand Down
5 changes: 5 additions & 0 deletions src/runtime/mprof.go
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down
6 changes: 6 additions & 0 deletions src/runtime/pprof/pprof.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 2 additions & 0 deletions src/runtime/runtime1.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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},
Expand Down

0 comments on commit ca7d300

Please sign in to comment.