Merge pull request #1702 from seebs/bitmapRun

callback logic fixes for intersectionCallback and containerCallback

field.go

@@ -1098,9 +1098,15 @@ func (f *Field) MutexCheck(ctx context.Context, qcx *Qcx, details bool, limit in
 	if f.Type() != FieldTypeMutex {
 		return nil, errors.New("mutex check only valid for mutex fields")
 	}
+
+	// Rather than deferring the unlock, we grab the standard view
+	// from the field's viewMap and unlock immediately. This avoids
+	// holding the rlock for a potentially long time which blocks any
+	// write lock, and pending write locks block other read locks.
 	f.mu.RLock()
-	defer f.mu.RUnlock()
 	standard := f.viewMap[viewStandard]
+	f.mu.RUnlock()
+
 	if standard == nil {
 		// no standard view present means we've never needed to create it,
 		// so it has no bits set, so it has no extra bits set.

roaring/roaring.go

@@ -2997,6 +2997,7 @@ func ArrayCountRange(array []uint16, start, end int32) (n int32) {
 	return n
 }
 
+// BitmapCountRange counts bits set in [start,end).
 func BitmapCountRange(bitmap []uint64, start, end int32) int32 {
 	if roaringParanoia {
 		if start > end {
@@ -3033,15 +3034,16 @@ func BitmapCountRange(bitmap []uint64, start, end int32) int32 {
 }
 
 func callbackBits(w uint64, base uint16, fn func(uint16)) {
-	bit := uint16(0)
 	for w != 0 {
-		trail := bits.TrailingZeros64(w)
-		bit += uint16(trail)
-		w >>= (trail + 1)
-		fn(base + bit)
+		trail := uint16(bits.TrailingZeros64(w))
+		fn(base + trail)
+		base += trail + 1
+		w >>= trail + 1
 	}
 }
 
+// bitmapCallbackRange calls the provided function for every bit set in
+// bitmap in the range [start,end).
 func bitmapCallbackRange(bitmap []uint64, start, end int32, fn func(uint16)) {
 	if roaringParanoia {
 		if start > end {
@@ -3051,11 +3053,30 @@ func bitmapCallbackRange(bitmap []uint64, start, end int32, fn func(uint16)) {
 	i, j := start/64, end/64
 	// Special case when start and end fall in the same word.
 	if i == j {
-		offi, offj := uint(start%64), uint(64-end%64)
-		w := (bitmap[i] >> offi) << (offj + offi)
+		// So, we want to know the offsets. For instance, if start and end
+		// are 65 and 69, we might want i=1, offi=1, j=1, offj=5. Then we
+		// compute masks from offi (masking out 0x1, or (1<<offi)-1), and
+		// offj (masking out everything *but* (1<<offj)-1).
+		//
+		// But we can also use "x >> offi << offi" to trim the lowest offi
+		// bits, and "x << (64-offj) >> (64-offj)" to trim all but the
+		// lowest offj bits.
+		//
+		// We can then simplify slightly further: we use the inverted value
+		// as offj, and compute (w << offi) >> (offi + offj) << offi.
+		//
+		// But wait, you ask. What if offi+offj is too large! Well, then
+		// start and end were in the wrong order. We have 0 <= i <= j < 64.
+		// If x+i > 64, then x > (64-i). Thus, if (64-j)+i > 64, it
+		// follows that (64-j) > (64-i). So they'd have been in the wrong order.
+		// In which case, we correctly yield a value of (0 << offi), or 0,
+		// because nothing is between them.
+		offi, offj := uint(start%64), uint(64-(end%64))
+		w := (bitmap[i] << offj) >> (offi + offj) << offi
 		if w != 0 {
 			callbackBits(w, uint16(i)*64, fn)
 		}
+		return
 	}
 
 	// Count partial starting word.
@@ -4492,7 +4513,7 @@ func intersectionCallbackArrayArray(a, b *Container, fn func(uint16)) {
 	}
 	if (na << 2) < nb {
 		for _, va := range ca {
-			for cb[0] < va {
+			if cb[0] < va {
 				// try to skip ahead a bit faster
 				for len(cb) > 7 && cb[7] < va {
 					cb = cb[8:]
@@ -4530,19 +4551,21 @@ func intersectionCallbackArrayRun(a, b *Container, fn func(uint16)) {
 	na, nb := len(array), len(runs)
 	for i, j := 0, 0; i < na && j < nb; {
 		va, vb := array[i], runs[j]
-		if va < vb.Start {
-			i++
-		} else if va >= vb.Start && va <= vb.Last {
-			i++
-			fn(va)
-		} else if va > vb.Last {
+		if va > vb.Last {
 			j++
+			continue
+		}
+		// If we got here, va is either before or in the current run,
+		// so we're definitely done with this member of the array.
+		i++
+		if va >= vb.Start {
+			fn(va)
 		}
 	}
 }
 
 func intersectionCallbackRunRun(a, b *Container, fn func(uint16)) {
-	statsHit("intersectionCount/RunRun")
+	statsHit("intersectionCallback/RunRun")
 	ra, rb := a.runs(), b.runs()
 	na, nb := len(ra), len(rb)
 	for i, j := 0, 0; i < na && j < nb; {
@@ -4582,14 +4605,14 @@ func intersectionCallbackRunRun(a, b *Container, fn func(uint16)) {
 }
 
 func intersectionCallbackBitmapRun(a, b *Container, fn func(uint16)) {
-	statsHit("intersectionCount/BitmapRun")
+	statsHit("intersectionCallback/BitmapRun")
 	for _, iv := range b.runs() {
 		bitmapCallbackRange(a.bitmap(), int32(iv.Start), int32(iv.Last)+1, fn)
 	}
 }
 
 func intersectionCallbackArrayBitmap(a, b *Container, fn func(uint16)) {
-	statsHit("intersectionCount/ArrayBitmap")
+	statsHit("intersectionCallback/ArrayBitmap")
 	bitmap := b.bitmap()
 	ln := len(bitmap)
 	for _, val := range a.array() {
@@ -4598,14 +4621,14 @@ func intersectionCallbackArrayBitmap(a, b *Container, fn func(uint16)) {
 			break
 		}
 		off := val % 64
-		if (bitmap[i]>>off) & 1 != 0 {
+		if (bitmap[i]>>off)&1 != 0 {
 			fn(val)
 		}
 	}
 }
 
 func intersectionCallbackBitmapBitmap(a, b *Container, fn func(uint16)) {
-	statsHit("intersectionCount/BitmapBitmap")
+	statsHit("intersectionCallback/BitmapBitmap")
 	ab, bb := a.bitmap(), b.bitmap()
 	for i := range ab {
 		w := ab[i] & bb[i]

roaring/roaring_internal_test.go

@@ -4636,3 +4636,103 @@ func TestContainer_unionInPlace_ArrayUnionRun(t *testing.T) {
 		}
 	}
 }
+
+func TestContainerCallback(t *testing.T) {
+	containers, err := InitContainerArchetypes()
+	if err != nil {
+		t.Fatalf("creating containers: %v", err)
+	}
+	got := make([]uint16, 65536)
+	hit := func(u uint16) {
+		got = append(got, u)
+	}
+	var expected []uint16
+	// complain() wraps up some pretty-printing logic for this,
+	// but note also the closure trapping expected/got so we can
+	// just refer to them without passing them in.
+	complain := func(t *testing.T, msg string, args ...interface{}) {
+		l1 := len(expected)
+		l2 := len(got)
+		dotdot1 := ""
+		dotdot2 := ""
+		if l1 > 8 {
+			expected = expected[:8]
+			dotdot1 = "..."
+		}
+		if l2 > 8 {
+			got = got[:8]
+			dotdot2 = "..."
+		}
+		t.Fatalf("%s: expected %d%s, got %d%s", fmt.Sprintf(msg, args...), expected, dotdot1, got, dotdot2)
+	}
+	for t1, ci := range containers {
+		t.Run(ContainerArchetypeNames[t1], func(t *testing.T) {
+			for _, c1 := range ci {
+				got = got[:0]
+				expected = c1.Slice()
+				containerCallback(c1, hit)
+				if len(got) != len(expected) {
+					complain(t, "wrong length (%d vs %d)", len(expected), len(got))
+				}
+				for i := range got {
+					if got[i] != expected[i] {
+						complain(t, "element %d differs: expected %d, got %d", i, expected[i], got[i])
+					}
+				}
+			}
+		})
+	}
+}
+
+func TestIntersectionCallback(t *testing.T) {
+	containers, err := InitContainerArchetypes()
+	if err != nil {
+		t.Fatalf("creating containers: %v", err)
+	}
+	got := make([]uint16, 65536)
+	hit := func(u uint16) {
+		got = append(got, u)
+	}
+	var expected []uint16
+	// complain() wraps up some pretty-printing logic for this,
+	// but note also the closure trapping expected/got so we can
+	// just refer to them without passing them in.
+	complain := func(t *testing.T, msg string, args ...interface{}) {
+		l1 := len(expected)
+		l2 := len(got)
+		dotdot1 := ""
+		dotdot2 := ""
+		if l1 > 8 {
+			expected = expected[:8]
+			dotdot1 = "..."
+		}
+		if l2 > 8 {
+			got = got[:8]
+			dotdot2 = "..."
+		}
+		t.Fatalf("%s: expected %d%s, got %d%s", fmt.Sprintf(msg, args...), expected, dotdot1, got, dotdot2)
+	}
+	for t1, ci := range containers {
+		for t2, cj := range containers {
+			t.Run(fmt.Sprintf("%s-%s", ContainerArchetypeNames[t1], ContainerArchetypeNames[t2]), func(t *testing.T) {
+				for _, c1 := range ci {
+					for _, c2 := range cj {
+						got = got[:0]
+						expectedContainer := intersect(c1, c2)
+						expected = expectedContainer.Slice()
+						intersectionCallback(c1, c2, hit)
+						if len(got) != len(expected) {
+							complain(t, "wrong length (%d vs %d)", len(expected), len(got))
+						}
+						for i := range got {
+							if got[i] != expected[i] {
+								complain(t, "element %d differs: expected %d, got %d", i, expected[i], got[i])
+							}
+						}
+					}
+				}
+
+			})
+		}
+	}
+}

view.go

@@ -300,8 +300,8 @@ func (v *view) Fragment(shard uint64) *fragment {
 
 // allFragments returns a list of all fragments in the view.
 func (v *view) allFragments() []*fragment {
-	v.mu.Lock()
-	defer v.mu.Unlock()
+	v.mu.RLock()
+	defer v.mu.RUnlock()
 
 	other := make([]*fragment, 0, len(v.fragments))
 	for _, fragment := range v.fragments {