From c8e6fd2e43581e83dc357750c714e6d7fd36f529 Mon Sep 17 00:00:00 2001 From: Seebs Date: Fri, 9 Nov 2018 22:44:37 -0600 Subject: [PATCH 1/7] improve type matrix for IntersectionCount benchmarks The circumstances under which bitmaps are converted between types are not 100% nailed down, and the IntersectionCount benchmark was actually using a bitmap for the "run" data set as well as for the "bitmap" data set. Fix that by using Optimize() explicitly. Also, add a second RLE set so we can compare the difference between "one run for the entire set" and "several runs". Also add array/array comparisons. We use two different lengths of arrays, because performance turns out to vary between "first array longer" and "second array longer". Also added a benchmark for getBenchData itself, since it's at least one possible use case for "creating a lot of containers". --- roaring/roaring_test.go | 117 ++++++++++++++++++++++++++++++++++------ 1 file changed, 101 insertions(+), 16 deletions(-) diff --git a/roaring/roaring_test.go b/roaring/roaring_test.go index 64cb45e81..6c31b7701 100644 --- a/roaring/roaring_test.go +++ b/roaring/roaring_test.go @@ -1056,19 +1056,39 @@ func TestBitmapBufIterator(t *testing.T) { } -var benchmarkBitmapIntersectionCountData struct { - a, b, r *roaring.Bitmap +// this data is used to test various operations across +// different types. +type benchmarkSampleData struct { + a1, a2, b, r1, r2 *roaring.Bitmap } -func getBenchData() *struct{ a, b, r *roaring.Bitmap } { - data := &benchmarkBitmapIntersectionCountData - if data.a == nil { +var sampleData benchmarkSampleData + +func isAllType(b *roaring.Bitmap, typ string) bool { + bi := b.Info() + for _, c := range bi.Containers { + if c.Type != typ { + return false + } + } + return true +} + +func getBenchData(b *testing.B) *benchmarkSampleData { + data := &sampleData + if data.a1 == nil { const max = (1 << 24) / 64 // Build bitmap with array container. - data.a = roaring.NewFileBitmap() - for i, n := 0, 2*roaring.ArrayMaxSize/3; i < n; i++ { - data.a.Add(uint64(rand.Intn(max))) + data.a1 = roaring.NewFileBitmap() + data.a2 = roaring.NewFileBitmap() + // two lists of different lengths + for i, n := 0, roaring.ArrayMaxSize/3; i < n; i++ { + data.a1.Add(uint64(rand.Intn(max))) + data.a2.Add(uint64(rand.Intn(max))) + } + for i, n := 0, roaring.ArrayMaxSize/3; i < n; i++ { + data.a1.Add(uint64(rand.Intn(max))) } // Build bitmap with bitmap container. @@ -1078,12 +1098,42 @@ func getBenchData() *struct{ a, b, r *roaring.Bitmap } { } // build bitmap with run container - data.r = roaring.NewFileBitmap() + data.r1 = roaring.NewFileBitmap() for i, n := 0, MaxContainerVal; i < n; i++ { - data.r.Add(uint64(i)) + data.r1.Add(uint64(i)) } + // build bitmap with multiple runs + data.r2 = roaring.NewFileBitmap() + for i, n := 0, MaxContainerVal; i < n; i++ { + data.r2.Add(uint64(i)) + // break the runs up, this should produce 16 runs, which + // is small enough to make RLE tempting + if i&0xfff == 0xfff { + i += 5 + } + } + data.a1.Optimize() + data.a2.Optimize() + data.b.Optimize() + data.r1.Optimize() + data.r2.Optimize() } + if !isAllType(data.a1, "array") { + b.Fatalf("expected data.a1 to be an array, it wasn't.") + } + if !isAllType(data.a2, "array") { + b.Fatalf("expected data.a2 to be an array, it wasn't.") + } + if !isAllType(data.b, "bitmap") { + b.Fatalf("expected data.b to be a bitmap, it wasn't.") + } + if !isAllType(data.r1, "run") { + b.Fatalf("expected data.r1 to be RLE, it wasn't.") + } + if !isAllType(data.r2, "run") { + b.Fatalf("expected data.r2 to be RLE, it wasn't.") + } return data } @@ -1138,30 +1188,65 @@ func TestBitmap_Intersect(t *testing.T) { } } +func BenchmarkGetBenchData(b *testing.B) { + for i := 0; i < b.N; i++ { + sampleData = benchmarkSampleData{} + getBenchData(b) + } +} + func BenchmarkBitmap_IntersectionCount_ArrayRun(b *testing.B) { - data := getBenchData() + data := getBenchData(b) // Reset timer & benchmark. b.ResetTimer() for i := 0; i < b.N; i++ { - data.a.IntersectionCount(data.r) + data.a1.IntersectionCount(data.r1) + } +} + +func BenchmarkBitmap_IntersectionCount_ArrayRuns(b *testing.B) { + data := getBenchData(b) + // Reset timer & benchmark. + b.ResetTimer() + for i := 0; i < b.N; i++ { + data.a1.IntersectionCount(data.r2) } } func BenchmarkBitmap_IntersectionCount_BitmapRun(b *testing.B) { - data := getBenchData() + data := getBenchData(b) // Reset timer & benchmark. b.ResetTimer() for i := 0; i < b.N; i++ { - data.b.IntersectionCount(data.r) + data.b.IntersectionCount(data.r1) + } +} + +func BenchmarkBitmap_IntersectionCount_BitmapRuns(b *testing.B) { + data := getBenchData(b) + // Reset timer & benchmark. + b.ResetTimer() + for i := 0; i < b.N; i++ { + data.b.IntersectionCount(data.r2) + } +} + +func BenchmarkBitmap_IntersectionCount_ArrayArray(b *testing.B) { + data := getBenchData(b) + // Reset timer & benchmark. + b.ResetTimer() + for i := 0; i < b.N; i++ { + data.a1.IntersectionCount(data.a2) + data.a2.IntersectionCount(data.a1) } } func BenchmarkBitmap_IntersectionCount_ArrayBitmap(b *testing.B) { - data := getBenchData() + data := getBenchData(b) // Reset timer & benchmark. b.ResetTimer() for i := 0; i < b.N; i++ { - data.a.IntersectionCount(data.b) + data.a1.IntersectionCount(data.b) } } From 32c4b3540f33d9384e291ce16e481e4384184482 Mon Sep 17 00:00:00 2001 From: Seebs Date: Tue, 13 Nov 2018 12:12:13 -0600 Subject: [PATCH 2/7] simplify intersectBitmapRun output to remove a conversion If the total number of things returned was small enough to make an array, intersectBitmapRun converted to an array. This seems possibly-premature; future processing might well prefer a bitmap. We know everything gets optimized before being written out, let's not convert without a specific reason. But also, let's use an array no matter which container is small enough to prove that we can do so safely. Fixes #854. --- roaring/roaring.go | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/roaring/roaring.go b/roaring/roaring.go index 3a4050cea..ced003d37 100644 --- a/roaring/roaring.go +++ b/roaring/roaring.go @@ -2102,12 +2102,12 @@ func intersectRunRun(a, b *Container) *Container { return output } -// intersectBitmapRun returns an array container if the run container's -// cardinality is < ArrayMaxSize. Otherwise it returns a bitmap container. +// intersectBitmapRun returns an array container if either container's +// cardinality is <= ArrayMaxSize. Otherwise it returns a bitmap container. func intersectBitmapRun(a, b *Container) *Container { statsHit("intersect/BitmapRun") var output *Container - if b.n < ArrayMaxSize { + if b.n <= ArrayMaxSize || a.n <= ArrayMaxSize { // output is array container output = &Container{containerType: containerArray} for _, iv := range b.runs { @@ -2161,9 +2161,6 @@ func intersectBitmapRun(a, b *Container) *Container { valast = vastart + 63 } } - if output.n < ArrayMaxSize { - output.bitmapToArray() - } } return output } From d4364bea527f3a7d023e41974dc97b2c2ef8cc13 Mon Sep 17 00:00:00 2001 From: Seebs Date: Mon, 12 Nov 2018 18:04:54 -0600 Subject: [PATCH 3/7] slightly streamline array/array comparison The net effect of this is to not recompute "the current value of the first array" on every loop, pretty much. However, the swap to make sure the inner loop is on the longer array seems to be significant for performance. On my system, this moves runtime from ~29us per op to ~17us per op. --- roaring/roaring.go | 28 +++++++++++++++++++--------- 1 file changed, 19 insertions(+), 9 deletions(-) diff --git a/roaring/roaring.go b/roaring/roaring.go index ced003d37..6cf7eda1c 100644 --- a/roaring/roaring.go +++ b/roaring/roaring.go @@ -1903,16 +1903,26 @@ func intersectionCount(a, b *Container) int32 { func intersectionCountArrayArray(a, b *Container) (n int32) { statsHit("intersectionCount/ArrayArray") - na, nb := len(a.array), len(b.array) - for i, j := 0, 0; i < na && j < nb; { - va, vb := a.array[i], b.array[j] - if va < vb { - i++ - } else if va > vb { - j++ - } else { + s1, s2 := a.array, b.array + if len(s1) == 0 || len(s2) == 0 { + return 0 + } + if len(s1) > len(s2) { + s1, s2 = s2, s1 + } + l2 := len(s2) + i2 := 0 + v2 := s2[0] + for _, v1 := range s1 { + for v2 < v1 { + i2++ + if i2 >= l2 { + return n + } + v2 = s2[i2] + } + if v2 == v1 { n++ - i, j = i+1, j+1 } } return n From 9b552ab5086ef9a9937baf06b773a427bf245ae9 Mon Sep 17 00:00:00 2001 From: Seebs Date: Tue, 13 Nov 2018 12:21:47 -0600 Subject: [PATCH 4/7] enhance TestRunCountRange confirm that the number of runs comes out as expected, and add a couple of numbers out of order to verify that the 17-18-19 set gets coalesced into one run even if we add 17 and 19 before 18. --- roaring/roaring_internal_test.go | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/roaring/roaring_internal_test.go b/roaring/roaring_internal_test.go index 266bf39e7..78b686371 100644 --- a/roaring/roaring_internal_test.go +++ b/roaring/roaring_internal_test.go @@ -165,8 +165,8 @@ func TestRunCountRange(t *testing.T) { } c.add(17) - c.add(18) c.add(19) + c.add(18) cnt = c.runCountRange(1, 22) if cnt != 10 { @@ -180,6 +180,11 @@ func TestRunCountRange(t *testing.T) { if cnt != 9 { t.Fatalf("should get 9 from multiple ranges overlapping both sides, but got: %v", cnt) } + // verify that the disparate ops resulted in three separate runs + cnt = c.countRuns() + if cnt != 3 { + t.Fatalf("should get 3 total runs, but got: %v [%v]", cnt, c.runs) + } } func TestRunContains(t *testing.T) { From 1a8633f3a5eaafb03b2b5e384c7e70e04fec57d8 Mon Sep 17 00:00:00 2001 From: Seebs Date: Tue, 13 Nov 2018 13:33:52 -0600 Subject: [PATCH 5/7] use roaring conventions for variable names Roaring likes to call things "a" and "b", not "1" and "2", and use "n" for length, not "l", etcetera. Adopt these conventions to make code more readable. Also drop the 'vb' value since it isn't expensive to compute and the compiler can figure out that it can reuse the value. --- roaring/roaring.go | 25 ++++++++++++------------- 1 file changed, 12 insertions(+), 13 deletions(-) diff --git a/roaring/roaring.go b/roaring/roaring.go index 6cf7eda1c..bb3c5da6e 100644 --- a/roaring/roaring.go +++ b/roaring/roaring.go @@ -1903,25 +1903,24 @@ func intersectionCount(a, b *Container) int32 { func intersectionCountArrayArray(a, b *Container) (n int32) { statsHit("intersectionCount/ArrayArray") - s1, s2 := a.array, b.array - if len(s1) == 0 || len(s2) == 0 { + ca, cb := a.array, b.array + na, nb := len(ca), len(cb) + if na == 0 || nb == 0 { return 0 } - if len(s1) > len(s2) { - s1, s2 = s2, s1 + if na > nb { + ca, cb = cb, ca + na, nb = nb, na } - l2 := len(s2) - i2 := 0 - v2 := s2[0] - for _, v1 := range s1 { - for v2 < v1 { - i2++ - if i2 >= l2 { + j := 0 + for _, va := range ca { + for cb[j] < va { + j++ + if j >= nb { return n } - v2 = s2[i2] } - if v2 == v1 { + if cb[j] == va { n++ } } From 7c82f4804604a12c1efa1c688110dc6aeead2ab0 Mon Sep 17 00:00:00 2001 From: Seebs Date: Tue, 13 Nov 2018 14:06:15 -0600 Subject: [PATCH 6/7] improve testing for intersections of array/array pairs A transient bug introduced in intersectionCountArrayArray was not caught by the tests, because it would only manifest when two containers of different lengths were being compared. Also improve the testing for intersectArrayArray, even though that code hasn't been changed. --- roaring/roaring_test.go | 24 ++++++++++++++++++++---- 1 file changed, 20 insertions(+), 4 deletions(-) diff --git a/roaring/roaring_test.go b/roaring/roaring_test.go index 6c31b7701..f5c016ea3 100644 --- a/roaring/roaring_test.go +++ b/roaring/roaring_test.go @@ -411,13 +411,29 @@ func TestBitmap_Intersection_Empty(t *testing.T) { } func TestBitmap_IntersectArrayArray(t *testing.T) { - bm0 := roaring.NewFileBitmap(0, 1, 2683, 5005) + bm0 := roaring.NewFileBitmap(0, 1, 7, 9, 11, 2683, 5005) bm1 := roaring.NewFileBitmap(0, 2683, 2684, 5000) + expected := []uint64{0, 2683} result := bm0.Intersect(bm1) if n := result.Count(); n != 2 { t.Fatalf("unexpected n: %d", n) } + for _, e := range expected { + if !result.Contains(e) { + t.Fatalf("missing value %d", e) + } + } + // confirm that it also works going the other way + result = bm1.Intersect(bm0) + if n := result.Count(); n != 2 { + t.Fatalf("unexpected n: %d", n) + } + for _, e := range expected { + if !result.Contains(e) { + t.Fatalf("missing value %d", e) + } + } } func TestBitmap_IntersectBitmapBitmap(t *testing.T) { @@ -689,10 +705,10 @@ func TestBitmap_Flip_After(t *testing.T) { } -// Ensure bitmap can return the number of intersecting bits in two bitmaps. +// Ensure bitmap can return the number of intersecting bits in two arrays. func TestBitmap_IntersectionCount_ArrayArray(t *testing.T) { - bm0 := roaring.NewFileBitmap(0, 1, 1000001, 1000002, 1000003) - bm1 := roaring.NewFileBitmap(0, 50000, 1000001, 1000002) + bm0 := roaring.NewFileBitmap(0, 1000001, 1000002, 1000003) + bm1 := roaring.NewFileBitmap(0, 50000, 999998, 999999, 1000000, 1000001, 1000002) if n := bm0.IntersectionCount(bm1); n != 3 { t.Fatalf("unexpected n: %d", n) From e20671b2b4c8a542fd30ffb1829b836aa38dc5de Mon Sep 17 00:00:00 2001 From: Seebs Date: Tue, 13 Nov 2018 14:20:25 -0600 Subject: [PATCH 7/7] silence gometalinter I am aware that I don't actually ever use the length of a after this line of code, but if I don't correctly update it, any future change that needs that length will break mysteriously. We humbly ask gometalinter to consider the reply of counsel in _Arkell v. Pressdram_ (1971). --- roaring/roaring.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/roaring/roaring.go b/roaring/roaring.go index bb3c5da6e..9c4274df9 100644 --- a/roaring/roaring.go +++ b/roaring/roaring.go @@ -1910,7 +1910,7 @@ func intersectionCountArrayArray(a, b *Container) (n int32) { } if na > nb { ca, cb = cb, ca - na, nb = nb, na + na, nb = nb, na // nolint: ineffassign } j := 0 for _, va := range ca {