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 9e2833f..6da9689 100644
--- a/src/runtime/proc.go
+++ b/src/runtime/proc.go
@@ -2632,7 +2632,13 @@
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 @@
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 97f0f7a..99eb19eb 100644
--- a/src/runtime/runtime2.go
+++ b/src/runtime/runtime2.go
@@ -649,13 +649,17 @@
// 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 6c3c1a6..e8323ce 100644
--- a/src/runtime/time.go
+++ b/src/runtime/time.go
@@ -292,6 +292,7 @@
if t == pp.timers[0] {
atomic.Store64(&pp.timer0When, uint64(t.when))
}
+ atomic.Xadd(&pp.numTimers, 1)
return ok
}
@@ -370,6 +371,7 @@
if i == 0 {
updateTimer0When(pp)
}
+ atomic.Xadd(&pp.numTimers, -1)
return ok
}
@@ -394,6 +396,7 @@
ok = siftdownTimer(pp.timers, 0)
}
updateTimer0When(pp)
+ atomic.Xadd(&pp.numTimers, -1)
return ok
}
@@ -650,7 +653,7 @@
}
if atomic.Load(&pp.adjustTimers) == 0 {
if verifyTimers {
- verifyTimerHeap(pp.timers)
+ verifyTimerHeap(pp)
}
return
}
@@ -712,7 +715,7 @@
}
if verifyTimers {
- verifyTimerHeap(pp.timers)
+ verifyTimerHeap(pp)
}
}
@@ -954,21 +957,24 @@
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 @@
// 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.
