From 52aa3e2e23338ca184970190c0341805f9acebe8 Mon Sep 17 00:00:00 2001 From: Seebs Date: Fri, 5 Jun 2020 13:38:33 -0500 Subject: [PATCH] Improve container/bitmap comparison logic for testing We have a "deadcode" bitmapsEqual which is actually used in testing but probably shouldn't be, and we don't have a good container equality test. Problem is, equality tests are sort of slow in the things-are-equal case, which is the most common case, so we've got some moderately-specialized code here; specifically, special comparison code that takes advantage of knowing that if two containers have the same number of bits, you only have to check whether all the bits from one are present in the other, because that can't be true for differing containers with the same number of bits. This reduces the runtime for the ContainerCombinations case from about 24 seconds to a bit under 2 on my laptop, or from around 10 minutes to about 37 seconds with the race detector on. Also simplify the InPlaceWrapper functions not to invoke bitmaps, because it's not really necessary. --- roaring/roaring.go | 154 +++++++++++++++++-------------- roaring/roaring_helpers_test.go | 89 ++++++++++-------- roaring/roaring_internal_test.go | 103 ++++++++------------- roaring/roaring_test.go | 12 --- 4 files changed, 169 insertions(+), 189 deletions(-) diff --git a/roaring/roaring.go b/roaring/roaring.go index 9a636a519..db63b0edd 100644 --- a/roaring/roaring.go +++ b/roaring/roaring.go @@ -4552,50 +4552,95 @@ func (c *Container) bitmapZeroRange(i, j uint64) { c.setN(n) } -// equals reports whether two containers are equal. -func (c *Container) equals(c2 *Container) bool { - if c == nil || c2 == nil { - if c != c2 { - return false +func typePair(ct1, ct2 byte) int { + return int((ct1 << 4) | ct2) +} + +// compareArrayBitmap actually only verifies that everything in the array +// is in the bitmap. It's used only after comparing the N for the containers, +// so if there's anything in the bitmap that's not in the array, either there's +// something in the array that's not in the bitmap, or we didn't get here. +func compareArrayBitmap(a []uint16, b []uint64) error { + for _, v := range a { + w, bit := b[v>>6], v&63 + if w>>bit&1 == 0 { + return fmt.Errorf("value %d missing", v) } } - if c.Mapped() != c2.Mapped() || c.typ() != c2.typ() || c.N() != c2.N() { - return false + return nil +} + +// compareArrayRuns determines whether an array matches a provided +// set of runs. As with compareArrayBitmap, it only verifies presence +// of the array's values in the run collection. the run collection +// can't be empty; if it were, N would have been 0, and we wouldn't +// have gotten here. +func compareArrayRuns(a []uint16, r []interval16) error { + ri := 0 + ru := r[ri] + ri++ + for _, v := range a { + if v < ru.start { + return fmt.Errorf("value %d missing", v) + } + if v > ru.last { + if ri >= len(r) { + return fmt.Errorf("value %d missing", v) + } + ru = r[ri] + ri++ + // if they're identical, the array value must be + // the start of the next run. + if v != ru.start { + return fmt.Errorf("value %d missing", v) + } + } } - if c.typ() == containerArray { - ca, c2a := c.array(), c2.array() - if len(ca) != len(c2a) { - return false - } - for i := 0; i < len(ca); i++ { - if ca[i] != c2a[i] { - return false - } - } - } else if c.typ() == containerBitmap { - cb, c2b := c.bitmap(), c2.bitmap() - if len(cb) != len(c2b) { - return false - } - for i := 0; i < len(cb); i++ { - if cb[i] != c2b[i] { - return false - } - } - } else if c.typ() == containerRun { - cr, c2r := c.runs(), c2.runs() - if len(cr) != len(c2r) { - return false - } - for i := 0; i < len(cr); i++ { - if cr[i] != c2r[i] { - return false - } - } - } else { - panic(fmt.Sprintf("unknown container type: %v", c.typ())) + return nil +} + +// compareArrayArray reports whether everything in a1 is equal to everything +// in a2. +func compareArrayArray(a1, a2 []uint16) error { + if len(a1) != len(a2) { + return fmt.Errorf("unexpected length mismatch, %d vs %d", len(a1), len(a2)) } - return true + for i := range a1 { + if a1[i] != a2[i] { + return fmt.Errorf("item %d: %d vs %d", i, a1[i], a2[i]) + } + } + return nil +} + +// BitwiseCompare reports whether two containers are equal. It returns +// an error describing any difference it finds. This is mostly intended +// for use in tests that expect equality. +func (c *Container) BitwiseCompare(c2 *Container) error { + if c.N() != c2.N() { + return errors.New("containers are different lengths") + } + if c.N() == 0 { + return nil + } + switch typePair(c.typ(), c2.typ()) { + case typePair(containerArray, containerArray): + return compareArrayArray(c.array(), c2.array()) + case typePair(containerArray, containerBitmap): + return compareArrayBitmap(c.array(), c2.bitmap()) + case typePair(containerBitmap, containerArray): + return compareArrayBitmap(c2.array(), c.bitmap()) + case typePair(containerArray, containerRun): + return compareArrayRuns(c.array(), c2.runs()) + case typePair(containerRun, containerArray): + return compareArrayRuns(c2.array(), c.runs()) + default: + c3 := xor(c, c2) + if c3.N() != 0 { + return fmt.Errorf("%d bits differenct between containers", c3.N()) + } + } + return nil } func unionArrayBitmap(a, b *Container) *Container { @@ -5855,35 +5900,6 @@ func (b *Bitmap) BitwiseEqual(c *Bitmap) (bool, error) { return true, nil } -func bitmapsEqual(b, c *Bitmap) error { // nolint: deadcode - statsHit("bitmapsEqual") - if b.OpWriter != c.OpWriter { - return errors.New("opWriters not equal") - } - if b.opN != c.opN { - return errors.New("opNs not equal") - } - - biter, _ := b.Containers.Iterator(0) - citer, _ := c.Containers.Iterator(0) - bn, cn := biter.Next(), citer.Next() - for ; bn && cn; bn, cn = biter.Next(), citer.Next() { - bk, bc := biter.Value() - ck, cc := citer.Value() - if bk != ck { - return errors.New("keys not equal") - } - if !bc.equals(cc) { - return errors.New("containers not equal") - } - } - if bn && !cn || cn && !bn { - return errors.New("different numbers of containers") - } - - return nil -} - func popcount(x uint64) uint64 { return uint64(bits.OnesCount64(x)) } diff --git a/roaring/roaring_helpers_test.go b/roaring/roaring_helpers_test.go index 41d4059fb..f8de764fb 100644 --- a/roaring/roaring_helpers_test.go +++ b/roaring/roaring_helpers_test.go @@ -14,6 +14,8 @@ package roaring +import "sync" + /////////////////////////////////////////////////////////////////////////// var containerWidth uint64 = 65536 @@ -261,51 +263,56 @@ func doContainer(typ byte, data interface{}) *Container { return nil } +var makeCts sync.Once +var sampleTestContainers map[byte]map[string]*Container + func setupContainerTests() map[byte]map[string]*Container { - cts := make(map[byte]map[string]*Container) + makeCts.Do(func() { + sampleTestContainers = make(map[byte]map[string]*Container) - // array containers - cts[containerArray] = map[string]*Container{ - "empty": doContainer(containerArray, arrayEmpty()), - "full": doContainer(containerArray, arrayFull()), - "firstBitSet": doContainer(containerArray, arrayFirstBitSet()), - "lastBitSet": doContainer(containerArray, arrayLastBitSet()), - "firstBitUnset": doContainer(containerArray, arrayFirstBitUnset()), - "lastBitUnset": doContainer(containerArray, arrayLastBitUnset()), - "innerBitsSet": doContainer(containerArray, arrayInnerBitsSet()), - "outerBitsSet": doContainer(containerArray, arrayOuterBitsSet()), - "oddBitsSet": doContainer(containerArray, arrayOddBitsSet()), - "evenBitsSet": doContainer(containerArray, arrayEvenBitsSet()), - } + // array containers + sampleTestContainers[containerArray] = map[string]*Container{ + "empty": doContainer(containerArray, arrayEmpty()), + "full": doContainer(containerArray, arrayFull()), + "firstBitSet": doContainer(containerArray, arrayFirstBitSet()), + "lastBitSet": doContainer(containerArray, arrayLastBitSet()), + "firstBitUnset": doContainer(containerArray, arrayFirstBitUnset()), + "lastBitUnset": doContainer(containerArray, arrayLastBitUnset()), + "innerBitsSet": doContainer(containerArray, arrayInnerBitsSet()), + "outerBitsSet": doContainer(containerArray, arrayOuterBitsSet()), + "oddBitsSet": doContainer(containerArray, arrayOddBitsSet()), + "evenBitsSet": doContainer(containerArray, arrayEvenBitsSet()), + } - // bitmap containers - cts[containerBitmap] = map[string]*Container{ - "empty": doContainer(containerBitmap, bitmapEmpty()), - "full": doContainer(containerBitmap, bitmapFull()), - "firstBitSet": doContainer(containerBitmap, bitmapFirstBitSet()), - "lastBitSet": doContainer(containerBitmap, bitmapLastBitSet()), - "firstBitUnset": doContainer(containerBitmap, bitmapFirstBitUnset()), - "lastBitUnset": doContainer(containerBitmap, bitmapLastBitUnset()), - "innerBitsSet": doContainer(containerBitmap, bitmapInnerBitsSet()), - "outerBitsSet": doContainer(containerBitmap, bitmapOuterBitsSet()), - "oddBitsSet": doContainer(containerBitmap, bitmapOddBitsSet()), - "evenBitsSet": doContainer(containerBitmap, bitmapEvenBitsSet()), - } + // bitmap containers + sampleTestContainers[containerBitmap] = map[string]*Container{ + "empty": doContainer(containerBitmap, bitmapEmpty()), + "full": doContainer(containerBitmap, bitmapFull()), + "firstBitSet": doContainer(containerBitmap, bitmapFirstBitSet()), + "lastBitSet": doContainer(containerBitmap, bitmapLastBitSet()), + "firstBitUnset": doContainer(containerBitmap, bitmapFirstBitUnset()), + "lastBitUnset": doContainer(containerBitmap, bitmapLastBitUnset()), + "innerBitsSet": doContainer(containerBitmap, bitmapInnerBitsSet()), + "outerBitsSet": doContainer(containerBitmap, bitmapOuterBitsSet()), + "oddBitsSet": doContainer(containerBitmap, bitmapOddBitsSet()), + "evenBitsSet": doContainer(containerBitmap, bitmapEvenBitsSet()), + } - // run containers - cts[containerRun] = map[string]*Container{ - "empty": doContainer(containerRun, runEmpty()), - "full": doContainer(containerRun, runFull()), - "firstBitSet": doContainer(containerRun, runFirstBitSet()), - "lastBitSet": doContainer(containerRun, runLastBitSet()), - "firstBitUnset": doContainer(containerRun, runFirstBitUnset()), - "lastBitUnset": doContainer(containerRun, runLastBitUnset()), - "innerBitsSet": doContainer(containerRun, runInnerBitsSet()), - "outerBitsSet": doContainer(containerRun, runOuterBitsSet()), - "oddBitsSet": doContainer(containerRun, runOddBitsSet()), - "evenBitsSet": doContainer(containerRun, runEvenBitsSet()), - } + // run containers + sampleTestContainers[containerRun] = map[string]*Container{ + "empty": doContainer(containerRun, runEmpty()), + "full": doContainer(containerRun, runFull()), + "firstBitSet": doContainer(containerRun, runFirstBitSet()), + "lastBitSet": doContainer(containerRun, runLastBitSet()), + "firstBitUnset": doContainer(containerRun, runFirstBitUnset()), + "lastBitUnset": doContainer(containerRun, runLastBitUnset()), + "innerBitsSet": doContainer(containerRun, runInnerBitsSet()), + "outerBitsSet": doContainer(containerRun, runOuterBitsSet()), + "oddBitsSet": doContainer(containerRun, runOddBitsSet()), + "evenBitsSet": doContainer(containerRun, runEvenBitsSet()), + } + }) - return cts + return sampleTestContainers } diff --git a/roaring/roaring_internal_test.go b/roaring/roaring_internal_test.go index 7784064ab..3a0749cc8 100644 --- a/roaring/roaring_internal_test.go +++ b/roaring/roaring_internal_test.go @@ -2632,7 +2632,7 @@ func TestBitmapClone(t *testing.T) { } } c := b.Clone() - if err := bitmapsEqual(b, c); err != nil { + if _, err := b.BitwiseEqual(c); err != nil { t.Fatalf("Clone Objects not equal: %v\n", err) } d := func() *Bitmap { //anybody know how to declare a nil value? @@ -2735,34 +2735,46 @@ func getFunctionName(i interface{}) string { return y[0] } -// UnionInPlace is defined at the Bitmap level, but this wrapper lets us insert -// it into our ContainerCombinations tests so that it gets exercised on a wide -// variety of container data. func unionInPlaceWrapper(a, b *Container) *Container { - out := NewBitmap() - out.Containers.Put(0, a.Clone()) - B := NewBitmap() - B.Containers.Put(0, b) - out.UnionInPlace(B) - return out.Containers.Get(0) + ret := a.Clone().unionInPlace(b) + ret.Repair() + return ret } func differenceInPlaceWrapper(a, b *Container) *Container { - out := NewBitmap() - out.Containers.Put(0, a.Clone()) - B := NewBitmap() - B.Containers.Put(0, b) - out.DifferenceInPlace(B) - return out.Containers.Get(0) + a = a.Clone() + // this should probably return its new value, but currently does not + a.differenceInPlace(b) + return a } func intersectInPlaceWrapper(a, b *Container) *Container { - out := NewBitmap() - out.Containers.Put(0, a.Clone()) - B := NewBitmap() - B.Containers.Put(0, b) - out.IntersectInPlace(B) - return out.Containers.Get(0) + return a.Clone().intersectInPlace(b) +} + +func TestContainerBitwiseCompare(t *testing.T) { + cts := setupContainerTests() + + for t1, containers := range cts { + for name, c := range containers { + for t2, other := range cts { + for otherName, otherC := range other { + err := c.BitwiseCompare(otherC) + if err != nil { + if otherName == name { + t.Fatalf("container types %d/%d, contents %s: unexpected error %v", + t1, t2, name, err) + } + } else { + if name != otherName { + t.Fatalf("container types %d/%d, unexpected %s == %s", + t1, t2, name, otherName) + } + } + } + } + } + } } func TestContainerCombinations(t *testing.T) { @@ -3562,51 +3574,8 @@ func TestContainerCombinations(t *testing.T) { // Convert to all container types and check result. for _, ct := range containerTypes { - clone := ret.Clone() - if ct == containerArray { - if clone == nil { - clone = NewContainerArray(nil) - } else if clone.isBitmap() { - clone = clone.bitmapToArray() - } else if clone.isRun() { - clone = clone.runToArray() - } - if clone.N() != cts[ct][exp].N() { - t.Errorf("test %s expected array n=%d, but got n=%d", desc, cts[ct][exp].N(), clone.N()) - } - // Because xorRunRun resulting in an empty container returns an array container with a - // nil slice array, then we need to check len() on array first (look for 0). - if !(len(clone.array()) == 0 && len(cts[ct][exp].array()) == 0) && !reflect.DeepEqual(clone.array(), cts[ct][exp].array()) { - t.Errorf("test %s expected array %X, but got %X", desc, cts[ct][exp].array(), clone.array()) - } - } else if ct == containerBitmap { - if clone == nil { - clone = NewContainerBitmap(0, nil) - } else if clone.isArray() { - clone = clone.arrayToBitmap() - } else if clone.isRun() { - clone = clone.runToBitmap() - } - if clone.N() != cts[ct][exp].N() { - t.Errorf("test %s expected bitmap n=%d, but got n=%d", desc, cts[ct][exp].N(), clone.N()) - } - if !reflect.DeepEqual(clone.bitmap(), cts[ct][exp].bitmap()) { - t.Errorf("test %s expected bitmap %X, but got %X", desc, cts[ct][exp].bitmap(), clone.bitmap()) - } - } else if ct == containerRun { - if clone == nil { - clone = NewContainerRun(nil) - } else if clone.isArray() { - clone = clone.arrayToRun(0) - } else if clone.isBitmap() { - clone = clone.bitmapToRun(0) - } - if clone.N() != cts[ct][exp].N() { - t.Errorf("test %s expected runs n=%d, but got n=%d", desc, cts[ct][exp].N(), clone.N()) - } - if !reflect.DeepEqual(clone.runs(), cts[ct][exp].runs()) { - t.Errorf("test %s expected runs %X, but got %X", desc, cts[ct][exp].runs(), clone.runs()) - } + if err := ret.BitwiseCompare(cts[ct][exp]); err != nil { + t.Errorf("test %s: %v", desc, err) } } } diff --git a/roaring/roaring_test.go b/roaring/roaring_test.go index c8eb2617a..55395ea73 100644 --- a/roaring/roaring_test.go +++ b/roaring/roaring_test.go @@ -1837,18 +1837,6 @@ func getBenchData(tb testing.TB) *benchmarkSampleData { return data } -func diff(a, b []uint64) string { - if len(a) != len(b) { - return fmt.Sprintf("len: %d != %d", len(a), len(b)) - } - for i := range a { - if a[i] != b[i] { - return fmt.Sprintf("index %d: %d != %d", i, a[i], b[i]) - } - } - return "" -} - func TestBitmap_Intersect(t *testing.T) { bm0 := testBM() result := bm0.Intersect(bm0)