From d35dfd405eaa21654807fac3891e198538d3c402 Mon Sep 17 00:00:00 2001 From: Austin Clements Date: Sun, 30 Oct 2016 17:59:06 -0400 Subject: [PATCH] runtime: make assists perform root jobs Currently, assists can only perform heap marking jobs. However, at the beginning of GC, there are only root jobs and no heap marking jobs. As a result, there's often a period at the beginning of a GC cycle where no goroutine has accumulated assist credit, but at the same time it can't get any credit because there are no heap marking jobs for it to do yet. As a result, many goroutines often block on the assist queue at the very beginning of the GC cycle. This commit fixes this by allowing assists to perform root marking jobs. The tricky part of this (and the reason we haven't done this before) is that stack scanning jobs can lead to deadlocks if the goroutines performing the stack scanning are themselves non-preemptible, since two non-preemptible goroutines may try to scan each other. To address this, we use the same insight d6625ca used to simplify the mark worker stack scanning: as long as we're careful with the stacks and only drain jobs while on the system stack, we can put the goroutine into a preemptible state while we drain jobs. This means an assist's user stack can be scanned while it continues to do work. This reduces the rate of assist blocking in the x/benchmarks HTTP benchmark by a factor of 3 and all remaining blocking happens towards the *end* of the GC cycle, when there may genuinely not be enough work to go around. Ideally, assists would get credit for working on root jobs. Currently they do not; however, this change prioritizes heap work over root jobs in assists, so they're likely to mostly perform heap work. In contrast with mark workers, for assists, the root jobs act only as a backstop to create heap work when there isn't enough heap work. Fixes #15361. Change-Id: If6e169863e4ad75710b0c8dc00f6125b41e9a595 Reviewed-on: https://go-review.googlesource.com/32432 Reviewed-by: Rick Hudson --- src/runtime/mgcmark.go | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/src/runtime/mgcmark.go b/src/runtime/mgcmark.go index 30a73178ac..71092cb19d 100644 --- a/src/runtime/mgcmark.go +++ b/src/runtime/mgcmark.go @@ -521,6 +521,10 @@ func gcAssistAlloc1(gp *g, scanWork int64) { throw("nwait > work.nprocs") } + // gcDrainN requires the caller to be preemptible. + casgstatus(gp, _Grunning, _Gwaiting) + gp.waitreason = "GC assist marking" + // drain own cached work first in the hopes that it // will be more cache friendly. gcw := &getg().m.p.ptr().gcw @@ -531,6 +535,8 @@ func gcAssistAlloc1(gp *g, scanWork int64) { gcw.dispose() } + casgstatus(gp, _Gwaiting, _Grunning) + // Record that we did this much scan work. // // Back out the number of bytes of assist credit that @@ -1083,7 +1089,13 @@ func gcDrain(gcw *gcWork, flags gcDrainFlags) { // buffer. Otherwise, it will perform at least n units of work, but // may perform more because scanning is always done in whole object // increments. It returns the amount of scan work performed. +// +// The caller goroutine must be in a preemptible state (e.g., +// _Gwaiting) to prevent deadlocks during stack scanning. As a +// consequence, this must be called on the system stack. +// //go:nowritebarrier +//go:systemstack func gcDrainN(gcw *gcWork, scanWork int64) int64 { if !writeBarrier.needed { throw("gcDrainN phase incorrect") @@ -1111,6 +1123,18 @@ func gcDrainN(gcw *gcWork, scanWork int64) int64 { } if b == 0 { + // Try to do a root job. + // + // TODO: Assists should get credit for this + // work. + if work.markrootNext < work.markrootJobs { + job := atomic.Xadd(&work.markrootNext, +1) - 1 + if job < work.markrootJobs { + markroot(gcw, job) + continue + } + } + // No heap or root jobs. break } scanobject(b, gcw) -- 2.48.1