runtime: refactor finalizer goroutine status

Use an atomic.Uint32 to represent the state of finalizer goroutine.
fingStatus will only be changed to fingWake in non fingWait state,
so it is safe to set fingRunningFinalizer status in runfinq.

name            old time/op  new time/op  delta
Finalizer-8      592µs ± 4%   561µs ± 1%  -5.22%  (p=0.000 n=10+10)
FinalizerRun-8   694ns ± 6%   675ns ± 7%    ~     (p=0.059 n=9+8)

Change-Id: I7e4da30cec98ce99f7d8cf4c97f933a8a2d1cae1
Reviewed-on: https://go-review.googlesource.com/c/go/+/400134
Reviewed-by: Joedian Reid <joedian@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Daniel Martí <mvdan@mvdan.cc>
Reviewed-by: Michael Pratt <mpratt@google.com>

diff --git a/src/runtime/export_test.go b/src/runtime/export_test.go
index 77e4279b9d1e528ff7d113e8993eb2981b0aa462..c29d64a88507d3b383d1c05acc387b113d3ba252 100644 (file)
--- a/src/runtime/export_test.go
+++ b/src/runtime/export_test.go
@@ -1264,10 +1264,7 @@ func SetIntArgRegs(a int) int {
 }
 
 func FinalizerGAsleep() bool {
-       lock(&finlock)
-       result := fingwait
-       unlock(&finlock)
-       return result
+       return fingStatus.Load()&fingWait != 0
 }
 
 // For GCTestMoveStackOnNextCall, it's important not to introduce an

diff --git a/src/runtime/mfinal.go b/src/runtime/mfinal.go
index 9de364c260b2d83d423983d64b7e41370e3c1a60..ef11b7df96dda67dfcb689b4d258e06b67a6940b 100644 (file)
--- a/src/runtime/mfinal.go
+++ b/src/runtime/mfinal.go
@@ -29,13 +29,23 @@ type finblock struct {
        fin     [(_FinBlockSize - 2*goarch.PtrSize - 2*4) / unsafe.Sizeof(finalizer{})]finalizer
 }
 
+var fingStatus atomic.Uint32
+
+// finalizer goroutine status.
+const (
+       fingUninitialized uint32 = iota
+       fingCreated       uint32 = 1 << (iota - 1)
+       fingRunningFinalizer
+       fingWait
+       fingWake
+)
+
 var finlock mutex  // protects the following variables
 var fing *g        // goroutine that runs finalizers
 var finq *finblock // list of finalizers that are to be executed
 var finc *finblock // cache of free blocks
 var finptrmask [_FinBlockSize / goarch.PtrSize / 8]byte
-var fingwait bool
-var fingwake bool
+
 var allfin *finblock // list of all blocks
 
 // NOTE: Layout known to queuefinalizer.
@@ -126,8 +136,8 @@ func queuefinalizer(p unsafe.Pointer, fn *funcval, nret uintptr, fint *_type, ot
        f.fint = fint
        f.ot = ot
        f.arg = p
-       fingwake = true
        unlock(&finlock)
+       fingStatus.Or(fingWake)
 }
 
 //go:nowritebarrier
@@ -141,29 +151,27 @@ func iterate_finq(callback func(*funcval, unsafe.Pointer, uintptr, *_type, *ptrt
 }
 
 func wakefing() *g {
-       var res *g
-       lock(&finlock)
-       if fingwait && fingwake {
-               fingwait = false
-               fingwake = false
-               res = fing
+       if ok := fingStatus.CompareAndSwap(fingCreated|fingWait|fingWake, fingCreated); ok {
+               return fing
        }
-       unlock(&finlock)
-       return res
+       return nil
 }
 
-var (
-       fingCreate  uint32
-       fingRunning bool
-)
-
 func createfing() {
        // start the finalizer goroutine exactly once
-       if fingCreate == 0 && atomic.Cas(&fingCreate, 0, 1) {
+       if fingStatus.Load() == fingUninitialized && fingStatus.CompareAndSwap(fingUninitialized, fingCreated) {
                go runfinq()
        }
 }
 
+func finalizercommit(gp *g, lock unsafe.Pointer) bool {
+       unlock((*mutex)(lock))
+       // fingStatus should be modified after fing is put into a waiting state
+       // to avoid waking fing in running state, even if it is about to be parked.
+       fingStatus.Or(fingWait)
+       return true
+}
+
 // This is the goroutine that runs all of the finalizers
 func runfinq() {
        var (
@@ -182,8 +190,7 @@ func runfinq() {
                fb := finq
                finq = nil
                if fb == nil {
-                       fingwait = true
-                       goparkunlock(&finlock, waitReasonFinalizerWait, traceEvGoBlock, 1)
+                       gopark(finalizercommit, unsafe.Pointer(&finlock), waitReasonFinalizerWait, traceEvGoBlock, 1)
                        continue
                }
                argRegs = intArgRegs
@@ -244,9 +251,9 @@ func runfinq() {
                                default:
                                        throw("bad kind in runfinq")
                                }
-                               fingRunning = true
+                               fingStatus.Or(fingRunningFinalizer)
                                reflectcall(nil, unsafe.Pointer(f.fn), frame, uint32(framesz), uint32(framesz), uint32(framesz), &regs)
-                               fingRunning = false
+                               fingStatus.And(^fingRunningFinalizer)
 
                                // Drop finalizer queue heap references
                                // before hiding them from markroot.

diff --git a/src/runtime/mprof.go b/src/runtime/mprof.go
index 6547b6b56bca577573637f1effe7c7cc2a584744..8cef0b0601a03e04c8433a051d81263ea074b869 100644 (file)
--- a/src/runtime/mprof.go
+++ b/src/runtime/mprof.go
@@ -917,7 +917,7 @@ func goroutineProfileWithLabelsConcurrent(p []StackRecord, labels []unsafe.Point
        // doesn't change during the collection. So, check the finalizer goroutine
        // in particular.
        n = int(gcount())
-       if fingRunning {
+       if fingStatus.Load()&fingRunningFinalizer != 0 {
                n++
        }
 

diff --git a/src/runtime/proc.go b/src/runtime/proc.go
index 49f2caceacfe24f0a43d68c0e3cc7d5af643b1bf..9ebb25bfd0caceb278b4cf91857833025f407080 100644 (file)
--- a/src/runtime/proc.go
+++ b/src/runtime/proc.go
@@ -2628,7 +2628,7 @@ top:
        }
 
        // Wake up the finalizer G.
-       if fingwait && fingwake {
+       if fingStatus.Load()&(fingWait|fingWake) == fingWait|fingWake {
                if gp := wakefing(); gp != nil {
                        ready(gp, 0, true)
                }

diff --git a/src/runtime/traceback.go b/src/runtime/traceback.go
index a9bec426d1b60b798c77b2e3ab15d819d069edb7..4cc5eb91c820180bcb4b5c4784f68eaa6660a773 100644 (file)
--- a/src/runtime/traceback.go
+++ b/src/runtime/traceback.go
@@ -1051,7 +1051,7 @@ func isSystemGoroutine(gp *g, fixed bool) bool {
                        // always consider it a user goroutine.
                        return false
                }
-               return !fingRunning
+               return fingStatus.Load()&fingRunningFinalizer == 0
        }
        return hasPrefix(funcname(f), "runtime.")
 }