[go] runtime: respect timeout in semasleep on Darwin
86 views
Skip to first unread message
Nikhil Benesch (Gerrit)
Jun 23, 2018, 1:30:38 AM6/23/18
to Ian Lance Taylor, goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com
Nikhil Benesch has uploaded this change for review.
runtime: respect timeout in semasleep on Darwin
semasleep on Darwin was refactored in https://golang.org/cl/118736 to
use the pthread_cond_timedwait function from libc. The new code
incorrectly assumed that pthread_cond_timedwait took a timeout relative
to the current time, when it in fact it takes a timeout specified in
absolute time. semasleep thus specified a timeout well in the past,
which was equivalent to specifying an infinite timeout. This caused a
large performance hit to CockroachDB (#26019).
Adjust semasleep to instead call pthread_cond_timedwait_relative_np,
which properly interprets its timeout parameter as relative to the
current time.
pthread_cond_timedwait_relative_np is non-portable, but using
pthread_cond_timedwait correctly would require two calls to
gettimeofday: one in the runtime package to convert the relative timeout
to absolute time, then another in the pthread library to convert back to
a relative offset [0], as the Darwin kernel expects a relative offset.
[0]: https://opensource.apple.com/source/libpthread/libpthread-301.30.1/src/pthread_cond.c.auto.html
Fix #26019.
Change-Id: I1a8c2429f79513b43d2b256365cd9166d235af8b
---
M src/runtime/os_darwin.go
M src/runtime/sys_darwin.go
M src/runtime/sys_darwin_386.s
M src/runtime/sys_darwin_amd64.s
M src/runtime/sys_darwin_arm.s
M src/runtime/sys_darwin_arm64.s
6 files changed, 13 insertions(+), 13 deletions(-)
diff --git a/src/runtime/os_darwin.go b/src/runtime/os_darwin.go
index 5019b94..ff37500 100644
--- a/src/runtime/os_darwin.go
+++ b/src/runtime/os_darwin.go
@@ -45,7 +45,7 @@
if ns >= 0 {
var t timespec
t.set_nsec(ns)
- err := pthread_cond_timedwait(&mp.cond, &mp.mutex, &t)
+ err := pthread_cond_timedwait_relative_np(&mp.cond, &mp.mutex, &t)
if err == _ETIMEDOUT {
pthread_mutex_unlock(&mp.mutex)
return -1
diff --git a/src/runtime/sys_darwin.go b/src/runtime/sys_darwin.go
index ef5aef1..f0d0815 100644
--- a/src/runtime/sys_darwin.go
+++ b/src/runtime/sys_darwin.go
@@ -287,10 +287,10 @@
//go:nosplit
//go:cgo_unsafe_args
-func pthread_cond_timedwait(c *pthreadcond, m *pthreadmutex, t *timespec) int32 {
- return libcCall(unsafe.Pointer(funcPC(pthread_cond_timedwait_trampoline)), unsafe.Pointer(&c))
+func pthread_cond_timedwait_relative_np(c *pthreadcond, m *pthreadmutex, t *timespec) int32 {
+ return libcCall(unsafe.Pointer(funcPC(pthread_cond_timedwait_relative_np_trampoline)), unsafe.Pointer(&c))
}
-func pthread_cond_timedwait_trampoline()
+func pthread_cond_timedwait_relative_np_trampoline()
//go:nosplit
//go:cgo_unsafe_args
@@ -348,7 +348,7 @@
//go:cgo_import_dynamic libc_pthread_mutex_unlock pthread_mutex_unlock "/usr/lib/libSystem.B.dylib"
//go:cgo_import_dynamic libc_pthread_cond_init pthread_cond_init "/usr/lib/libSystem.B.dylib"
//go:cgo_import_dynamic libc_pthread_cond_wait pthread_cond_wait "/usr/lib/libSystem.B.dylib"
-//go:cgo_import_dynamic libc_pthread_cond_timedwait pthread_cond_timedwait "/usr/lib/libSystem.B.dylib"
+//go:cgo_import_dynamic libc_pthread_cond_timedwait_relative_np pthread_cond_timedwait_relative_np "/usr/lib/libSystem.B.dylib"
//go:cgo_import_dynamic libc_pthread_cond_signal pthread_cond_signal "/usr/lib/libSystem.B.dylib"
// Magic incantation to get libSystem actually dynamically linked.
diff --git a/src/runtime/sys_darwin_386.s b/src/runtime/sys_darwin_386.s
index c0903e7..09f1228 100644
--- a/src/runtime/sys_darwin_386.s
+++ b/src/runtime/sys_darwin_386.s
@@ -595,7 +595,7 @@
POPL BP
RET
-TEXT runtime·pthread_cond_timedwait_trampoline(SB),NOSPLIT,$0
+TEXT runtime·pthread_cond_timedwait_relative_np_trampoline(SB),NOSPLIT,$0
PUSHL BP
MOVL SP, BP
SUBL $24, SP
@@ -606,7 +606,7 @@
MOVL AX, 4(SP)
MOVL 8(CX), AX // arg 3 timeout
MOVL AX, 8(SP)
- CALL libc_pthread_cond_timedwait(SB)
+ CALL libc_pthread_cond_timedwait_relative_np(SB)
MOVL BP, SP
POPL BP
RET
diff --git a/src/runtime/sys_darwin_amd64.s b/src/runtime/sys_darwin_amd64.s
index 5522a86..1429335 100644
--- a/src/runtime/sys_darwin_amd64.s
+++ b/src/runtime/sys_darwin_amd64.s
@@ -447,13 +447,13 @@
POPQ BP
RET
-TEXT runtime·pthread_cond_timedwait_trampoline(SB),NOSPLIT,$0
+TEXT runtime·pthread_cond_timedwait_relative_np_trampoline(SB),NOSPLIT,$0
PUSHQ BP
MOVQ SP, BP
MOVQ 8(DI), SI // arg 2 mutex
MOVQ 16(DI), DX // arg 3 timeout
MOVQ 0(DI), DI // arg 1 cond
- CALL libc_pthread_cond_timedwait(SB)
+ CALL libc_pthread_cond_timedwait_relative_np(SB)
POPQ BP
RET
diff --git a/src/runtime/sys_darwin_arm.s b/src/runtime/sys_darwin_arm.s
index 5b3f553..9b5c667 100644
--- a/src/runtime/sys_darwin_arm.s
+++ b/src/runtime/sys_darwin_arm.s
@@ -368,11 +368,11 @@
BL libc_pthread_cond_wait(SB)
RET
-TEXT runtime·pthread_cond_timedwait_trampoline(SB),NOSPLIT,$0
+TEXT runtime·pthread_cond_timedwait_relative_np_trampoline(SB),NOSPLIT,$0
MOVW 4(R0), R1 // arg 2 mutex
MOVW 8(R0), R2 // arg 3 timeout
MOVW 0(R0), R0 // arg 1 cond
- BL libc_pthread_cond_timedwait(SB)
+ BL libc_pthread_cond_timedwait_relative_np(SB)
RET
TEXT runtime·pthread_cond_signal_trampoline(SB),NOSPLIT,$0
diff --git a/src/runtime/sys_darwin_arm64.s b/src/runtime/sys_darwin_arm64.s
index eb01774..c324994 100644
--- a/src/runtime/sys_darwin_arm64.s
+++ b/src/runtime/sys_darwin_arm64.s
@@ -357,11 +357,11 @@
BL libc_pthread_cond_wait(SB)
RET
-TEXT runtime·pthread_cond_timedwait_trampoline(SB),NOSPLIT,$0
+TEXT runtime·pthread_cond_timedwait_relative_np_trampoline(SB),NOSPLIT,$0
MOVD 8(R0), R1 // arg 2 mutex
MOVD 16(R0), R2 // arg 3 timeout
MOVD 0(R0), R0 // arg 1 cond
- BL libc_pthread_cond_timedwait(SB)
+ BL libc_pthread_cond_timedwait_relative_np(SB)
RET
TEXT runtime·pthread_cond_signal_trampoline(SB),NOSPLIT,$0
To view, visit change 120635. To unsubscribe, or for help writing mail filters, visit settings.
Nikhil Benesch (Gerrit)
Jun 23, 2018, 2:31:53 AM6/23/18
to Keith Randall, goph...@pubsubhelper.golang.org, Martin Möhrmann, golang-co...@googlegroups.com
Nikhil Benesch uploaded patch set #2 to this change.
runtime: respect timeout in semasleep on Darwin
semasleep on Darwin was refactored in https://golang.org/cl/118736 to
use the pthread_cond_timedwait function from libc. The new code
incorrectly assumed that pthread_cond_timedwait took a timeout relative
to the current time, when it in fact it takes a timeout specified in
absolute time. semasleep thus specified a timeout well in the past,
causing it to immediately exceed the timeout and spin hot. This was the
source of a large performance hit to CockroachDB (#26019).
Adjust semasleep to instead call pthread_cond_timedwait_relative_np,
which properly interprets its timeout parameter as relative to the
current time.
pthread_cond_timedwait_relative_np is non-portable, but using
pthread_cond_timedwait correctly would require two calls to
gettimeofday: one in the runtime package to convert the relative timeout
to absolute time, then another in the pthread library to convert back to
a relative offset [0], as the Darwin kernel expects a relative offset.
[0]: https://opensource.apple.com/source/libpthread/libpthread-301.30.1/src/pthread_cond.c.auto.html
Fix #26019.
Change-Id: I1a8c2429f79513b43d2b256365cd9166d235af8b
---
M src/runtime/os_darwin.go
M src/runtime/sys_darwin.go
M src/runtime/sys_darwin_386.s
M src/runtime/sys_darwin_amd64.s
M src/runtime/sys_darwin_arm.s
M src/runtime/sys_darwin_arm64.s
6 files changed, 13 insertions(+), 13 deletions(-)
To view, visit change 120635. To unsubscribe, or for help writing mail filters, visit settings.
Keith Randall (Gerrit)
Jun 23, 2018, 8:03:11 AM6/23/18
to Nikhil Benesch, goph...@pubsubhelper.golang.org, Keith Randall, Martin Möhrmann, golang-co...@googlegroups.com
Patch set 2:Code-Review +2
Josh Bleecher Snyder (Gerrit)
Jun 23, 2018, 9:05:52 AM6/23/18
to Nikhil Benesch, goph...@pubsubhelper.golang.org, Josh Bleecher Snyder, Keith Randall, Martin Möhrmann, golang-co...@googlegroups.com
Patch set 2:Run-TryBot +1
Gobot Gobot (Gerrit)
Jun 23, 2018, 9:06:01 AM6/23/18
to Nikhil Benesch, goph...@pubsubhelper.golang.org, Josh Bleecher Snyder, Keith Randall, Martin Möhrmann, golang-co...@googlegroups.com
TryBots beginning. Status page: https://farmer.golang.org/try?commit=5427da4b
Gobot Gobot (Gerrit)
Jun 23, 2018, 9:17:26 AM6/23/18
to Nikhil Benesch, goph...@pubsubhelper.golang.org, Josh Bleecher Snyder, Keith Randall, Martin Möhrmann, golang-co...@googlegroups.com
TryBots are happy.
Patch set 2:TryBot-Result +1
Josh Bleecher Snyder (Gerrit)
Jun 24, 2018, 10:22:08 PM6/24/18
to Josh Bleecher Snyder, Nikhil Benesch, goph...@pubsubhelper.golang.org, golang-...@googlegroups.com, Kevin Burke, Gobot Gobot, Keith Randall, Martin Möhrmann, golang-co...@googlegroups.com
Josh Bleecher Snyder merged this change.
Approvals:
Keith Randall: Looks good to me, approved
Josh Bleecher Snyder: Run TryBots
Gobot Gobot: TryBots succeeded
runtime: respect timeout in semasleep on Darwin
semasleep on Darwin was refactored in https://golang.org/cl/118736 to
use the pthread_cond_timedwait function from libc. The new code
incorrectly assumed that pthread_cond_timedwait took a timeout relative
to the current time, when it in fact it takes a timeout specified in
absolute time. semasleep thus specified a timeout well in the past,
causing it to immediately exceed the timeout and spin hot. This was the
source of a large performance hit to CockroachDB (#26019).
Adjust semasleep to instead call pthread_cond_timedwait_relative_np,
which properly interprets its timeout parameter as relative to the
current time.
pthread_cond_timedwait_relative_np is non-portable, but using
pthread_cond_timedwait correctly would require two calls to
gettimeofday: one in the runtime package to convert the relative timeout
to absolute time, then another in the pthread library to convert back to
a relative offset [0], as the Darwin kernel expects a relative offset.
[0]: https://opensource.apple.com/source/libpthread/libpthread-301.30.1/src/pthread_cond.c.auto.html
Fix #26019.
Change-Id: I1a8c2429f79513b43d2b256365cd9166d235af8b
Reviewed-on: https://go-review.googlesource.com/120635
Reviewed-by: Keith Randall <k...@golang.org>
Run-TryBot: Josh Bleecher Snyder <josh...@gmail.com>
TryBot-Result: Gobot Gobot <go...@golang.org>
Reply all
Reply to author
Forward
0 new messages