Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cmd/cgo: add #cgo noescape/nocallback annotations #56378

Closed
doujiang24 opened this issue Oct 22, 2022 · 35 comments
Closed

cmd/cgo: add #cgo noescape/nocallback annotations #56378

doujiang24 opened this issue Oct 22, 2022 · 35 comments

Comments

@doujiang24
Copy link
Contributor

This is for better performance, which will avoid escaping.

example:

//go:cgo_unsafe_stack_pointer str
void strpointer(void *str) {
    // must not callback to go
}

Now, the str pointer must be allocated on the heap, for safety.

AFAIK, the unsafe cases:

  1. C code calls back into Go code, and the Go code triggers a stack copy, the str pointer might move. from: cmd/cgo: replace _Cgo_use with runtime.KeepAlive? #20281 (comment)
  2. GC may trigger shrinkstack, the str pointer might move, before C returns.

So, when people use //go:cgo_unsafe_stack_pointer,
they must make sure the C code will not call back to go, it's an unsafe usage.

And, the go compiler needs these changes:

  1. skip generating _Cgo_use for str pointer.
  2. skip shrinkstack when the goroutine is invoking such an unsafe C function (we could set a flag under g, before invoking the C function).
@gopherbot gopherbot added this to the Proposal milestone Oct 22, 2022
@ianlancetaylor ianlancetaylor moved this to Incoming in Proposals Oct 22, 2022
@ianlancetaylor ianlancetaylor changed the title proposal: cgo: add "//go:cgo_unsafe_stack_pointer" to avoid escaping proposal: cmd/cgo: add "//go:cgo_unsafe_stack_pointer" to avoid escaping Oct 22, 2022
@doujiang24
Copy link
Contributor Author

  1. GC may trigger shrinkstack, the str pointer might move, before C returns.

Oh, sorry. I made a mistake here. The stack won't move while in syscall.

Then, seems this can be easier, just need to skip _Cgo_use.

@aclements
Copy link
Member

Alternatively, we could have an annotation that says a C call does not call back into Go. That captures a user intent rather than a language implementation detail, and we can easily check that the call obeys this at run-time.

@aclements
Copy link
Member

//go:cgo_no_go_callback? We use the term "callback" in the runtime for this, but it could be interpreted as "this doesn't take a callback function pointer."

//go:cgo_no_go_calls?

@rsc
Copy link
Contributor

rsc commented Feb 1, 2023

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

@rsc rsc moved this from Incoming to Active in Proposals Feb 1, 2023
@bcmills
Copy link
Contributor

bcmills commented Feb 1, 2023

we can easily check that the call obeys this at run-time.

I don't think it's sufficient just to check that the same C thread doesn't call back into Go. What if the C thread passes the pointer to a different C thread, when then passes that pointer back to a Go function?

@aclements
Copy link
Member

That's an interesting point. That's probably something we'd want to account for in the documentation of any such directive (and maybe in its name), but I'm not sure that having a check that doesn't trip in a tiny fraction of cases is a deal-breaker.

@hherman1
Copy link

hherman1 commented Feb 2, 2023

How much of a performance improvement might we expect from using this directive? And in what situations? I’m trying to understand where this proposal is coming from.

@doujiang24
Copy link
Contributor Author

Thanks all, happy to see this proposal is active.

That's probably something we'd want to account for in the documentation of any such directive (and maybe in its name)

yep, I think it's an unsafe annotation - for better performance, since we can not protect it at run-time.

For its name, I think the unsafe keywords may be deserved.
But, I have no good idea for the full name if we want a high-level name, not implementation details.

@doujiang24
Copy link
Contributor Author

How much of a performance improvement might we expect from using this directive? And in what situations? I’m trying to understand where this proposal is coming from.

Hi @hherman1

That depends, it could be very significant - when it's heavily using cgo and escaping Go objects frequency - with large GC overhead.

We are implementing the Envoy Golang filter extension, https://www.envoyproxy.io/docs/envoy/latest/configuration/http/http_filters/golang_filter
That's the heavy cgo use case, it may lead to multiple Go objects escaping in each request, and that could a significant overhead.

@ianlancetaylor
Copy link
Contributor

Note that I believe that it is possible today for a goroutine that is suspended in a call to C to have its stack shrunk. That is OK because the current escaping implementation mean that no stack allocated address can be passed to C. If we implement this new pragma, then I think that in order to make it useful it will have to somehow disable stack shrinking for any call to a C function using this pragma.

@ianlancetaylor
Copy link
Contributor

Upon further thought I don't fully understand where this pragma should be placed. Currently the cgo tool makes no attempt to parse any portion of the cgo comment. It just passes everything to the C compiler. So I don't see how the pragma can appear in the cgo comment. But I also don't understand where else it could appear.

@doujiang24
Copy link
Contributor Author

Thanks @ianlancetaylor
In the current implementation, maybe isShrinkStackSafe is protecting a goroutine from shrinking while invoking
into C? https://github.com/golang/go/blob/master/src/runtime/stack.go#L1142
I'm not sure about it.

Currently the cgo tool makes no attempt to parse any portion of the cgo comment.

Oh, that was my idea, a line before the C function. So, that sounds like a big change.

@aclements
Copy link
Member

That depends, it could be very significant - when it's heavily using cgo and escaping Go objects frequency - with large GC overhead.

It would be valuable to quantify this more, if possible. This is tricky to do at scale, though I think not impossible. For your particular application, are you positive that cgo is the only reason these objects are escaping? We've always assumed that, in practice, objects being passed to cgo are typically going to be on the heap for other reasons anyway. Strong evidence that this isn't true, at least in some range of applications, would help motivate this.

Note also that we might improve escape analysis in the future, so having more information about C calls may have more impact in the future.

In the current implementation, maybe isShrinkStackSafe is protecting a goroutine from shrinking while invoking
into C?

I believe you're correct that this prevents us from shrinking stacks while in C. If it doesn't, that's certainly something we could do.

@rsc
Copy link
Contributor

rsc commented Feb 8, 2023

The annotations in the C comment today begin with #cgo, and we can't easily match it to the "upcoming" C function since we don't parse the C code. Perhaps:

#cgo noescape <function name>

for the annotation? The important part is that the arguments do not escape from the C function back into Go. Unfortunately to put something on the stack in the main Go toolchain we also need to know that there is no call back into Go, because that might grow the stack. So we probably also need

#cgo nocallback <function name>

Implementations that use a segmented stack would be able to make the optimization of keeping values on the stack with only "noescape". The current toolchain would need both for the same optimization.

So maybe we should have both, to allow a C implementation to declare what it does and a Go implementation to make use of what it needs.

Thoughts?

@doujiang24
Copy link
Contributor Author

It would be valuable to quantify this more, if possible. This is tricky to do at scale, though I think not impossible.

Yep, it's possible. Now, the total GC CPU overhead is about 10% of the total CPU usage, in our application(gateway).
We need to quantify the GC overhead of this escaping thing.
I think there are two ways:

  1. the number of GC objects that could be avoided escaping / total temporarily GC objects, could be near 5%, which means we could save 0.5% of the total CPU usage. It's should be significant for us, if our application(gateway) is a large-scale infrastructure, even 0.1% is a big value for us.
  2. testing the CPU usage with a POC optimization implementation, that I haven't implemented yet.

For your particular application, are you positive that cgo is the only reason these objects are escaping

Yes, I can confirm it, here is an example in the Envoy Golang extension:

	var value string
	res := C.envoyGoFilterHttpGetStringValue(r, ValueRouteName, unsafe.Pointer(&value))
        return strings.Clone(value)

Note also that we might improve escape analysis in the future, so having more information about C calls may have more impact in the future.

Cool, sounds great.

@doujiang24
Copy link
Contributor Author

#cgo noescape <function name>

Great, I think it's very good.

Implementations that use a segmented stack would be able to make the optimization of keeping values on the stack with only "noescape".

Sorry, I'm confused about this, the current Go implement is not using a segmented stack, right?

So maybe we should have both, to allow a C implementation to declare what it does and a Go implementation to make use of what it needs.

Sorry, I think I haven't got the meaning to have them both.
In my opinion, if the pointer won't callback to Go, then it's safe to avoid escape.
So, I think one annotation may be enough.

Thanks.

@ianlancetaylor
Copy link
Contributor

Note that I don't think your example code is good programming style. You are passing a pointer to a Go string to C. The documented API permits C code to accept that string as _GoString_*. But there is no documented way for C code to change the values in the string. The only documented API is to fetch the length and and byte pointer from the string (using _GoStringLen and _GoStringPtr). Your code must be using an undocumented and unsupported API.

It would be cleaner to stick to the documented and supported APIs by having the C code return a length and a C pointer, one way or another, and have the Go code call C.GoStringN.

@ianlancetaylor
Copy link
Contributor

The current Go implementation does not use a segmented stack, but it's something we've used in the past and that we've considered using again in the future.

Technically, a C function that is marked nocallback does not guarantee that any pointer passed to that function does not escape. For example, the C function could pass the pointer to a different thread that could call back into Go. That is why we need both nocallback and noescape. Of course, we could say that nocallback implies noescape, but that is a subtle point and it seems at least possible that we would regret that in the future.

@doujiang24
Copy link
Contributor Author

The current Go implementation does not use a segmented stack, but it's something we've used in the past and that we've considered using again in the future.

Okay, understand. Thanks for your clarification.
Then, having these two annotations does make sense.

Note that I don't think your example code is good programming style.

Yep, I knew it is an unsafe usage, using this way, just to make the code simpler.
Also, in the Envoy Go extension, we do disable cgocheck to support this usage.
In some other more complicated cases, we use it for better performance.

i.e. we preallocate memory Go side and pass pointer to C, then fill memory(write) on C side.
https://github.com/envoyproxy/envoy/blob/main/contrib/golang/filters/http/source/go/pkg/http/capi_impl.go#L86-L98
so that we could save memory copy on the C side, since the original memory lifecycle is controlled by Envoy filtermanager(related to downstream request), maybe freed in an Envoy worker thread, we can not assume that memory is still there when we copy them in Go(in another Go thread).

@rsc rsc changed the title proposal: cmd/cgo: add "//go:cgo_unsafe_stack_pointer" to avoid escaping proposal: cmd/cgo: add Feb 22, 2023
@rsc rsc changed the title proposal: cmd/cgo: add proposal: cmd/cgo: add #cgo noescape/nocallback annotations Feb 22, 2023
@rsc
Copy link
Contributor

rsc commented Feb 22, 2023

Updated title. Sounds like #cgo noescape and #cgo nocallback are okay.

Have all concerns about this proposal been addressed?

@doujiang24
Copy link
Contributor Author

Thanks, yep, it's good from my side.

@rsc
Copy link
Contributor

rsc commented Nov 2, 2023

It turns out that for compatibility reasons we cannot release this feature until Go 1.23.
We will land the appropriate preparatory work in Go 1.22 and then enable it in Go 1.23.
Reopening.

@rsc rsc reopened this Nov 2, 2023
@rsc
Copy link
Contributor

rsc commented Nov 2, 2023

Also any rollback of my rollback for Go 1.23 should chase down why using #cgo noescape causes crashes, as in #63739.

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/539235 mentions this issue: cmd/cgo: disable #cgo noescape/nocallback until Go 1.23

@rsc rsc modified the milestones: Go1.22, Go1.23 Nov 2, 2023
gopherbot pushed a commit that referenced this issue Nov 2, 2023
Go 1.21 and earlier do not understand this line, causing
"go mod vendor" of //go:build go1.22-tagged code that
uses this feature to fail.

The solution is to include the go/build change to skip over
the line in Go 1.22 (making "go mod vendor" from Go 1.22 onward
work with this change) and then wait to deploy the cgo change
until Go 1.23, at which point Go 1.21 and earlier will be unsupported.

For #56378.
Fixes #63293.

Change-Id: Ifa08b134eac5a6aa15d67dad0851f00e15e1e58b
Reviewed-on: https://go-review.googlesource.com/c/go/+/539235
LUCI-TryBot-Result: Go LUCI <[email protected]>
Reviewed-by: Cherry Mui <[email protected]>
Reviewed-by: Bryan Mills <[email protected]>
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/548876 mentions this issue: doc: remove empty cgo section

gopherbot pushed a commit that referenced this issue Dec 12, 2023
The only issue in this section, #56378, does not need a release note
for Go 1.22 because its feature was disabled for this release.

For #61422.
Updates #56378.

Change-Id: Ia4e090994cd9ac04e855f8b3a2c6ca0cde4485d2
Reviewed-on: https://go-review.googlesource.com/c/go/+/548876
Auto-Submit: Bryan Mills <[email protected]>
Reviewed-by: Cherry Mui <[email protected]>
LUCI-TryBot-Result: Go LUCI <[email protected]>
ezz-no pushed a commit to ezz-no/go-ezzno that referenced this issue Feb 18, 2024
The only issue in this section, golang#56378, does not need a release note
for Go 1.22 because its feature was disabled for this release.

For golang#61422.
Updates golang#56378.

Change-Id: Ia4e090994cd9ac04e855f8b3a2c6ca0cde4485d2
Reviewed-on: https://go-review.googlesource.com/c/go/+/548876
Auto-Submit: Bryan Mills <[email protected]>
Reviewed-by: Cherry Mui <[email protected]>
LUCI-TryBot-Result: Go LUCI <[email protected]>
aykevl added a commit to tinygo-org/tinygo that referenced this issue Mar 16, 2024
Here is the proposal:
golang/go#56378

They are documented here:
https://pkg.go.dev/cmd/cgo@master#hdr-Optimizing_calls_of_C_code

This would have been very useful to fix
tinygo-org/bluetooth#176 in a nice way. That
bug is now fixed in a different way using a wrapper function, but once
this new noescape pragma gets included in TinyGo we could remove the
workaround and use `#cgo noescape` instead.
doujiang24 added a commit to doujiang24/go that referenced this issue Apr 18, 2024
This reverts commit 607e020.

Reason for revert: Go1.22 is released.

It's aggressive to introdcue #cgo noescape/nocallback in Go1.22, as in golang#63739
And it won't be a problem again while upgrading from Go1.22 to Go1.23.

fix golang#56378

Signed-off-by: doujiang24 <[email protected]>
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/579955 mentions this issue: Revert "cmd/cgo: disable #cgo noescape/nocallback until Go 1.23"

aykevl added a commit to tinygo-org/tinygo that referenced this issue Jun 10, 2024
Here is the proposal:
golang/go#56378

They are documented here:
https://pkg.go.dev/cmd/cgo@master#hdr-Optimizing_calls_of_C_code

This would have been very useful to fix
tinygo-org/bluetooth#176 in a nice way. That
bug is now fixed in a different way using a wrapper function, but once
this new noescape pragma gets included in TinyGo we could remove the
workaround and use `#cgo noescape` instead.
@dominikh
Copy link
Member

dominikh commented Jul 3, 2024

Would #cgo noescape + #cgo nocallback allow passing Go pointers to Go pointers if both are stack allocated, or would that continue to be a violation of the pointer passing rules?

For example, the WebGPU C API has functions like these:

WGPU_EXPORT void wgpuSurfaceConfigure(WGPUSurface surface, WGPUSurfaceConfiguration const * config) WGPU_FUNCTION_ATTRIBUTE;

where WGPUSurfaceConfiguration is

typedef struct WGPUSurfaceConfiguration {
    WGPUChainedStruct const * nextInChain;
    WGPUDevice device;
    WGPUTextureFormat format;
    WGPUTextureUsageFlags usage;
    size_t viewFormatCount;
    WGPUTextureFormat const * viewFormats;
    WGPUCompositeAlphaMode alphaMode;
    uint32_t width;
    uint32_t height;
    WGPUPresentMode presentMode;
} WGPUSurfaceConfiguration WGPU_STRUCTURE_ATTRIBUTE;

In C, all of the pointers would either be on the stack, or read-only globals. nextInChain in particular constructs a singly linked list of vendor-specific extensions. The intended usage is to construct the nested structs and arrays on the stack, and for the called function to process them / copy any data it needs to retain.

@ianlancetaylor
Copy link
Contributor

Would #cgo noescape + #cgo nocallback allow passing Go pointers to Go pointers if both are stack allocated, or would that continue to be a violation of the pointer passing rules?

I think that we could change the rules to permit that if we wanted to. However, it gets really subtle: C code would not be permitted to change the values of any Go pointers stored in Go memory. And of course C code could not retain Go pointers to unpinned Go memory.

doujiang24 added a commit to doujiang24/go that referenced this issue Jul 5, 2024
This reverts commit 607e020.

Reason for revert: Go1.22 is released.

It's aggressive to introdcue #cgo noescape/nocallback in Go1.22, as in golang#63739
And it won't be a problem again while upgrading from Go1.22 to Go1.23.

fix golang#56378

Signed-off-by: doujiang24 <[email protected]>
@gopherbot gopherbot modified the milestones: Go1.23, Go1.24 Aug 13, 2024
aykevl added a commit to tinygo-org/tinygo that referenced this issue Nov 19, 2024
Here is the proposal:
golang/go#56378

They are documented here:
https://pkg.go.dev/cmd/cgo@master#hdr-Optimizing_calls_of_C_code

This would have been very useful to fix
tinygo-org/bluetooth#176 in a nice way. That
bug is now fixed in a different way using a wrapper function, but once
this new noescape pragma gets included in TinyGo we could remove the
workaround and use `#cgo noescape` instead.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Accepted
10 participants