[go] runtime: inline addtimer/deltimer calls in startTimer/stopTimer
Aliaksandr Valialkin (Gerrit)
Aliaksandr Valialkin has uploaded this change for review.
runtime: inline addtimer/deltimer calls in startTimer/stopTimer
startTimer/stopTimer consist of a single call to addtimer/deltimer.
Let's inline it in order to remove call overhead.
Relevant benchmark results on linux/amd64:
name old time/op new time/op delta
Reset 77.8µs ± 1% 75.3µs ± 1% -3.16% (p=0.000 n=10+8)
Reset-2 41.1µs ± 4% 39.2µs ± 1% -4.65% (p=0.000 n=10+9)
Reset-4 32.7µs ± 1% 29.1µs ± 1% -11.01% (p=0.000 n=9+10)
Change-Id: Ie9ce4494947652425ebdf90a36c0e68e60fcfaa5
---
M src/runtime/netpoll.go
M src/runtime/time.go
M src/time/internal_test.go
3 files changed, 41 insertions(+), 47 deletions(-)
diff --git a/src/runtime/netpoll.go b/src/runtime/netpoll.go
index 8dd4fb6..13fda45 100644
--- a/src/runtime/netpoll.go
+++ b/src/runtime/netpoll.go
@@ -200,11 +200,11 @@
pd.seq++ // invalidate current timers
// Reset current timers.
if pd.rt.f != nil {
- deltimer(&pd.rt)
+ stopTimer(&pd.rt)
pd.rt.f = nil
}
if pd.wt.f != nil {
- deltimer(&pd.wt)
+ stopTimer(&pd.wt)
pd.wt.f = nil
}
// Setup new timers.
@@ -225,21 +225,21 @@
// if they differ the descriptor was reused or timers were reset.
pd.rt.arg = pd
pd.rt.seq = pd.seq
- addtimer(&pd.rt)
+ startTimer(&pd.rt)
} else {
if pd.rd > 0 {
pd.rt.f = netpollReadDeadline
pd.rt.when = pd.rd
pd.rt.arg = pd
pd.rt.seq = pd.seq
- addtimer(&pd.rt)
+ startTimer(&pd.rt)
}
if pd.wd > 0 {
pd.wt.f = netpollWriteDeadline
pd.wt.when = pd.wd
pd.wt.arg = pd
pd.wt.seq = pd.seq
- addtimer(&pd.wt)
+ startTimer(&pd.wt)
}
}
// If we set the new deadline in the past, unblock currently pending IO if any.
@@ -273,11 +273,11 @@
rg = netpollunblock(pd, 'r', false)
wg = netpollunblock(pd, 'w', false)
if pd.rt.f != nil {
- deltimer(&pd.rt)
+ stopTimer(&pd.rt)
pd.rt.f = nil
}
if pd.wt.f != nil {
- deltimer(&pd.wt)
+ stopTimer(&pd.wt)
pd.wt.f = nil
}
unlock(&pd.lock)
diff --git a/src/runtime/time.go b/src/runtime/time.go
index b9454d6..6cd8724 100644
--- a/src/runtime/time.go
+++ b/src/runtime/time.go
@@ -107,14 +107,44 @@
if raceenabled {
racerelease(unsafe.Pointer(t))
}
- addtimer(t)
+
+ tb := t.assignBucket()
+ lock(&tb.lock)
+ tb.addtimerLocked(t)
+ unlock(&tb.lock)
}
// stopTimer removes t from the timer heap if it is there.
// It returns true if t was removed, false if t wasn't even there.
//go:linkname stopTimer time.stopTimer
func stopTimer(t *timer) bool {
- return deltimer(t)
+ tb := t.tb
+
+ // Delete timer t from the heap.
+ // Do not need to update the timerproc: if it wakes up early, no big deal.
+
+ lock(&tb.lock)
+ // t may not be registered anymore and may have
+ // a bogus i (typically 0, if generated by Go).
+ // Verify it before proceeding.
+ i := t.i
+ last := len(tb.t) - 1
+ if i < 0 || i > last || tb.t[i] != t {
+ unlock(&tb.lock)
+ return false
+ }
+ if i != last {
+ tb.t[i] = tb.t[last]
+ tb.t[i].i = i
+ }
+ tb.t[last] = nil
+ tb.t = tb.t[:last]
+ if i != last {
+ siftupTimer(tb.t, i)
+ siftdownTimer(tb.t, i)
+ }
+ unlock(&tb.lock)
+ return true
}
// Go runtime.
@@ -124,13 +154,6 @@
goready(arg.(*g), 0)
}
-func addtimer(t *timer) {
- tb := t.assignBucket()
- lock(&tb.lock)
- tb.addtimerLocked(t)
- unlock(&tb.lock)
-}
-
// Add a timer to the heap and start or kick timerproc if the new timer is
// earlier than any of the others.
// Timers are locked.
@@ -160,38 +183,9 @@
}
}
-// Delete timer t from the heap.
-// Do not need to update the timerproc: if it wakes up early, no big deal.
-func deltimer(t *timer) bool {
- tb := t.tb
-
- lock(&tb.lock)
- // t may not be registered anymore and may have
- // a bogus i (typically 0, if generated by Go).
- // Verify it before proceeding.
- i := t.i
- last := len(tb.t) - 1
- if i < 0 || i > last || tb.t[i] != t {
- unlock(&tb.lock)
- return false
- }
- if i != last {
- tb.t[i] = tb.t[last]
- tb.t[i].i = i
- }
- tb.t[last] = nil
- tb.t = tb.t[:last]
- if i != last {
- siftupTimer(tb.t, i)
- siftdownTimer(tb.t, i)
- }
- unlock(&tb.lock)
- return true
-}
-
// Timerproc runs the time-driven events.
// It sleeps until the next event in the tb heap.
-// If addtimer inserts a new earlier event, it wakes timerproc early.
+// If startTimer inserts a new earlier event, it wakes timerproc early.
func timerproc(tb *timersBucket) {
tb.gp = getg()
for {
diff --git a/src/time/internal_test.go b/src/time/internal_test.go
index edd523b..83270ca 100644
--- a/src/time/internal_test.go
+++ b/src/time/internal_test.go
@@ -22,7 +22,7 @@
func CheckRuntimeTimerOverflow() {
// We manually create a runtimeTimer to bypass the overflow
// detection logic in NewTimer: we're testing the underlying
- // runtime.addtimer function.
+ // runtime.startTimer function.
r := &runtimeTimer{
when: runtimeNano() + (1<<63 - 1),
f: empty,
To view, visit change 63210. To unsubscribe, or for help writing mail filters, visit settings.
Ian Lance Taylor (Gerrit)
1 comment:
Patch Set #2, Line 228:
startTimer(&pd.rt)This actually changes the current code in that we will now run a race detector check that we did not run before. Are you sure that is correct?
To view, visit change 63210. To unsubscribe, or for help writing mail filters, visit settings.
Aliaksandr Valialkin (Gerrit)
1 comment:
Patch Set #2, Line 228:
startTimer(&pd.rt)
This actually changes the current code in that we will now run a race detector check that we did not […]
I don't actually know. This code is executed only when race detector is enabled. The following quick test passes:
go test -race -short runtime net
cc'ing @dvyukov and @rsc, who should know more about race detector.
To view, visit change 63210. To unsubscribe, or for help writing mail filters, visit settings.
Russ Cox (Gerrit)
You're making the code less clear for a very small benefit. Most programs do not call Reset in a loop. Given that inlining is a focus for the next couple Go releases, manual inlining that hurts code clarity (and possibly race detector correctness) does not seem worthwhile.
I am also surprised that inlining is saving 2 microseconds. That seems far too high. Do you know why it is so high?
Russ Cox (Gerrit)
As for the race detector, I am pretty sure the new code is incorrect. It is introducing a new happens-before edge from a net.Conn.SetDeadline call to the return from a net.Conn.Read/Write that hits a deadline expiry. If the SetDeadline happens in an unrelated goroutine from the Read/Write, the memory model does not guarantee a happens-before relation, so the edge should not be introduced.
Ian Lance Taylor (Gerrit)
Ian Lance Taylor abandoned this change.
To view, visit change 63210. To unsubscribe, or for help writing mail filters, visit settings.
Aliaksandr Valialkin (Gerrit)
Patch Set 2:
You're making the code less clear for a very small benefit. Most programs do not call Reset in a loop. Given that inlining is a focus for the next couple Go releases, manual inlining that hurts code clarity (and possibly race detector correctness) does not seem worthwhile.
Agreed.
Btw, why stopTimer doesn't contain raceacquire to be symmetric with startTimer?
> I am also surprised that inlining is saving 2 microseconds. That seems far too high. Do you know why it is so high?
This saves 2 nanoseconds per Reset call because of function call overhead -
addtimer and deltimer account for 6% of CPU time in BenchmarkReset:
(pprof) top
Showing nodes accounting for 15190ms, 93.53% of 16240ms total
Dropped 12 nodes (cum <= 81.20ms)
Showing top 10 nodes out of 17
flat flat% sum% cum cum%
3550ms 21.86% 21.86% 3550ms 21.86% runtime.unlock /home/aliaksandr/work/go-tip/src/runtime/lock_futex.go
3470ms 21.37% 43.23% 3470ms 21.37% runtime._ExternalCode /home/aliaksandr/work/go-tip/src/runtime/proc.go
3360ms 20.69% 63.92% 3360ms 20.69% runtime.lock /home/aliaksandr/work/go-tip/src/runtime/lock_futex.go
860ms 5.30% 69.21% 1440ms 8.87% runtime.(*timersBucket).addtimerLocked /home/aliaksandr/work/go-tip/src/runtime/time.go
830ms 5.11% 74.32% 4330ms 26.66% runtime.deltimer /home/aliaksandr/work/go-tip/src/runtime/time.go
710ms 4.37% 78.69% 12400ms 76.35% time.(*Timer).Reset /home/aliaksandr/work/go-tip/src/time/sleep.go
650ms 4.00% 82.70% 1470ms 9.05% time.when /home/aliaksandr/work/go-tip/src/time/sleep.go
640ms 3.94% 86.64% 5510ms 33.93% runtime.addtimer /home/aliaksandr/work/go-tip/src/runtime/time.go
580ms 3.57% 90.21% 580ms 3.57% runtime.siftupTimer /home/aliaksandr/work/go-tip/src/runtime/time.go
540ms 3.33% 93.53% 820ms 5.05% time.runtimeNano /home/aliaksandr/work/go-tip/src/runtime/time.go
Aliaksandr Valialkin (Gerrit)
oops, copy-pasted incorrect result. Here is the correct one - startTimer + stopTimer account for 2.4% of CPU time:
(pprof) peek (start|stop)Timer
Showing nodes accounting for 16.24s, 100% of 16.24s total
----------------------------------------------------------+-------------
flat flat% sum% cum cum% calls calls% + context
----------------------------------------------------------+-------------
5.70s 99.82% | time.(*Timer).Reset /home/aliaksandr/work/go-tip/src/time/sleep.go
0.01s 0.18% | time.NewTimer /home/aliaksandr/work/go-tip/src/time/sleep.go
0.20s 1.23% 1.23% 5.71s 35.16% | time.startTimer /home/aliaksandr/work/go-tip/src/runtime/time.go
5.51s 96.50% | runtime.addtimer /home/aliaksandr/work/go-tip/src/runtime/time.go
----------------------------------------------------------+-------------
4.52s 100% | time.(*Timer).Reset /home/aliaksandr/work/go-tip/src/time/sleep.go
0.19s 1.17% 2.40% 4.52s 27.83% | time.stopTimer /home/aliaksandr/work/go-tip/src/runtime/time.go
4.33s 95.80% | runtime.deltimer /home/aliaksandr/work/go-tip/src/runtime/time.go
----------------------------------------------------------+-------------
Dmitry Vyukov (Gerrit)
Patch Set 2:
Btw, why stopTimer doesn't contain raceacquire to be symmetric with startTimer?
startTimer pairs with timerproc, not with stopTimer.