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

runtime: cgo check of []byte checks every element uselessly #14387

Closed
ianlancetaylor opened this issue Feb 18, 2016 · 2 comments
Closed

runtime: cgo check of []byte checks every element uselessly #14387

ianlancetaylor opened this issue Feb 18, 2016 · 2 comments

Comments

@ianlancetaylor
Copy link
Contributor

This program

package main

// void f(void* p) {}
import "C"

import (
    "fmt"
    "time"
    "unsafe"
)

func main() {
    b := make([]byte, 1e6)
    t := time.Now()
    for i := 0; i < 1e3; i++ {
        C.f(unsafe.Pointer(&b[0]))
    }
    fmt.Println(time.Since(t))
}

takes 4 seconds on my laptop. Setting GODEBUG=cgocheck=0 it takes 430 microseconds. This is a []byte that should not take any time to check.

I have a simple fix. Opening this issue so that the fix goes into a future 1.6.1 release, if there is one.

@ianlancetaylor ianlancetaylor self-assigned this Feb 18, 2016
@ianlancetaylor ianlancetaylor added this to the Go1.6.1 milestone Feb 18, 2016
@gopherbot
Copy link
Contributor

CL https://golang.org/cl/19621 mentions this issue.

ns-codereview pushed a commit to couchbase/goforestdb that referenced this issue Mar 6, 2016
previous implementation used C.memmove to do heavy lifting
doing this involves passing a pointer into the []byte to C
in Go 1.6 this has an unintended performance penalty as it
unnecessarily checks the entire slice

this is a bug in Go 1.6 documented here:
golang/go#14387

however, it seems as though this call can be avoided entirely,
by creating an artificial slice header which points to the
newly malloc'd C buffer, we use Go's builtin copy() method
in addition to avoid this bug, it also avoids a cgo call
which we know can add up

Change-Id: I910e5705e29e6c86343fd0e6315c8430f56b07ef
Reviewed-on: http://review.couchbase.org/60991
Reviewed-by: Steve Yen <steve.yen@gmail.com>
Tested-by: Marty Schoch <marty.schoch@gmail.com>
bmatsuo added a commit to bmatsuo/lmdb-go that referenced this issue Mar 31, 2016
It was discovered in #64 that void* arguments cause allocations in cgo
and extra time checking arguments as opposed to char* arguments.

The issue golang/go#14387 claims that it will fix this problem but I am
going to keep this patch around for testing against tip and later
merging if necessary.
bmatsuo added a commit to bmatsuo/lmdb-go that referenced this issue Mar 31, 2016
It was discovered in #63 that void* arguments cause allocations in cgo
and extra time checking arguments as opposed to char* arguments.

The issue golang/go#14387 claims that it will fix this problem but I am
going to keep this patch around for testing against tip and later
merging if necessary.
@adg adg added the Release-OK label Apr 7, 2016
@adg adg modified the milestones: Go1.6.1, Go1.6.2 Apr 7, 2016
@gopherbot
Copy link
Contributor

CL https://golang.org/cl/22036 mentions this issue.

gopherbot pushed a commit that referenced this issue Apr 14, 2016
Fixes #14387.

Change-Id: Icc98be80f549c5e1f55c5e693bfea97b456a6c41
Reviewed-on: https://go-review.googlesource.com/19621
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Reviewed-on: https://go-review.googlesource.com/22036
Reviewed-by: Ian Lance Taylor <iant@golang.org>
@golang golang locked and limited conversation to collaborators Apr 19, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants