diff --git a/rbf/cursor.go b/rbf/cursor.go index 20d8c0e7a..40d0c5e86 100644 --- a/rbf/cursor.go +++ b/rbf/cursor.go @@ -1180,7 +1180,7 @@ func (c *Cursor) merge(key uint64, data *roaring.Container) (bool, error) { if err != nil { return false, errors.Wrap(err, "cursor.merge") } - container = roaring.NewContainerBitmap(cell.BitN, d) + container = roaring.NewContainerBitmap(-1, d) case ContainerTypeRLE: d := toInterval16(cell.Data) container = roaring.NewContainerRun(d) @@ -1267,7 +1267,7 @@ func (c *Cursor) difference(key uint64, data *roaring.Container) (bool, error) { if err != nil { return false, errors.Wrap(err, "cursor.difference") } - container = roaring.NewContainerBitmap(cell.N, d) + container = roaring.NewContainerBitmap(-1, d) case ContainerTypeRLE: d := toInterval16(cell.Data) container = roaring.NewContainerRun(d) diff --git a/rbf/cursorx.go b/rbf/cursorx.go index 26d0c002d..5cf75af24 100644 --- a/rbf/cursorx.go +++ b/rbf/cursorx.go @@ -179,12 +179,16 @@ func toContainer(l leafCell, tx *Tx) (c *roaring.Container) { cloneMaybe = make([]uint64, len(bm)) copy(cloneMaybe, bm) } - c = roaring.NewContainerBitmap(l.N, cloneMaybe) + c = roaring.NewContainerBitmap(-1, cloneMaybe) case ContainerTypeBitmap: - c = roaring.NewContainerBitmap(l.N, toArray64(cpMaybe)) + c = roaring.NewContainerBitmap(-1, toArray64(cpMaybe)) case ContainerTypeRLE: c = roaring.NewContainerRun(toInterval16(cpMaybe)) } + // Note: If the "roaringparanoia" build tag isn't set, this + // should be optimized away entirely. Otherwise it's moderately + // expensive. + c.CheckN() c.SetMapped(mapped) return c } diff --git a/roaring/container_stash.go b/roaring/container_stash.go index e87a65fb8..f6a9f8f05 100644 --- a/roaring/container_stash.go +++ b/roaring/container_stash.go @@ -146,6 +146,9 @@ func NewContainerBitmap(n int, bitmap []uint64) *Container { c.bitmapRepair() } else { c.setN(int32(n)) + if roaringParanoia { + c.CheckN() + } } return c } @@ -164,6 +167,9 @@ func NewContainerBitmapN(bitmap []uint64, n int32) *Container { } else { c.setBitmap(bitmap) } + if roaringParanoia { + c.CheckN() + } return c } @@ -219,6 +225,9 @@ func NewContainerRunCopy(set []Interval16) *Container { func NewContainerRunN(set []Interval16, n int32) *Container { c := &Container{typeID: ContainerRun, n: n} c.setRuns(set) + if roaringParanoia { + c.CheckN() + } return c } @@ -624,61 +633,6 @@ func (c *Container) setRunsMaybeCopy(runs []Interval16, doCopy bool) { c.pointer, c.len, c.cap = &runs[0].Start, int32(len(runs)), int32(cap(runs)) } -// UpdateOrMake updates the container, yielding a new container if necessary. -func (c *Container) UpdateOrMake(typ byte, n int32, mapped bool) *Container { - if c == nil { - switch typ { - case ContainerRun: - c = NewContainerRunN(nil, n) - case ContainerBitmap: - c = NewContainerBitmapN(nil, n) - default: - c = NewContainerArrayN(nil, n) - } - c.flags |= flagMapped - return c - } - // ensure that we are allowed to modify this container - c = c.Thaw() - c.typeID = typ - c.n = n - // note: this probably shouldn't be happening, the decision should be getting - // made when we specify the storage. - c.setMapped(mapped) - // we don't know that any existing slice is usable, so let's ditch it - switch c.typeID { - case ContainerArray: - c.pointer, c.len, c.cap = &c.data[0], 0, stashedArraySize - case ContainerRun: - c.pointer, c.len, c.cap = &c.data[0], 0, stashedRunSize - default: - c.pointer, c.len, c.cap = nil, 0, 0 - } - return c -} - -// Update updates the container if possible. It is an error to -// call Update on a frozen container. -func (c *Container) Update(typ byte, n int32, mapped bool) { - if c == nil || c.frozen() { - panic("cannot Update a nil or frozen container") - } - c.typeID = typ - c.n = n - // note: this probably shouldn't be happening, the decision should be getting - // made when we specify the storage. - c.setMapped(mapped) - // we don't know that any existing slice is usable, so let's ditch it - switch c.typeID { - case ContainerArray: - c.pointer, c.len, c.cap = nil, 0, 0 - case ContainerRun: - c.pointer, c.len, c.cap = nil, 0, 0 - default: - c.pointer, c.len, c.cap = nil, 0, 0 - } -} - // isArray returns true if the container is an array container. func (c *Container) isArray() bool { if c == nil { diff --git a/roaring/roaring.go b/roaring/roaring.go index af9ce78a2..d427def14 100644 --- a/roaring/roaring.go +++ b/roaring/roaring.go @@ -3465,8 +3465,11 @@ func (c *Container) bitmapToArray() *Container { } // arrayToBitmap converts from array format to bitmap format. -func (c *Container) arrayToBitmap() *Container { +func (c *Container) arrayToBitmap() (out *Container) { statsHit("arrayToBitmap") + if roaringParanoia { + defer func() { out.CheckN() }() + } if c == nil { if roaringParanoia { panic("nil container for arrayToBitmap") @@ -3498,8 +3501,11 @@ func (c *Container) arrayToBitmap() *Container { } // runToBitmap converts from RLE format to bitmap format. -func (c *Container) runToBitmap() *Container { +func (c *Container) runToBitmap() (out *Container) { statsHit("runToBitmap") + if roaringParanoia { + defer func() { c.CheckN() }() + } if c == nil { if roaringParanoia { panic("nil container for runToBitmap") @@ -3725,6 +3731,9 @@ func (c *Container) runToArray() *Container { // Clone returns a copy of c. func (c *Container) Clone() (out *Container) { + if roaringParanoia { + defer func() { out.CheckN() }() + } statsHit("Container/Clone") if c == nil { return nil @@ -3735,8 +3744,9 @@ func (c *Container) Clone() (out *Container) { out = NewContainerArrayCopy(c.array()) case ContainerBitmap: statsHit("Container/Clone/Bitmap") - other := NewContainerBitmapN(nil, c.N()) + other := NewContainerBitmapN(nil, 0) copy(other.bitmap(), c.bitmap()) + other.n = c.n out = other case ContainerRun: statsHit("Container/Clone/Run") @@ -4098,7 +4108,10 @@ func intersectionCountBitmapBitmap(a, b *Container) (n int32) { return int32(popcountAndSlice(a.bitmap(), b.bitmap())) } -func intersect(a, b *Container) *Container { +func intersect(a, b *Container) (c *Container) { + if roaringParanoia { + defer func() { c.CheckN() }() + } if a.N() == MaxContainerVal+1 { return b.Freeze() } @@ -4320,7 +4333,10 @@ func intersectBitmapBitmap(a, b *Container) *Container { return output } -func union(a, b *Container) *Container { +func union(a, b *Container) (c *Container) { + if roaringParanoia { + defer func() { c.CheckN() }() + } if a.N() == MaxContainerVal+1 || b.N() == MaxContainerVal+1 { return fullContainer } @@ -5029,7 +5045,10 @@ func appendInterval16At(a []Interval16, val Interval16, off int) ([]Interval16, return a, off } -func difference(a, b *Container) *Container { +func difference(a, b *Container) (c *Container) { + if roaringParanoia { + defer func() { c.CheckN() }() + } if a.N() == 0 || b.N() == MaxContainerVal+1 { return nil } @@ -5386,7 +5405,10 @@ func differenceBitmapBitmap(a, b *Container) *Container { return output } -func xor(a, b *Container) *Container { +func xor(a, b *Container) (c *Container) { + if roaringParanoia { + defer func() { c.CheckN() }() + } if a.N() == 0 { return b.Freeze() } diff --git a/roaring/roaring_internal_test.go b/roaring/roaring_internal_test.go index b432f1858..e924b63a6 100644 --- a/roaring/roaring_internal_test.go +++ b/roaring/roaring_internal_test.go @@ -1895,9 +1895,10 @@ func TestWriteReadArray(t *testing.T) { func TestWriteReadBitmap(t *testing.T) { // create bitmap containing > 4096 bits - cb := NewContainerBitmapN(nil, 129*32) + cb := NewContainerBitmapN(nil, 0) for i := 0; i < 129; i++ { cb.bitmap()[i] = 0x5555555555555555 + cb.n += 32 } bb := NewFileBitmap() bb.Containers.Put(0, cb) @@ -1918,9 +1919,10 @@ func TestWriteReadBitmap(t *testing.T) { func TestWriteReadFullBitmap(t *testing.T) { // create bitmap containing > 4096 bits - cb := NewContainerBitmapN(nil, 65536) + cb := NewContainerBitmapN(nil, 0) for i := 0; i < bitmapN; i++ { cb.bitmap()[i] = 0xffffffffffffffff + cb.n += 64 } bb := NewFileBitmap() bb.Containers.Put(0, cb) diff --git a/roaring/roaring_nop_paranoia.go b/roaring/roaring_nop_paranoia.go index 6f2fa354b..d89ade062 100644 --- a/roaring/roaring_nop_paranoia.go +++ b/roaring/roaring_nop_paranoia.go @@ -17,3 +17,10 @@ package roaring const roaringParanoia = false + +// CheckN verifies that a container's cached count is correct, but +// there are two versions; this is the one which doesn't actually +// do anything, because the check is expensive. Which one you get is +// controlled by the roaringparanoia build tag. +func (c *Container) CheckN() { +} diff --git a/roaring/roaring_paranoia.go b/roaring/roaring_paranoia.go index 6f7d0c57e..1ba98ae29 100644 --- a/roaring/roaring_paranoia.go +++ b/roaring/roaring_paranoia.go @@ -16,4 +16,19 @@ package roaring +import "fmt" + const roaringParanoia = true + +// CheckN verifies that the container's cached count is correct. Note +// that this has two definitions, depending on the presence of the +// roaringparanoia build tag. +func (c *Container) CheckN() { + if c == nil { + return + } + count := c.count() + if count != c.n { + panic(fmt.Sprintf("CheckN (%p): n %d, count %d", c, c.n, count)) + } +}