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

Open
doujiang24 opened this issue Oct 22, 2022 · 33 comments · May be fixed by #66879
Open

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

doujiang24 opened this issue Oct 22, 2022 · 33 comments · May be fixed by #66879

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 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

@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.

@dr2chase
Copy link
Contributor

dr2chase commented Mar 1, 2023

Skimming this, I didn't see mention/proposal of the possibility of enforcing this dynamically, and I think we can do that for #cgo nocallback. If that sets a bit in the gororutine (we need to note that the stack cannot be shrunk, right?) we could also check that bit on any callback into Go, and panic if it is set.

@rsc
Copy link
Contributor

rsc commented Mar 1, 2023

Yes, we should check nocallback dynamically - very easy.

@rsc
Copy link
Contributor

rsc commented Mar 1, 2023

Based on the discussion above, this proposal seems like a likely accept.
— rsc for the proposal review group

@gopherbot
Copy link

Change https://go.dev/cl/522937 mentions this issue: doc/go1.22: mention new #cgo directives

@dmitshur dmitshur modified the milestones: Backlog, Go1.22 Aug 26, 2023
gopherbot pushed a commit that referenced this issue Aug 26, 2023
For #56378

Change-Id: I0c9c662c6d765cad6f7bf17fdd648db8d73e429b
Reviewed-on: https://go-review.googlesource.com/c/go/+/522937
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
TryBot-Bypass: Ian Lance Taylor <iant@google.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
Auto-Submit: Ian Lance Taylor <iant@google.com>
@gopherbot
Copy link

Change https://go.dev/cl/522939 mentions this issue: cmd/cgo/internal/test: benchmark for #cgo noescape directive

gopherbot pushed a commit that referenced this issue Aug 28, 2023
case: passing a single Go string object to C function.
result: 87 ns vs 61 ns.

BenchmarkCgoCall/string-pointer-escape
BenchmarkCgoCall/string-pointer-escape-12        67731663   87.02 ns/op
BenchmarkCgoCall/string-pointer-noescape
BenchmarkCgoCall/string-pointer-noescape-12    99424776   61.30 ns/op

For #56378

Change-Id: Iff5c69d8deedfa248f5d7399e1921a5cb0dc8b16
GitHub-Last-Rev: fc67d5a
GitHub-Pull-Request: #62297
Reviewed-on: https://go-review.googlesource.com/c/go/+/522939
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@google.com>
Auto-Submit: Ian Lance Taylor <iant@google.com>
Run-TryBot: Ian Lance Taylor <iant@golang.org>
Run-TryBot: Ian Lance Taylor <iant@google.com>
Reviewed-by: Bryan Mills <bcmills@google.com>
aykevl added a commit to tinygo-org/tinygo that referenced this issue Sep 2, 2023
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.
@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

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 <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Cherry Mui <cherryyz@google.com>
Reviewed-by: Bryan Mills <bcmills@google.com>
@gopherbot
Copy link

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 <bcmills@google.com>
Reviewed-by: Cherry Mui <cherryyz@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
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 <bcmills@google.com>
Reviewed-by: Cherry Mui <cherryyz@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
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 <doujiang24@gmail.com>
@gopherbot
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Accepted
9 participants