Browse Source

codec: ensure that GC doesn't reclaim the input for a (string|bytes)View

This is especially pertinent in unsafe mode.

Also, clarify documentation on when Intern flag should be used during decode.
Ugorji Nwoke 8 years ago
parent
commit
5dfac81833
3 changed files with 45 additions and 16 deletions
  1. 19 10
      codec/decode.go
  2. 16 0
      codec/helper_not_unsafe.go
  3. 10 6
      codec/helper_unsafe.go

+ 19 - 10
codec/decode.go

@@ -161,7 +161,9 @@ type DecodeOptions struct {
 	// look them up from a map (than to allocate them afresh).
 	//
 	// Note: Handles will be smart when using the intern functionality.
-	// So everything will not be interned.
+	// Every string should not be interned.
+	// An excellent use-case for interning is struct field names,
+	// or map keys where key type is string.
 	InternString bool
 
 	// PreferArrayOverSlice controls whether to decode to an array or a slice.
@@ -740,7 +742,8 @@ func (f *decFnInfo) kStruct(rv reflect.Value) {
 				if cr != nil {
 					cr.sendContainerState(containerMapKey)
 				}
-				rvkencname := stringView(dd.DecodeBytes(f.d.b[:], true, true))
+				rvkencnameB := dd.DecodeBytes(f.d.b[:], true, true)
+				rvkencname := stringView(rvkencnameB)
 				// rvksi := ti.getForEncName(rvkencname)
 				if cr != nil {
 					cr.sendContainerState(containerMapValue)
@@ -755,6 +758,7 @@ func (f *decFnInfo) kStruct(rv reflect.Value) {
 				} else {
 					d.structFieldNotFound(-1, rvkencname)
 				}
+				keepAlive4StringView(rvkencnameB) // maintain ref 4 stringView
 			}
 		} else {
 			for j := 0; !dd.CheckBreak(); j++ {
@@ -762,7 +766,8 @@ func (f *decFnInfo) kStruct(rv reflect.Value) {
 				if cr != nil {
 					cr.sendContainerState(containerMapKey)
 				}
-				rvkencname := stringView(dd.DecodeBytes(f.d.b[:], true, true))
+				rvkencnameB := dd.DecodeBytes(f.d.b[:], true, true)
+				rvkencname := stringView(rvkencnameB)
 				// rvksi := ti.getForEncName(rvkencname)
 				if cr != nil {
 					cr.sendContainerState(containerMapValue)
@@ -777,6 +782,7 @@ func (f *decFnInfo) kStruct(rv reflect.Value) {
 				} else {
 					d.structFieldNotFound(-1, rvkencname)
 				}
+				keepAlive4StringView(rvkencnameB) // maintain ref 4 stringView
 			}
 		}
 		if cr != nil {
@@ -1873,11 +1879,14 @@ func (d *Decoder) errorf(format string, params ...interface{}) {
 	panic(err)
 }
 
+// Possibly get an interned version of a string
+//
+// This should mostly be used for map keys, where the key type is string
 func (d *Decoder) string(v []byte) (s string) {
 	if d.is != nil {
-		s, ok := d.is[string(v)] // no allocation here.
+		s, ok := d.is[string(v)] // no allocation here, per go implementation
 		if !ok {
-			s = string(v)
+			s = string(v) // new allocation here
 			d.is[s] = s
 		}
 		return s
@@ -1885,11 +1894,11 @@ func (d *Decoder) string(v []byte) (s string) {
 	return string(v) // don't return stringView, as we need a real string here.
 }
 
-func (d *Decoder) intern(s string) {
-	if d.is != nil {
-		d.is[s] = s
-	}
-}
+// func (d *Decoder) intern(s string) {
+// 	if d.is != nil {
+// 		d.is[s] = s
+// 	}
+// }
 
 // nextValueBytes returns the next value in the stream as a set of bytes.
 func (d *Decoder) nextValueBytes() []byte {

+ 16 - 0
codec/helper_not_unsafe.go

@@ -8,6 +8,9 @@ package codec
 // stringView returns a view of the []byte as a string.
 // In unsafe mode, it doesn't incur allocation and copying caused by conversion.
 // In regular safe mode, it is an allocation and copy.
+//
+// Usage: Always maintain a reference to v while result of this call is in use,
+//        and call keepAlive4BytesView(v) at point where done with view.
 func stringView(v []byte) string {
 	return string(v)
 }
@@ -15,6 +18,19 @@ func stringView(v []byte) string {
 // bytesView returns a view of the string as a []byte.
 // In unsafe mode, it doesn't incur allocation and copying caused by conversion.
 // In regular safe mode, it is an allocation and copy.
+//
+// Usage: Always maintain a reference to v while result of this call is in use,
+//        and call keepAlive4BytesView(v) at point where done with view.
 func bytesView(v string) []byte {
 	return []byte(v)
 }
+
+// keepAlive4BytesView maintains a reference to the input parameter for bytesView.
+//
+// Usage: call this at point where done with the bytes view.
+func keepAlive4BytesView(v string) {}
+
+// keepAlive4BytesView maintains a reference to the input parameter for stringView.
+//
+// Usage: call this at point where done with the string view.
+func keepAlive4StringView(v []byte) {}

+ 10 - 6
codec/helper_unsafe.go

@@ -6,10 +6,12 @@
 package codec
 
 import (
+	"runtime"
 	"unsafe"
 )
 
 // This file has unsafe variants of some helper methods.
+// NOTE: See helper_not_unsafe.go for the usage information.
 
 type unsafeString struct {
 	Data uintptr
@@ -22,9 +24,6 @@ type unsafeSlice struct {
 	Cap  int
 }
 
-// stringView returns a view of the []byte as a string.
-// In unsafe mode, it doesn't incur allocation and copying caused by conversion.
-// In regular safe mode, it is an allocation and copy.
 func stringView(v []byte) string {
 	if len(v) == 0 {
 		return ""
@@ -35,9 +34,6 @@ func stringView(v []byte) string {
 	return *(*string)(unsafe.Pointer(&sx))
 }
 
-// bytesView returns a view of the string as a []byte.
-// In unsafe mode, it doesn't incur allocation and copying caused by conversion.
-// In regular safe mode, it is an allocation and copy.
 func bytesView(v string) []byte {
 	if len(v) == 0 {
 		return zeroByteSlice
@@ -47,3 +43,11 @@ func bytesView(v string) []byte {
 	bx := unsafeSlice{sx.Data, sx.Len, sx.Len}
 	return *(*[]byte)(unsafe.Pointer(&bx))
 }
+
+func keepAlive4BytesView(v string) {
+	runtime.KeepAlive(v)
+}
+
+func keepAlive4StringView(v []byte) {
+	runtime.KeepAlive(v)
+}