runtime: severe performance drop for cgo calls in go1.22.5 #68587
Comments
|
It also looks like the Rust devs are running into this problem too: https://internals.rust-lang.org/t/who-is-doing-read-proc-self-maps-1024-at-startup/17348 |
dmitshur
added
Performance
NeedsInvestigation
|
Thanks for the report. From the Rust post it sounds like it only does the read if it is called for the main thread. Could you try calling Go from a different thread, a pthread-created one? |
|
@cherrymui I only included the rust post because it was referring to the same scenario of calling In the context of the original issue it's being called through Python loading up the shared library and then making calls to it. As this is a client library that we provide we don't have control over whether consumers call from the main thread or other threads. I can try calling from another thread instead and seeing if that mitigates the issue a bit, but that wouldn't be a viable solution IMO as we can't expect all calls to the shared library to be funneled through a separate thread instead of being able to use the main thread. Right? |
|
Yes, trying calling from a different thread is for validating the assumption. Calling on the main thread should not be slow, either. So if that's the case we'll look into a targeted solution. Thanks. |
|
Looks like you're right! I updated the code from the reproducer to the following: #include "libtrivial.h"
#include <stdio.h>
#include <stdlib.h>
#include <pthread.h>
void* f(void*) {
int a = 0;
for (int i = 0; i < 500; ++i) {
a = trivialFunctionCall((size_t)i);
}
printf("Done %d\n", a);
pthread_exit(NULL);
}
int main() {
pthread_t thr;
int rc;
if ((rc = pthread_create(&thr, NULL, &f, NULL)) {
fprintf(stderr, "error: pthread_create: rc: %d\n", rc);
return EXIT_FAILURE;
}
pthread_join(thr, NULL);
}After running the result using So this just needs to be handled for the main thread as it appears to not cause slowdowns on other threads, only if you call from the main thread. |
seankhliao
changed the title
gopherbot
added
the
compiler/runtime
|
In triage, we're thinking we're probably stuck with having to work around this by caching the stack bounds one way or another. |
|
@cherrymui @mknyszek any updates here? |
|
Sorry, I haven't got a chance to implement this. My plan is that if we can get an accurate stack bounds, which is the common case with glibc, we just get it once per thread and stick with it. If we cannot get the accurate bounds (on systems that |
|
Thanks for the update! That's about what I was expecting the fix would end up being |
|
Workaround for now is to revert 3560cf0 if using Go 1.23, NOTE this has quite severe performance degradation for graphical programs calling c-shared Go functions every frame. |
|
The Teradata SQL Driver for Python is also impacted by this performance regression in Go 1.22.5. The driver is a thin Python wrapper around a shared library implemented in Go. |
|
I've also experienced this slowdown; in a minimal piece of code where I've implemented an identity function and exported it to Node.js via N-API, Go 1.23 can only do about 5,000 ops per second (roughly 204,000 ns per op). Reverting 3560cf0 brings that up to 2.5 million ops per second (~400 ns per op), a 500x difference. |
|
@cherrymui @mknyszek Since this is a change introduced in a minor version, there are performance degradations mentioned in the current issue, as well as some possible potential functional issues We are currently unable to update to the latest upstream go1.22 version to follow some bugfixes (such as 6fab4b9 ) and CVE fixes |
|
Marking as a release blocker since it seems to have a real impact on some programs. |
|
@gopherbot , please open a backport to 1.23. |
|
Backport issue(s) opened: #69988 (for 1.23). Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://go.dev/wiki/MinorReleases. |
|
Just a thought: 3560cf0 was to deal with an OS thread's stack potentially moving between calls, which is a fairly obscure condition. If we discover the stack has moved, could we just set that g0's stack bounds to [0, ∞) and stop worrying about it? That will lose bounds checks on the g0 stack, which is unfortunate, but again this is uncommon. Is there anything more critical this would mess up? |
|
Change https://go.dev/cl/600296 mentions this issue: |
|
I updated CL 600296 to cover this, implemented basically #68587 (comment). If the stack moved, we use a new bounds of [SP, SP+32KB), which I think is not that different from [0, ∞). I tried on a Linux glibc machine and that gets mostly of the performance back: Feel free to try the CL, and let me know if there is any issue. Thanks! |
dmitshur
added
NeedsFix
dr2chase
added
the
Critical
|
Change https://go.dev/cl/635775 mentions this issue: |
…t cgocallback Currently, at a cgo callback where there is already a Go frame on the stack (i.e. C->Go->C->Go), we require that at the inner Go callback the SP is within the g0's stack bounds set by a previous callback. This is to prevent that the C code switches stack while having a Go frame on the stack, which we don't really support. But this could also happen when we cannot get accurate stack bounds, e.g. when pthread_getattr_np is not available. Since the stack bounds are just estimates based on the current SP, if there are multiple C->Go callbacks with various stack depth, it is possible that the SP of a later callback falls out of a previous call's estimate. This leads to runtime throw in a seemingly reasonable program. This CL changes it to save the old g0 stack bounds at cgocallback, update the bounds, and restore the old bounds at return. So each callback will get its own stack bounds based on the current SP, and when it returns, the outer callback has the its old stack bounds restored. Also, at a cgo callback when there is no Go frame on the stack, we currently always get new stack bounds. We do this because if we can only get estimated bounds based on the SP, and the stack depth varies a lot between two C->Go calls, the previous estimates may be off and we fall out or nearly fall out of the previous bounds. But this causes a performance problem: the pthread API to get accurate stack bounds (pthread_getattr_np) is very slow when called on the main thread. Getting the stack bounds every time significantly slows down repeated C->Go calls on the main thread. This CL fixes it by "caching" the stack bounds if they are accurate. I.e. at the second time Go calls into C, if the previous stack bounds are accurate, and the current SP is in bounds, we can be sure it is the same stack and we don't need to update the bounds. This avoids the repeated calls to pthread_getattr_np. If we cannot get the accurate bounds, we continue to update the stack bounds based on the SP, and that operation is very cheap. On a Linux/AMD64 machine with glibc: name old time/op new time/op delta CgoCallbackMainThread-8 96.4µs ± 3% 0.1µs ± 2% -99.92% (p=0.000 n=10+9) Updates #68285. Updates #68587. Fixes #69988. Change-Id: I3422badd5ad8ff63e1a733152d05fb7a44d5d435 Reviewed-on: https://go-review.googlesource.com/c/go/+/600296 LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com> Reviewed-by: Michael Pratt <mpratt@google.com> (cherry picked from commit 76a8409) Reviewed-on: https://go-review.googlesource.com/c/go/+/635775
…t cgocallback Currently, at a cgo callback where there is already a Go frame on the stack (i.e. C->Go->C->Go), we require that at the inner Go callback the SP is within the g0's stack bounds set by a previous callback. This is to prevent that the C code switches stack while having a Go frame on the stack, which we don't really support. But this could also happen when we cannot get accurate stack bounds, e.g. when pthread_getattr_np is not available. Since the stack bounds are just estimates based on the current SP, if there are multiple C->Go callbacks with various stack depth, it is possible that the SP of a later callback falls out of a previous call's estimate. This leads to runtime throw in a seemingly reasonable program. This CL changes it to save the old g0 stack bounds at cgocallback, update the bounds, and restore the old bounds at return. So each callback will get its own stack bounds based on the current SP, and when it returns, the outer callback has the its old stack bounds restored. Also, at a cgo callback when there is no Go frame on the stack, we currently always get new stack bounds. We do this because if we can only get estimated bounds based on the SP, and the stack depth varies a lot between two C->Go calls, the previous estimates may be off and we fall out or nearly fall out of the previous bounds. But this causes a performance problem: the pthread API to get accurate stack bounds (pthread_getattr_np) is very slow when called on the main thread. Getting the stack bounds every time significantly slows down repeated C->Go calls on the main thread. This CL fixes it by "caching" the stack bounds if they are accurate. I.e. at the second time Go calls into C, if the previous stack bounds are accurate, and the current SP is in bounds, we can be sure it is the same stack and we don't need to update the bounds. This avoids the repeated calls to pthread_getattr_np. If we cannot get the accurate bounds, we continue to update the stack bounds based on the SP, and that operation is very cheap. On a Linux/AMD64 machine with glibc: name old time/op new time/op delta CgoCallbackMainThread-8 96.4µs ± 3% 0.1µs ± 2% -99.92% (p=0.000 n=10+9) Updates golang#68285. Updates golang#68587. Fixes golang#69988. Change-Id: I3422badd5ad8ff63e1a733152d05fb7a44d5d435 Reviewed-on: https://go-review.googlesource.com/c/go/+/600296 LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com> Reviewed-by: Michael Pratt <mpratt@google.com> (cherry picked from commit 76a8409) Reviewed-on: https://go-review.googlesource.com/c/go/+/635775
…t cgocallback Currently, at a cgo callback where there is already a Go frame on the stack (i.e. C->Go->C->Go), we require that at the inner Go callback the SP is within the g0's stack bounds set by a previous callback. This is to prevent that the C code switches stack while having a Go frame on the stack, which we don't really support. But this could also happen when we cannot get accurate stack bounds, e.g. when pthread_getattr_np is not available. Since the stack bounds are just estimates based on the current SP, if there are multiple C->Go callbacks with various stack depth, it is possible that the SP of a later callback falls out of a previous call's estimate. This leads to runtime throw in a seemingly reasonable program. This CL changes it to save the old g0 stack bounds at cgocallback, update the bounds, and restore the old bounds at return. So each callback will get its own stack bounds based on the current SP, and when it returns, the outer callback has the its old stack bounds restored. Also, at a cgo callback when there is no Go frame on the stack, we currently always get new stack bounds. We do this because if we can only get estimated bounds based on the SP, and the stack depth varies a lot between two C->Go calls, the previous estimates may be off and we fall out or nearly fall out of the previous bounds. But this causes a performance problem: the pthread API to get accurate stack bounds (pthread_getattr_np) is very slow when called on the main thread. Getting the stack bounds every time significantly slows down repeated C->Go calls on the main thread. This CL fixes it by "caching" the stack bounds if they are accurate. I.e. at the second time Go calls into C, if the previous stack bounds are accurate, and the current SP is in bounds, we can be sure it is the same stack and we don't need to update the bounds. This avoids the repeated calls to pthread_getattr_np. If we cannot get the accurate bounds, we continue to update the stack bounds based on the SP, and that operation is very cheap. On a Linux/AMD64 machine with glibc: name old time/op new time/op delta CgoCallbackMainThread-8 96.4µs ± 3% 0.1µs ± 2% -99.92% (p=0.000 n=10+9) Updates golang#68285. Updates golang#68587. Fixes golang#69988. Change-Id: I3422badd5ad8ff63e1a733152d05fb7a44d5d435 Reviewed-on: https://go-review.googlesource.com/c/go/+/600296 LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com> Reviewed-by: Michael Pratt <mpratt@google.com> (cherry picked from commit 76a8409) Reviewed-on: https://go-review.googlesource.com/c/go/+/635775
Go version
go version go1.22.5 linux/amd64
Output of
go envin your module/workspace:What did you do?
Building a shared library with Go that is loaded and called from C. I was able to reproduce a trivial example with the following:
Build with
go build -o libtrivial.so -buildmode=c-shared main.goBuild with
gcc -o trivial trivial.c -ltrivial -L. -Wl,-rpath,..Then we run and record the performance:
perf record --call-graph lbr -- ./trivialand look at the report withperf report -g.What did you see happen?
The overwhelming amount of time in the cgocall is spent in
__isoc99_sscanfas seen here:If we instead build using
GOTOOLCHAIN=go1.22.4 go build -o libtrivial.so -buildmode=c-shared main.goand re-run it, you see that the cost ofcallbackUpdateSystemStackandpthread_getattr_npdon't even register in the performance trace:The problem was introduced by 3560cf0afb which forces all calls to cgo to update the system stack bounds. Ultimately the problem traces down to the glibc implementation of
pthread_getattr_npwhich is called bygo/src/runtime/cgo/gcc_stack_unix.c
Line 28 in 792a261
In glibc,
pthread_getattr_npends up directly parsing/proc/self/mapsby usingsscanf, hence the high cost as seen here https://github.com/lattera/glibc/blob/master/nptl/pthread_getattr_np.c#L81-L124It looks like musl's implementation of
pthread_getattr_npisn't as expensive as glibc's version though.For the full, in-context explanation see apache/arrow-adbc#2021 in context we're seeing a test run go from ~2s to now taking over 5.5s due to this issue and are concerned it may end up affecting consumers of the library
What did you expect to see?
Shifting from
go1.22.4togo1.22.5shouldn't come with such a significant drop in cgo's FFI performance when calling a Go shared library function from C that the cost is dominated by calls tosscanfif the underlying Go code itself isn't expensive enough. While the example used to reproduce is a trivial one to exemplify the problem, in the context of the actual library, the cgo calls are non-trivial and involve multiple network connection initializations, data processing and more, yet several of the calls end up completely dominated by thesscanfcall inpthread_getattr_np.Either we need find more situations where we can avoid having to fully update the stack bounds via
x_cgo_getstackboundor there needs to be a less expensive way to get those bounds than usingpthread_getattr_npwhen using glibc due to the high cost of it just parsing/proc/self/maps.The text was updated successfully, but these errors were encountered: