From 0cc2f9eb719508032132151254f53164195a70f4 Mon Sep 17 00:00:00 2001 From: Matt Jaffee Date: Thu, 18 May 2017 12:04:37 -0500 Subject: [PATCH 1/4] fix 3 separate bugs in bitmapCountRange in order of the diff: 1. When the start and end of the range fall in the same word, special handling is needed to "mask" off the beginning and end of the word simultaneously to avoid counting bits at the beginning or end of the word that aren't in the range. 2. `i++` is needed at the end of the first partial word to avoid counting this word in the next block. 3. the shift amount for right shifts is 64 - (end % 64) rather than just end % 64. If end is (e.g.) 68, then 68 - 64 is 4 and we are only interested in the first 4 bits of the word, so we must right shift by 60 bits, not 4 bits. --- roaring/roaring.go | 10 +++++++++- roaring/roaring_internal_test.go | 31 +++++++++++++++++++++++++++++++ 2 files changed, 40 insertions(+), 1 deletion(-) create mode 100644 roaring/roaring_internal_test.go diff --git a/roaring/roaring.go b/roaring/roaring.go index e74739311..1a1f8d011 100644 --- a/roaring/roaring.go +++ b/roaring/roaring.go @@ -890,9 +890,17 @@ func (c *container) bitmapCountRange(start, end uint32) int { var n uint64 i, j := start/64, end/64 + // Special case when start and end fall in the same word. + if i == j { + offi, offj := start%64, 64-end%64 + n += popcount((c.bitmap[i] << offi) >> (offj + offi)) + return int(n) + } + // Count partial starting word. if off := start % 64; off != 0 { n += popcount(c.bitmap[i] << off) + i++ } // Count words in between. @@ -902,7 +910,7 @@ func (c *container) bitmapCountRange(start, end uint32) int { // Count partial ending word. if int(j) < len(c.bitmap) { - if off := end % 64; off != 0 { + if off := 64 - (end % 64); off != 0 { n += popcount(c.bitmap[j] >> off) } } diff --git a/roaring/roaring_internal_test.go b/roaring/roaring_internal_test.go new file mode 100644 index 000000000..17a35c5e1 --- /dev/null +++ b/roaring/roaring_internal_test.go @@ -0,0 +1,31 @@ +package roaring + +import ( + "testing" +) + +func TestBitmapCountRange(t *testing.T) { + c := container{bitmap: []uint64{1}} + cnt := c.bitmapCountRange(63, 65) + if cnt != 1 { + t.Fatalf("count of %v from 63 to 65 should be 1, but got %v", c.bitmap, cnt) + } + + c = container{bitmap: []uint64{0, 0x8000000000000000}} + cnt = c.bitmapCountRange(65, 66) + if cnt != 0 { + t.Fatalf("count of %v from 65 to 66 should be 0, but got %v", c.bitmap, cnt) + } + + c = container{bitmap: []uint64{0, 0xF000000000000000}} + cnt = c.bitmapCountRange(65, 66) + if cnt != 1 { + t.Fatalf("count of %v from 65 to 66 should be 1, but got %v", c.bitmap, cnt) + } + + c = container{bitmap: []uint64{0x1, 0xFF00000000000000}} + cnt = c.bitmapCountRange(62, 66) + if cnt != 3 { + t.Fatalf("count of %v from 62 to 66 should be 3, but got %v", c.bitmap, cnt) + } +} From bc49ffb162671558ef8f04c045ad8075f252e097 Mon Sep 17 00:00:00 2001 From: Matt Jaffee Date: Fri, 19 May 2017 15:15:23 -0500 Subject: [PATCH 2/4] fix bitmapCountRange and test had been thinking that index 0 was the most significant bit, but based on the bitmapAdd function, it must be the least significant bit --- roaring/roaring.go | 6 ++--- roaring/roaring_internal_test.go | 40 +++++++++++++++----------------- 2 files changed, 22 insertions(+), 24 deletions(-) diff --git a/roaring/roaring.go b/roaring/roaring.go index 1a1f8d011..b1e0b7464 100644 --- a/roaring/roaring.go +++ b/roaring/roaring.go @@ -893,13 +893,13 @@ func (c *container) bitmapCountRange(start, end uint32) int { // Special case when start and end fall in the same word. if i == j { offi, offj := start%64, 64-end%64 - n += popcount((c.bitmap[i] << offi) >> (offj + offi)) + n += popcount((c.bitmap[i] >> offi) << (offj + offi)) return int(n) } // Count partial starting word. if off := start % 64; off != 0 { - n += popcount(c.bitmap[i] << off) + n += popcount(c.bitmap[i] >> off) i++ } @@ -911,7 +911,7 @@ func (c *container) bitmapCountRange(start, end uint32) int { // Count partial ending word. if int(j) < len(c.bitmap) { if off := 64 - (end % 64); off != 0 { - n += popcount(c.bitmap[j] >> off) + n += popcount(c.bitmap[j] << off) } } diff --git a/roaring/roaring_internal_test.go b/roaring/roaring_internal_test.go index 17a35c5e1..5cff4a30f 100644 --- a/roaring/roaring_internal_test.go +++ b/roaring/roaring_internal_test.go @@ -5,27 +5,25 @@ import ( ) func TestBitmapCountRange(t *testing.T) { - c := container{bitmap: []uint64{1}} - cnt := c.bitmapCountRange(63, 65) - if cnt != 1 { - t.Fatalf("count of %v from 63 to 65 should be 1, but got %v", c.bitmap, cnt) + c := container{} + tests := []struct { + start uint32 + end uint32 + bitmap []uint64 + exp int + }{ + {start: 0, end: 1, bitmap: []uint64{1}, exp: 1}, + {start: 2, end: 7, bitmap: []uint64{0xFFFFFFFFFFFFFF18}, exp: 2}, + {start: 67, end: 68, bitmap: []uint64{0, 0x8}, exp: 1}, + {start: 1, end: 68, bitmap: []uint64{0x3, 0x8, 0xF}, exp: 2}, + {start: 1, end: 258, bitmap: []uint64{0xF, 0x8, 0xA, 0x4, 0xFFFFFFFFFFFFFFFF}, exp: 9}, + {start: 66, end: 71, bitmap: []uint64{0xF, 0xFFFFFFFFFFFFFF18}, exp: 2}, + {start: 63, end: 64, bitmap: []uint64{0x8000000000000000}, exp: 1}, } - - c = container{bitmap: []uint64{0, 0x8000000000000000}} - cnt = c.bitmapCountRange(65, 66) - if cnt != 0 { - t.Fatalf("count of %v from 65 to 66 should be 0, but got %v", c.bitmap, cnt) - } - - c = container{bitmap: []uint64{0, 0xF000000000000000}} - cnt = c.bitmapCountRange(65, 66) - if cnt != 1 { - t.Fatalf("count of %v from 65 to 66 should be 1, but got %v", c.bitmap, cnt) - } - - c = container{bitmap: []uint64{0x1, 0xFF00000000000000}} - cnt = c.bitmapCountRange(62, 66) - if cnt != 3 { - t.Fatalf("count of %v from 62 to 66 should be 3, but got %v", c.bitmap, cnt) + for i, test := range tests { + c.bitmap = test.bitmap + if ret := c.bitmapCountRange(test.start, test.end); ret != test.exp { + t.Fatalf("test #%v count of %v from %v to %v should be %v but got %v", i, test.bitmap, test.start, test.end, test.exp, ret) + } } } From 022d84e8aae5e14cfe3b85c8603812f1cc53272c Mon Sep 17 00:00:00 2001 From: Matt Jaffee Date: Fri, 19 May 2017 17:09:22 -0500 Subject: [PATCH 3/4] fix offset check to make sense --- roaring/roaring.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/roaring/roaring.go b/roaring/roaring.go index b1e0b7464..ba1774678 100644 --- a/roaring/roaring.go +++ b/roaring/roaring.go @@ -910,7 +910,7 @@ func (c *container) bitmapCountRange(start, end uint32) int { // Count partial ending word. if int(j) < len(c.bitmap) { - if off := 64 - (end % 64); off != 0 { + if off := 64 - (end % 64); off != 64 { n += popcount(c.bitmap[j] << off) } } From eae9d5c7f6094b597452d9aed3b95f9d72a2e579 Mon Sep 17 00:00:00 2001 From: Matt Jaffee Date: Mon, 22 May 2017 10:38:40 -0500 Subject: [PATCH 4/4] remove branch and always count last container --- roaring/roaring.go | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/roaring/roaring.go b/roaring/roaring.go index ba1774678..0a91c4493 100644 --- a/roaring/roaring.go +++ b/roaring/roaring.go @@ -910,9 +910,8 @@ func (c *container) bitmapCountRange(start, end uint32) int { // Count partial ending word. if int(j) < len(c.bitmap) { - if off := 64 - (end % 64); off != 64 { - n += popcount(c.bitmap[j] << off) - } + off := 64 - (end % 64) + n += popcount(c.bitmap[j] << off) } return int(n)