Remove unsafe code for appengine #97
Comments
|
I can take a stab at this. Just wondering, how critical is it to avoid the |
|
@lukechampine - currently looking at all dependencies with |
|
@ineiti well yes, but everything depends on a context. AppEngine do not allow you to use anything from IMO: there is no sense to add |
|
I did some benchmarks |
|
@janisz what exactly do you compare? :) |
diff --git a/bytes.go b/bytes.go
deleted file mode 100644
index 3944bfe..0000000
--- a/bytes.go
+++ /dev/null
@@ -1,14 +0,0 @@
-// +build !appengine
-
-package bigcache
-
-import (
- "reflect"
- "unsafe"
-)
-
-func bytesToString(b []byte) string {
- bytesHeader := (*reflect.SliceHeader)(unsafe.Pointer(&b))
- strHeader := reflect.StringHeader{Data: bytesHeader.Data, Len: bytesHeader.Len}
- return *(*string)(unsafe.Pointer(&strHeader))
-}
diff --git a/bytes_appengine.go b/bytes_appengine.go
index 3892f3b..6718f6b 100644
--- a/bytes_appengine.go
+++ b/bytes_appengine.go
@@ -1,5 +1,3 @@
-// +build appengine
-
package bigcache
func bytesToString(b []byte) string { |
|
So I guess your diff misses the Anyway, looking at the times and memory allocated, it seems that the unsafe Funny thing is that https://github.com/prataprc/goparsec/blob/master/scanner.go#L228 is doing nearly the same. Was there an old version of go that misbehaved when you did a |
package bigcache_test
import (
"bytes"
"reflect"
"testing"
"unsafe"
)
func bytesToString(b []byte) string {
return string(b)
}
func bytesToStringUnsafe(b []byte) string {
bytesHeader := (*reflect.SliceHeader)(unsafe.Pointer(&b))
strHeader := reflect.StringHeader{Data: bytesHeader.Data, Len: bytesHeader.Len}
return *(*string)(unsafe.Pointer(&strHeader))
}
var a = bytes.Repeat([]byte("abcd"), 1024)
var sink = ""
func BenchmarkBytesToString(b *testing.B) {
for i := 0; i < b.N; i++ {
sink = bytesToString(a)
}
}
func BenchmarkBytesToStringUnsafe(b *testing.B) {
for i := 0; i < b.N; i++ {
sink = bytesToStringUnsafe(a)
}
}# go test -bench="BenchmarkBytesToString" -benchmem x_test.go
goos: linux
goarch: amd64
BenchmarkBytesToString-8 1074472 977 ns/op 4096 B/op 1 allocs/op
BenchmarkBytesToStringUnsafe-8 546898618 2.06 ns/op 0 B/op 0 allocs/op
PASS
ok command-line-arguments 3.296s
|
|
^^^ it does make sense, 'cause converting Converting Probably we can get rid of unsafe at all, wdyt @janisz ? |
|
We need more benchmarks, The only usage of And unsafe time is constant across the versions while safe differs from 352ns ± 7% (1.9.7) to 855ns ± 2% (1.12.16) so I prefer to stay with unsafe to reduce the risk of this having impact on our codebase under different go versions. I still do not understand why this have so minimal impact on our benchmarks |
I think this is the right methodology, constant performance is the right approach.
Do share your results here when you find it, I am curious. |
This function should be replaced with a safe versions and protected with a build tag:
See: #96
The text was updated successfully, but these errors were encountered: