From 3ae12391c79d0a3803876ab1ecfdb4e680ff0146 Mon Sep 17 00:00:00 2001 From: Seebs Date: Tue, 14 Sep 2021 15:48:19 -0500 Subject: [PATCH 1/5] callback logic fixes for intersectionCallback and containerCallback The inner loop of intersectionCallbackArrayArray's "fast" case has for len(ca) > 0 && ca[0] < va { } so we do not leave that loop unless len(ca) is 0, or ca[0] >= va. We then return from the whole function if len(ca) is 0, so the only way we finish one iteration of the outer for loop is if ca[0] >= va. Thus, this can be an `if` rather than a `for`. We also fix the logic for ArrayRun to make it require fewer tests and be clearer about why the tests work and clearer about always making progress. And, finally, the bitmap/range callback logic, and the underlying "callback per bit in word" logic, were both badly broken. In particular, if a range started and ended in the same word, it would hit the values in that word twice, once with them incorrectly shifted, but then it would further garble any offsets past the first in a word. Eww. --- roaring/roaring.go | 51 +++++++++++++++++++++++--------- roaring/roaring_internal_test.go | 12 ++++++++ 2 files changed, 49 insertions(+), 14 deletions(-) diff --git a/roaring/roaring.go b/roaring/roaring.go index 9567b37fb..26504f97b 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,13 +4551,15 @@ 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) } } } diff --git a/roaring/roaring_internal_test.go b/roaring/roaring_internal_test.go index ce6f61a7d..1aaa5fb88 100644 --- a/roaring/roaring_internal_test.go +++ b/roaring/roaring_internal_test.go @@ -4636,3 +4636,15 @@ func TestContainer_unionInPlace_ArrayUnionRun(t *testing.T) { } } } + +func TestIntersectionCallback(t *testing.T) { + var hits []uint16 + cb := func(u uint16) { + hits = append(hits, u) + } + bm := []uint64{0, 5, 0} + bitmapCallbackRange(bm, 64, 69, cb) + if len(hits) != 2 || hits[0] != 64 || hits[1] != 66 { + t.Fatalf("expected 64, 66, got %d", hits) + } +} From e0dfde99340fd551cf6373c6c4925dab2c5ef6ed Mon Sep 17 00:00:00 2001 From: Seebs Date: Wed, 15 Sep 2021 12:36:39 -0500 Subject: [PATCH 2/5] appease gofmt --- roaring/roaring.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/roaring/roaring.go b/roaring/roaring.go index 26504f97b..cf32e7689 100644 --- a/roaring/roaring.go +++ b/roaring/roaring.go @@ -4621,7 +4621,7 @@ 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) } } From 12244dcbed689644c670de157305c17336b04051 Mon Sep 17 00:00:00 2001 From: Seebs Date: Wed, 15 Sep 2021 13:40:07 -0500 Subject: [PATCH 3/5] record stats for intersectionCallback under the right name --- roaring/roaring.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/roaring/roaring.go b/roaring/roaring.go index cf32e7689..e35487a3e 100644 --- a/roaring/roaring.go +++ b/roaring/roaring.go @@ -4565,7 +4565,7 @@ func intersectionCallbackArrayRun(a, b *Container, fn func(uint16)) { } 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; { @@ -4605,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() { @@ -4628,7 +4628,7 @@ func intersectionCallbackArrayBitmap(a, b *Container, fn func(uint16)) { } 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] From e7e3331fb4be6c70a860f181e3632b9d9f3e754f Mon Sep 17 00:00:00 2001 From: Seebs Date: Wed, 15 Sep 2021 12:36:46 -0500 Subject: [PATCH 4/5] test intersectionCallback more carefully This takes our reasonably broad selection of predefined container types and tries intersectionCallback on each pair of them, comparing results against the results of plain old intersect(). We've had several intersectionCallback fixes recently; every one of them produces test failures here if reverted or broken, so I have at least some confidence in this coverage. Similarly, test everything on containerCallback, verifying that we get the same set of values called back that we get from Slice(). Both of these were verified with -coverprofile to actually be hitting all the lines of code that aren't insane edge case checks like "what if a run is in the wrong order". --- roaring/roaring_internal_test.go | 104 ++++++++++++++++++++++++++++--- 1 file changed, 96 insertions(+), 8 deletions(-) diff --git a/roaring/roaring_internal_test.go b/roaring/roaring_internal_test.go index 1aaa5fb88..c23adc6f4 100644 --- a/roaring/roaring_internal_test.go +++ b/roaring/roaring_internal_test.go @@ -4637,14 +4637,102 @@ func TestContainer_unionInPlace_ArrayUnionRun(t *testing.T) { } } -func TestIntersectionCallback(t *testing.T) { - var hits []uint16 - cb := func(u uint16) { - hits = append(hits, u) +func TestContainerCallback(t *testing.T) { + containers, err := InitContainerArchetypes() + if err != nil { + t.Fatalf("creating containers: %v", err) } - bm := []uint64{0, 5, 0} - bitmapCallbackRange(bm, 64, 69, cb) - if len(hits) != 2 || hits[0] != 64 || hits[1] != 66 { - t.Fatalf("expected 64, 66, got %d", hits) + 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]) + } + } + } + } + + }) + } } } From 3e222d87711275c132f5ffd5618c2e5e7641b10a Mon Sep 17 00:00:00 2001 From: Matthew Jaffee Date: Tue, 14 Sep 2021 17:00:00 -0500 Subject: [PATCH 5/5] tweak to locking which should avoid stall/deadlock w/ mutex check The view.go change is straightforward and fairly obviously more correct. The field.go change avoids holding the field read lock for the duration of the mutex check request. The thinking was that while the read lock was held something else was attempting to get a write lock, which blocked all other read locks and something was getting into a loop. Seebs might have a more detailed explanation, but that's as far as my understanding goes at the moment. I believe this change is safe though as we don't read/modify any field level data structures after grabbing the standard view. --- field.go | 8 +++++++- view.go | 4 ++-- 2 files changed, 9 insertions(+), 3 deletions(-) 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/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 {