runtime: don't skip checkTimers if we would clear deleted timers

The timers code used to have a problem: if code started and stopped a
lot of timers, as would happen with, for example, lots of calls to
context.WithTimeout, then it would steadily use memory holding timers
that had stopped but not been removed from the timer heap.
That problem was fixed by CL 214299, which would remove all deleted
timers whenever they got to be more than 1/4 of the total number of
timers on the heap.

The timers code had a different problem: if there were some idle P's,
the running P's would have lock contention trying to steal their timers.
That problem was fixed by CL 214185, which only acquired the timer lock
if the next timer was ready to run or there were some timers to adjust.

Unfortunately, CL 214185 partially undid 214299, in that we could now
accumulate an increasing number of deleted timers while there were no
timers ready to run. This CL restores the 214299 behavior, by checking
whether there are lots of deleted timers without acquiring the lock.

This is a performance issue to consider for the 1.14 release.

Change-Id: I13c980efdcc2a46eb84882750c39e3f7c5b2e7c3
Reviewed-on: https://go-review.googlesource.com/c/go/+/215722
Run-TryBot: Ian Lance Taylor <iant@golang.org>
Reviewed-by: Austin Clements <austin@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>

diff --git a/src/runtime/proc.go b/src/runtime/proc.go
index 9e2833fe6ac71b9f54d6085f2508573a4fd5d790..6da9689703f6927ebf9075a86cf157d8e146bfd3 100644 (file)
--- a/src/runtime/proc.go
+++ b/src/runtime/proc.go
@@ -2632,7 +2632,13 @@ func checkTimers(pp *p, now int64) (rnow, pollUntil int64, ran bool) {
                        now = nanotime()
                }
                if now < next {
-                       return now, next, false
+                       // Next timer is not ready to run.
+                       // But keep going if we would clear deleted timers.
+                       // This corresponds to the condition below where
+                       // we decide whether to call clearDeletedTimers.
+                       if pp != getg().m.p.ptr() || int(atomic.Load(&pp.deletedTimers)) <= int(atomic.Load(&pp.numTimers)/4) {
+                               return now, next, false
+                       }
                }
        }
 
@@ -4108,6 +4114,7 @@ func (pp *p) destroy() {
                lock(&pp.timersLock)
                moveTimers(plocal, pp.timers)
                pp.timers = nil
+               pp.numTimers = 0
                pp.adjustTimers = 0
                pp.deletedTimers = 0
                atomic.Store64(&pp.timer0When, 0)

diff --git a/src/runtime/runtime2.go b/src/runtime/runtime2.go
index 97f0f7a66240027cedd949dd54a4a7267340bd54..99eb19eb0c02e270eba165905bd6169736dc3bbc 100644 (file)
--- a/src/runtime/runtime2.go
+++ b/src/runtime/runtime2.go
@@ -649,13 +649,17 @@ type p struct {
        // Must hold timersLock to access.
        timers []*timer
 
+       // Number of timers in P's heap.
+       // Modified using atomic instructions.
+       numTimers uint32
+
        // Number of timerModifiedEarlier timers on P's heap.
        // This should only be modified while holding timersLock,
        // or while the timer status is in a transient state
        // such as timerModifying.
        adjustTimers uint32
 
-       // Number of timerDeleted times in P's heap.
+       // Number of timerDeleted timers in P's heap.
        // Modified using atomic instructions.
        deletedTimers uint32
 

diff --git a/src/runtime/time.go b/src/runtime/time.go
index 6c3c1a63c4a1e88c37d38d52602d1f47f07002e8..e8323ce0e6a69d997d57a6d642366c3a0bac90e6 100644 (file)
--- a/src/runtime/time.go
+++ b/src/runtime/time.go
@@ -292,6 +292,7 @@ func doaddtimer(pp *p, t *timer) bool {
        if t == pp.timers[0] {
                atomic.Store64(&pp.timer0When, uint64(t.when))
        }
+       atomic.Xadd(&pp.numTimers, 1)
        return ok
 }
 
@@ -370,6 +371,7 @@ func dodeltimer(pp *p, i int) bool {
        if i == 0 {
                updateTimer0When(pp)
        }
+       atomic.Xadd(&pp.numTimers, -1)
        return ok
 }
 
@@ -394,6 +396,7 @@ func dodeltimer0(pp *p) bool {
                ok = siftdownTimer(pp.timers, 0)
        }
        updateTimer0When(pp)
+       atomic.Xadd(&pp.numTimers, -1)
        return ok
 }
 
@@ -650,7 +653,7 @@ func adjusttimers(pp *p) {
        }
        if atomic.Load(&pp.adjustTimers) == 0 {
                if verifyTimers {
-                       verifyTimerHeap(pp.timers)
+                       verifyTimerHeap(pp)
                }
                return
        }
@@ -712,7 +715,7 @@ loop:
        }
 
        if verifyTimers {
-               verifyTimerHeap(pp.timers)
+               verifyTimerHeap(pp)
        }
 }
 
@@ -954,21 +957,24 @@ nextTimer:
                timers[i] = nil
        }
 
-       timers = timers[:to]
-       if verifyTimers {
-               verifyTimerHeap(timers)
-       }
-       pp.timers = timers
        atomic.Xadd(&pp.deletedTimers, -cdel)
+       atomic.Xadd(&pp.numTimers, -cdel)
        atomic.Xadd(&pp.adjustTimers, -cearlier)
+
+       timers = timers[:to]
+       pp.timers = timers
        updateTimer0When(pp)
+
+       if verifyTimers {
+               verifyTimerHeap(pp)
+       }
 }
 
 // verifyTimerHeap verifies that the timer heap is in a valid state.
 // This is only for debugging, and is only called if verifyTimers is true.
 // The caller must have locked the timers.
-func verifyTimerHeap(timers []*timer) {
-       for i, t := range timers {
+func verifyTimerHeap(pp *p) {
+       for i, t := range pp.timers {
                if i == 0 {
                        // First timer has no parent.
                        continue
@@ -976,11 +982,15 @@ func verifyTimerHeap(timers []*timer) {
 
                // The heap is 4-ary. See siftupTimer and siftdownTimer.
                p := (i - 1) / 4
-               if t.when < timers[p].when {
-                       print("bad timer heap at ", i, ": ", p, ": ", timers[p].when, ", ", i, ": ", t.when, "\n")
+               if t.when < pp.timers[p].when {
+                       print("bad timer heap at ", i, ": ", p, ": ", pp.timers[p].when, ", ", i, ": ", t.when, "\n")
                        throw("bad timer heap")
                }
        }
+       if numTimers := int(atomic.Load(&pp.numTimers)); len(pp.timers) != numTimers {
+               println("timer heap len", len(pp.timers), "!= numTimers", numTimers)
+               throw("bad timer heap len")
+       }
 }
 
 // updateTimer0When sets the P's timer0When field.