diff --git a/field.go b/field.go index 793371272..a6c4b5e75 100644 --- a/field.go +++ b/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. diff --git a/roaring/roaring.go b/roaring/roaring.go index 9567b37fb..e35487a3e 100644 --- a/roaring/roaring.go +++ b/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 << 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] diff --git a/roaring/roaring_internal_test.go b/roaring/roaring_internal_test.go index ce6f61a7d..c23adc6f4 100644 --- a/roaring/roaring_internal_test.go +++ b/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]) + } + } + } + } + + }) + } + } +} diff --git a/view.go b/view.go index bb0eb3a91..ac94f64d8 100644 --- a/view.go +++ b/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 {