Browse Source

codec: remove double-indirection and fix race in Copy-On-Write during TypeInfo lookup

Previously, we stored a pointer to a slice as an atomic.Value.
This meant that we were effectively storing a pointer to a pointer to an array.
Now, we just store the pointer to the array and the length separately,
and "compose" the slice using unsafe (in unsafe mode).

This gave a slight perforamce increase.

Also, we now create a new slice whenever we need to update the TypeInfo array.
Previously, we just updated the backing array and returned a new slice object.
But since the same backing array was used, it introduced a race condition.

Fixes #227
Ugorji Nwoke 8 years ago
parent
commit
9831f2c3ac
3 changed files with 31 additions and 19 deletions
  1. 12 8
      codec/helper.go
  2. 3 3
      codec/helper_not_unsafe.go
  3. 16 8
      codec/helper_unsafe.go

+ 12 - 8
codec/helper.go

@@ -132,7 +132,7 @@ const (
 	// should use "runtime/internal/sys".CacheLineSize, but that is not exposed.
 	cacheLineSize = 64
 
-	wordSizeBits = strconv.IntSize
+	wordSizeBits = 32 << (^uint(0) >> 63) // strconv.IntSize
 	wordSize     = wordSizeBits / 8
 
 	maxLevelsEmbedding = 15 // use this, so structFieldInfo fits into 8 bytes
@@ -440,10 +440,13 @@ type BasicHandle struct {
 	RPCOptions
 
 	// ---- cache line
+
 	DecodeOptions
 
 	// ---- cache line
+
 	EncodeOptions
+
 	// noBuiltInTypeChecker
 }
 
@@ -1148,7 +1151,7 @@ func (x *TypeInfos) get(rtid uintptr, rt reflect.Type) (pti *typeInfo) {
 	sp := x.infos.load()
 	var idx int
 	if sp != nil {
-		idx, pti = x.find(*sp, rtid)
+		idx, pti = x.find(sp, rtid)
 		if pti != nil {
 			return
 		}
@@ -1222,16 +1225,17 @@ func (x *TypeInfos) get(rtid uintptr, rt reflect.Type) (pti *typeInfo) {
 	sp = x.infos.load()
 	if sp == nil {
 		pti = &ti
-		vs := append(make([]rtid2ti, 0, 16), rtid2ti{rtid, pti})
-		x.infos.store(&vs)
+		vs := []rtid2ti{{rtid, pti}}
+		x.infos.store(vs)
 	} else {
-		idx, pti = x.find(*sp, rtid)
+		idx, pti = x.find(sp, rtid)
 		if pti == nil {
 			pti = &ti
-			vs := append(*sp, rtid2ti{})
-			copy(vs[idx+1:], vs[idx:])
+			vs := make([]rtid2ti, len(sp)+1)
+			copy(vs, sp[:idx])
+			copy(vs[idx+1:], sp[idx:])
 			vs[idx] = rtid2ti{rtid, pti}
-			x.infos.store(&vs)
+			x.infos.store(vs)
 		}
 	}
 	x.mu.Unlock()

+ 3 - 3
codec/helper_not_unsafe.go

@@ -98,15 +98,15 @@ type atomicTypeInfoSlice struct { // expected to be 2 words
 	v atomic.Value
 }
 
-func (x *atomicTypeInfoSlice) load() *[]rtid2ti {
+func (x *atomicTypeInfoSlice) load() []rtid2ti {
 	i := x.v.Load()
 	if i == nil {
 		return nil
 	}
-	return i.(*[]rtid2ti)
+	return i.([]rtid2ti)
 }
 
-func (x *atomicTypeInfoSlice) store(p *[]rtid2ti) {
+func (x *atomicTypeInfoSlice) store(p []rtid2ti) {
 	x.v.Store(p)
 }
 

+ 16 - 8
codec/helper_unsafe.go

@@ -23,12 +23,12 @@ const safeMode = false
 const unsafeFlagIndir = 1 << 7 // keep in sync with GO_ROOT/src/reflect/value.go
 
 type unsafeString struct {
-	Data uintptr
+	Data unsafe.Pointer
 	Len  int
 }
 
 type unsafeSlice struct {
-	Data uintptr
+	Data unsafe.Pointer
 	Len  int
 	Cap  int
 }
@@ -176,16 +176,24 @@ func isEmptyValue(v reflect.Value, tinfos *TypeInfos, deref, checkStruct bool) b
 // --------------------------
 
 type atomicTypeInfoSlice struct { // expected to be 2 words
-	v unsafe.Pointer
-	_ [8]byte // padding
+	v unsafe.Pointer // data array - Pointer (not uintptr) to maintain GC reference
+	l int64          // length of the data array
 }
 
-func (x *atomicTypeInfoSlice) load() *[]rtid2ti {
-	return (*[]rtid2ti)(atomic.LoadPointer(&x.v))
+func (x *atomicTypeInfoSlice) load() []rtid2ti {
+	l := int(atomic.LoadInt64(&x.l))
+	if l == 0 {
+		return nil
+	}
+	return *(*[]rtid2ti)(unsafe.Pointer(&unsafeSlice{Data: atomic.LoadPointer(&x.v), Len: l, Cap: l}))
+	// return (*[]rtid2ti)(atomic.LoadPointer(&x.v))
 }
 
-func (x *atomicTypeInfoSlice) store(p *[]rtid2ti) {
-	atomic.StorePointer(&x.v, unsafe.Pointer(p))
+func (x *atomicTypeInfoSlice) store(p []rtid2ti) {
+	s := (*unsafeSlice)(unsafe.Pointer(&p))
+	atomic.StorePointer(&x.v, s.Data)
+	atomic.StoreInt64(&x.l, int64(s.Len))
+	// atomic.StorePointer(&x.v, unsafe.Pointer(p))
 }
 
 // --------------------------