]> Cypherpunks repositories - gostls13.git/commitdiff
runtime: don't hold worldsema across mark phase
authorMichael Anthony Knyszek <mknyszek@google.com>
Mon, 17 Jun 2019 19:03:09 +0000 (19:03 +0000)
committerMichael Knyszek <mknyszek@google.com>
Wed, 18 Mar 2020 19:13:50 +0000 (19:13 +0000)
This change makes it so that worldsema isn't held across the mark phase.
This means that various operations like ReadMemStats may now stop the
world during the mark phase, reducing latency on such operations.

Only three such operations are still no longer allowed to occur during
marking: GOMAXPROCS, StartTrace, and StopTrace.

For the former it's because any change to GOMAXPROCS impacts GC mark
background worker scheduling and the details there are tricky.

For the latter two it's because tracing needs to observe consistent GC
start and GC end events, and if StartTrace or StopTrace may stop the
world during marking, then it's possible for it to see a GC end event
without a start or GC start event without an end, respectively.

To ensure that GOMAXPROCS and StartTrace/StopTrace cannot proceed until
marking is complete, the runtime now holds a new semaphore, gcsema,
across the mark phase just like it used to with worldsema.

This change is being landed once more after being reverted in the Go
1.14 release cycle, since CL 215157 allows it to have a positive
effect on system performance.

For the benchmark BenchmarkReadMemStatsLatency in the runtime, which
measures ReadMemStats latencies while the GC is exercised, the tail of
these latencies reduced dramatically on an 8-core machine:

name                   old 50%tile-ns  new 50%tile-ns  delta
ReadMemStatsLatency-8      4.40M ±74%      0.12M ± 2%  -97.35%  (p=0.008 n=5+5)

name                   old 90%tile-ns  new 90%tile-ns  delta
ReadMemStatsLatency-8       102M ± 6%         0M ±14%  -99.79%  (p=0.008 n=5+5)

name                   old 99%tile-ns  new 99%tile-ns  delta
ReadMemStatsLatency-8       147M ±18%         4M ±57%  -97.43%  (p=0.008 n=5+5)

Fixes #19812.

Change-Id: If66c3c97d171524ae29f0e7af4bd33509d9fd0bb
Reviewed-on: https://go-review.googlesource.com/c/go/+/216557
Run-TryBot: Michael Knyszek <mknyszek@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Cherry Zhang <cherryyz@google.com>
src/runtime/debug.go
src/runtime/mgc.go
src/runtime/proc.go
src/runtime/trace.go
src/runtime/trace/trace_stack_test.go

index af5c3a11709fad64ffa2d31b14ae3185096016c6..76eeb2e41afd82049dfbcd46cd36c333286c268b 100644 (file)
@@ -26,12 +26,12 @@ func GOMAXPROCS(n int) int {
                return ret
        }
 
-       stopTheWorld("GOMAXPROCS")
+       stopTheWorldGC("GOMAXPROCS")
 
        // newprocs will be processed by startTheWorld
        newprocs = int32(n)
 
-       startTheWorld()
+       startTheWorldGC()
        return ret
 }
 
index 604d7d09b4831d2413a8a2053f49540e9e75b2de..bda8eadc9d17af993704094b1a2cb98f10341093 100644 (file)
@@ -1269,6 +1269,7 @@ func gcStart(trigger gcTrigger) {
        }
 
        // Ok, we're doing it! Stop everybody else
+       semacquire(&gcsema)
        semacquire(&worldsema)
 
        if trace.enabled {
@@ -1367,6 +1368,13 @@ func gcStart(trigger gcTrigger) {
                work.pauseNS += now - work.pauseStart
                work.tMark = now
        })
+
+       // Release the world sema before Gosched() in STW mode
+       // because we will need to reacquire it later but before
+       // this goroutine becomes runnable again, and we could
+       // self-deadlock otherwise.
+       semrelease(&worldsema)
+
        // In STW mode, we could block the instant systemstack
        // returns, so don't do anything important here. Make sure we
        // block rather than returning to user code.
@@ -1436,6 +1444,10 @@ top:
                return
        }
 
+       // forEachP needs worldsema to execute, and we'll need it to
+       // stop the world later, so acquire worldsema now.
+       semacquire(&worldsema)
+
        // Flush all local buffers and collect flushedWork flags.
        gcMarkDoneFlushed = 0
        systemstack(func() {
@@ -1496,6 +1508,7 @@ top:
                // work to do. Keep going. It's possible the
                // transition condition became true again during the
                // ragged barrier, so re-check it.
+               semrelease(&worldsema)
                goto top
        }
 
@@ -1572,6 +1585,7 @@ top:
                                now := startTheWorldWithSema(true)
                                work.pauseNS += now - work.pauseStart
                        })
+                       semrelease(&worldsema)
                        goto top
                }
        }
@@ -1789,6 +1803,7 @@ func gcMarkTermination(nextTriggerRatio float64) {
        }
 
        semrelease(&worldsema)
+       semrelease(&gcsema)
        // Careful: another GC cycle may start now.
 
        releasem(mp)
index 6f143cbe187b6963b53972ebe526a7dda1812b27..c7097e290698f445cb4e9769dc42bb9a252d56d2 100644 (file)
@@ -858,8 +858,23 @@ func casGFromPreempted(gp *g, old, new uint32) bool {
 // goroutines.
 func stopTheWorld(reason string) {
        semacquire(&worldsema)
-       getg().m.preemptoff = reason
-       systemstack(stopTheWorldWithSema)
+       gp := getg()
+       gp.m.preemptoff = reason
+       systemstack(func() {
+               // Mark the goroutine which called stopTheWorld preemptible so its
+               // stack may be scanned.
+               // This lets a mark worker scan us while we try to stop the world
+               // since otherwise we could get in a mutual preemption deadlock.
+               // We must not modify anything on the G stack because a stack shrink
+               // may occur. A stack shrink is otherwise OK though because in order
+               // to return from this function (and to leave the system stack) we
+               // must have preempted all goroutines, including any attempting
+               // to scan our stack, in which case, any stack shrinking will
+               // have already completed by the time we exit.
+               casgstatus(gp, _Grunning, _Gwaiting)
+               stopTheWorldWithSema()
+               casgstatus(gp, _Gwaiting, _Grunning)
+       })
 }
 
 // startTheWorld undoes the effects of stopTheWorld.
@@ -871,10 +886,31 @@ func startTheWorld() {
        getg().m.preemptoff = ""
 }
 
-// Holding worldsema grants an M the right to try to stop the world
-// and prevents gomaxprocs from changing concurrently.
+// stopTheWorldGC has the same effect as stopTheWorld, but blocks
+// until the GC is not running. It also blocks a GC from starting
+// until startTheWorldGC is called.
+func stopTheWorldGC(reason string) {
+       semacquire(&gcsema)
+       stopTheWorld(reason)
+}
+
+// startTheWorldGC undoes the effects of stopTheWorldGC.
+func startTheWorldGC() {
+       startTheWorld()
+       semrelease(&gcsema)
+}
+
+// Holding worldsema grants an M the right to try to stop the world.
 var worldsema uint32 = 1
 
+// Holding gcsema grants the M the right to block a GC, and blocks
+// until the current GC is done. In particular, it prevents gomaxprocs
+// from changing concurrently.
+//
+// TODO(mknyszek): Once gomaxprocs and the execution tracer can handle
+// being changed/enabled during a GC, remove this.
+var gcsema uint32 = 1
+
 // stopTheWorldWithSema is the core implementation of stopTheWorld.
 // The caller is responsible for acquiring worldsema and disabling
 // preemption first and then should stopTheWorldWithSema on the system
index 67a84425a8895c9916928924e0eda3f4fe4c926c..9aa9facabee43728565581fa146240bdbb11ebc5 100644 (file)
@@ -180,9 +180,12 @@ func traceBufPtrOf(b *traceBuf) traceBufPtr {
 // Most clients should use the runtime/trace package or the testing package's
 // -test.trace flag instead of calling StartTrace directly.
 func StartTrace() error {
-       // Stop the world, so that we can take a consistent snapshot
+       // Stop the world so that we can take a consistent snapshot
        // of all goroutines at the beginning of the trace.
-       stopTheWorld("start tracing")
+       // Do not stop the world during GC so we ensure we always see
+       // a consistent view of GC-related events (e.g. a start is always
+       // paired with an end).
+       stopTheWorldGC("start tracing")
 
        // We are in stop-the-world, but syscalls can finish and write to trace concurrently.
        // Exitsyscall could check trace.enabled long before and then suddenly wake up
@@ -193,7 +196,7 @@ func StartTrace() error {
 
        if trace.enabled || trace.shutdown {
                unlock(&trace.bufLock)
-               startTheWorld()
+               startTheWorldGC()
                return errorString("tracing is already enabled")
        }
 
@@ -264,7 +267,7 @@ func StartTrace() error {
 
        unlock(&trace.bufLock)
 
-       startTheWorld()
+       startTheWorldGC()
        return nil
 }
 
@@ -273,14 +276,14 @@ func StartTrace() error {
 func StopTrace() {
        // Stop the world so that we can collect the trace buffers from all p's below,
        // and also to avoid races with traceEvent.
-       stopTheWorld("stop tracing")
+       stopTheWorldGC("stop tracing")
 
        // See the comment in StartTrace.
        lock(&trace.bufLock)
 
        if !trace.enabled {
                unlock(&trace.bufLock)
-               startTheWorld()
+               startTheWorldGC()
                return
        }
 
@@ -317,7 +320,7 @@ func StopTrace() {
        trace.shutdown = true
        unlock(&trace.bufLock)
 
-       startTheWorld()
+       startTheWorldGC()
 
        // The world is started but we've set trace.shutdown, so new tracing can't start.
        // Wait for the trace reader to flush pending buffers and stop.
index 62c06e67d9d37436ec34c5469f5eb01f6eff63fa..e3608c687fa70311d407a9452e3d7b3fdf9eda9b 100644 (file)
@@ -233,6 +233,7 @@ func TestTraceSymbolize(t *testing.T) {
                }},
                {trace.EvGomaxprocs, []frame{
                        {"runtime.startTheWorld", 0}, // this is when the current gomaxprocs is logged.
+                       {"runtime.startTheWorldGC", 0},
                        {"runtime.GOMAXPROCS", 0},
                        {"runtime/trace_test.TestTraceSymbolize", 0},
                        {"testing.tRunner", 0},