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: unsafe.Pointer containing bytes.Buffer slice data triggers argument check panic -- workaround causes performance degredation #14704

Closed
bmatsuo opened this issue Mar 8, 2016 · 10 comments

Comments

@bmatsuo
Copy link

commented Mar 8, 2016

  1. What version of Go are you using (go version)? go1.6
  2. What operating system and processor architecture are you using (go env)? linux/amd64
  3. What did you do? http://play.golang.org/p/1MigDGnopS (unfortunately this cannot execute because of cgo)
  4. What did you expect to see? One line printed to standard output containing text "test" (edit: as is output by this very similar, working example http://play.golang.org/p/eX1pSIjmo4)
  5. What did you see instead?
panic: runtime error: cgo argument has Go pointer to Go pointer

goroutine 1 [running]:
panic(0x474d20, 0xc82000a210)
    /usr/lib/go/src/runtime/panic.go:464 +0x3e6
main._cgoCheckPointer0(0x4697e0, 0xc820054094, 0x0, 0x0, 0x0, 0x0)
    github.com/bmatsuo/lmdb-go/tmp/_obj/_cgo_gotypes.go:42 +0x4d
main.main()
    /home/b/src/github.com/bmatsuo/lmdb-go/tmp/cgopanic.go:24 +0x153

I'm creating this issue to track a problem I brought up in #14210 (comment) but for which the cause/fix was different. I took some time before filing the issue to collect more information about the problem and the fix for my own understanding. My apologies for the delay. But it didn't look like a duplicate has been filed since.

I was also told to replace my code that looked like the above example with code which takes the address of a []byte at the actual cgo call site to help the tool figure out which memory region I was referring to. I have done so, and it works, but I have found the result to be significantly slower that before I made this change (both benchmarks takes from a stock go1.6 build -- no flags).

The changes I made to work around the above panic in my actual project can be found here: bmatsuo/lmdb-go@40ebaca

And the performance comparison against master can be seen in my pull request: bmatsuo/lmdb-go#57 (comment)

My benchmarks, unfortunately, have more noise than I would like but there is a definite pattern that shows this change having a real impact on call times (especially for some functions which should execute be extremely quickly). The relevant benchmarks show a 7-43% performance penalty from this change (100ns+ per affected C call).

@ianlancetaylor ianlancetaylor changed the title cgo: unsafe.Pointer containing bytes.Buffer slice data triggers argument check panic -- workaround causes performance degredation cmd/cgo: unsafe.Pointer containing bytes.Buffer slice data triggers argument check panic -- workaround causes performance degredation Mar 8, 2016

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Mar 8, 2016

Can you clarify what you are measuring when you measure a slowdown? It seems to me that you have code that fails the cgo check, and you are replacing it with code that passes the cgo check. The code that fails the cgo check won't run, so what are you timing?

@ianlancetaylor ianlancetaylor added this to the Go1.7 milestone Mar 8, 2016

@bmatsuo

This comment has been minimized.

Copy link
Author

commented Mar 8, 2016

@ianlancetaylor Forgive me for not explaining that clearly. The only time I have seen the cgo check fail is when it is passed a short []byte retrieved from the Bytes() method on bytes.Buffer, such that the slice address points into the array inside a bytes.Buffer object. Aside from this and other situations with similarly designed structs the code in my project, lmdb-go, does not trigger the cgo argument check panic (I thought I had already changed everything to comply!).

You should be able to see this. Changing the code I provided so that b := []byte("test"), or simply writing enough into the buf object to overflow the internal array.

That is what I am benchmarking in my lmdb-go project, byte slices that are unaffected by this issue. And after compensating for this corner case I see processing in the normal case degrading farther than I would like it to.

As I eluded to, I don't think the behavior here is captured well by the cgo change proposal, or any other documentation that I have seen about the restrictions (like the cmd/cgo docs). I was under the impression that the code example from the issue description was passing the address of a struct field (i.e. the byte array field inside bytes.Buffer), in which case the documentation states

When passing a pointer to a field in a struct, the Go memory in question is the memory occupied by the field, not the entire struct.

I don't think the documentation is very clear about the need to perform the indirection at the cgo call site (as was suggested in #14210 and demonstrated at http://play.golang.org/p/eX1pSIjmo4). It never says explicitly says my code should work but it doesn't say that it shouldn't work either.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Mar 8, 2016

Thanks for the additional details.

The documentation doesn't spell out the need to perform the indirection at the cgo call site because there is no intent that that be a permanent requirement. The intent is what you quote from the docs: the memory in question is the memory occupied by the field. The question is how to implement that.

@ianlancetaylor ianlancetaylor self-assigned this Mar 8, 2016

@bmatsuo

This comment has been minimized.

Copy link
Author

commented Mar 8, 2016

Thanks for the explanation. I have probably read you say as much before. But I now finally understand it in this context. So thank you for your patience as well.

I will try a little more to improve my own change, and to reduce the performance penalty I am seeing. It's not clear to me how escape analysis works with cgo. Though it seems to me that with the new restrictions pointers passed through cgo would be considered non-escaping? (edit: obviously unless they could be determined to escape for other reasons)

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Mar 8, 2016

I also am not sure what is escaping here. I think you can run go build -gcflags=-m to see escape analysis explanations.

@bmatsuo

This comment has been minimized.

Copy link
Author

commented Mar 8, 2016

Ah yes. I had forgotten about that flag. It certainly prints out a lot of stuff. I will poke around, see what I can do with it, and get back soon.

Previously I did try manually inlining the function which takes the []byte address. It did not seem to provide a visible performance improvement. But I will look again with this flag enabled and see if I did something dumb. Thanks.

@bmatsuo

This comment has been minimized.

Copy link
Author

commented Mar 12, 2016

I seem to have gotten the relevant benchmarks for my project slightly closer to the original version. But there is still a sizable gulf between performance of the original and fixed code on normal-case input, which do not trigger any panics.

In regards to escaping pointers, it seems like cgo may be assuming that all pointers passed to cgo escape. This is understandable because it cannot be verified but it does seem like that could be relaxed if cgo is to assume that people comply with the restrictions it states (though cannot enforce). Following is a simple example and the output of go build -gcflags=-m which shows a simple []byte escaping in a simple cgo call.

http://play.golang.org/p/gG3o96a4rD

# github.com/bmatsuo/lmdb-go/tmp/cgosimple
/tmp/go-build670788717/github.com/bmatsuo/lmdb-go/tmp/cgosimple/_obj/_cgo_gotypes.go:14: can inline _Cgo_ptr
/tmp/go-build670788717/github.com/bmatsuo/lmdb-go/tmp/cgosimple/_obj/_cgo_gotypes.go:14: leaking param: ptr to result ~r1 level=0
/tmp/go-build670788717/github.com/bmatsuo/lmdb-go/tmp/cgosimple/_obj/_cgo_gotypes.go:27: _cgo_runtime_cgocall assuming arg#2 is unsafe uintptr
/tmp/go-build670788717/github.com/bmatsuo/lmdb-go/tmp/cgosimple/_obj/_cgo_gotypes.go:30: _cgo_runtime_cmalloc assuming arg#1 is unsafe uintptr
/tmp/go-build670788717/github.com/bmatsuo/lmdb-go/tmp/cgosimple/_obj/_cgo_gotypes.go:33: _cgo_runtime_cgocallback assuming arg#3 is unsafe uintptr
/tmp/go-build670788717/github.com/bmatsuo/lmdb-go/tmp/cgosimple/_obj/_cgo_gotypes.go:41: leaking param: p
/tmp/go-build670788717/github.com/bmatsuo/lmdb-go/tmp/cgosimple/_obj/_cgo_gotypes.go:41: leaking param: args
/tmp/go-build670788717/github.com/bmatsuo/lmdb-go/tmp/cgosimple/_obj/_cgo_gotypes.go:53: p0 escapes to heap
/tmp/go-build670788717/github.com/bmatsuo/lmdb-go/tmp/cgosimple/_obj/_cgo_gotypes.go:50: leaking param: p0
/tmp/go-build670788717/github.com/bmatsuo/lmdb-go/tmp/cgosimple/_obj/_cgo_gotypes.go:54: p1 escapes to heap
/tmp/go-build670788717/github.com/bmatsuo/lmdb-go/tmp/cgosimple/_obj/_cgo_gotypes.go:51: _Cfunc_my_func &p0 does not escape
./cgosimple.go:18: unsafe.Pointer(&b[0]) escapes to heap
./cgosimple.go:18: &b[0] escapes to heap
./cgosimple.go:17: ([]byte)("test") escapes to heap
./cgosimple.go:18: ... argument escapes to heap
./cgosimple.go:18: b escapes to heap
./cgosimple.go:17: ([]byte)("test") escapes to heap
@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Mar 14, 2016

I think it's best to keep issues focused on a single problem. The problem of pointers escaping when passed to cgo is a different problem. See #10303.

@rsc

This comment has been minimized.

Copy link
Contributor

commented May 17, 2016

It is unclear to me what this issue is about. Can someone summarize what remains?

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented May 17, 2016

I think that any fix for this is the same as #14210, so closing as a duplicate.

@golang golang locked and limited conversation to collaborators May 17, 2017

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
4 participants
You can’t perform that action at this time.