diff --git a/roaring/container_stash.go b/roaring/container_stash.go index 6cdd53287..e08f823a6 100644 --- a/roaring/container_stash.go +++ b/roaring/container_stash.go @@ -34,6 +34,26 @@ const ( // less than 4,096 values, an array is often used. Containers with long runs of // integers would use run length encoding, and more random data usually uses // bitmap encoding. +// +// The Container type has somewhat magical semantics. Containers can be marked +// as "frozen" by the Freeze method, after which, nothing should ever modify +// that specific container object again, no matter what. Because of this, but +// also sometimes for Even More Esoteric Reasons, *no* container method should +// ever be assumed to be genuinely modifying the container it was called on, +// and *every* container method that might modify a container should return +// the "modified" *Container, which *may point to a different object*. The +// caller should always use this resulting container, and if you're storing +// a *Container in a data structure, you need to update the data structure's +// pointer too. +// +// A nil *Container is a valid empty container. +// +// In general, operations on containers which produce new containers *may* +// yield new containers, and *may* yield their operands. +// +// The reason for all of this is to allow containers to have copy-on-write +// semantics, which allow us to reduce memory usage dramatically, and GC +// load even more dramatically. type Container struct { pointer *uint16 // the data pointer len, cap int32 // length and cap @@ -54,16 +74,25 @@ var containerFlagStrings = [...]string{ "pristine/mapped", "pristine/frozen", "pristine/frozen/mapped", + "dirty", + "mapped/dirty", + "frozen/dirty", + "frozen/mapped/dirty", + "pristine/dirty", + "pristine/mapped/dirty", + "pristine/frozen/dirty", + "pristine/frozen/mapped/dirty", } func (f containerFlags) String() string { - return containerFlagStrings[f&7] + return containerFlagStrings[f&15] } const ( - flagMapped = containerFlags(1 << iota) - flagFrozen - flagPristine + flagMapped = containerFlags(1 << iota) // using memory-mapped or otherwise external storage + flagFrozen // not modifiable + flagPristine // flagPristine is used for mmapped containers referring to storage + flagDirty // flagDirty is used for containers which may have invalid N ) func (c *Container) String() string { @@ -212,11 +241,30 @@ func (c *Container) frozen() bool { return (c.flags & flagFrozen) != 0 } +// SafeN returns N, true if it can, otherwise it returns 0, false. For +// instance, a container subject to in-place operations can not know its +// current N, and it's not meaningful or safe to query it until a repair, +// so you can use this to get N "if it's available". +func (c *Container) SafeN() (int32, bool) { + if c == nil { + return 0, true + } + if (c.flags & flagDirty) != 0 { + return 0, false + } + return c.n, true +} + // N returns the 1-count of the container. func (c *Container) N() int32 { if c == nil { return 0 } + if roaringParanoia { + if c.flags&flagDirty != 0 { + panic("trying to call N() on a dirty container") + } + } return c.n } @@ -261,14 +309,39 @@ func (c *Container) setMapped(mapped bool) { } } +// setDirty marks a container as "dirty" -- we don't trust container's n. +// this should never happen except for bitmaps. +func (c *Container) setDirty(dirty bool) { + if roaringParanoia { + if c == nil || c.frozen() { + panic("setDirty on nil or frozen container") + } + } + if dirty { + c.flags |= flagDirty + } else { + c.flags &^= flagDirty + } +} + // Freeze returns an unmodifiable container identical to c. This might // be c, now marked unmodifiable, or might be a new container. If c // is currently marked as "mapped", referring to a backing store that's -// not a conventional Go pointer, the storage may be copied. +// not a conventional Go pointer, the storage may (or may not) be copied. +// Do not call Freeze on a temporarily-corrupt container, such as one +// returned from UnionInPlace but on which you haven't since called Repair. func (c *Container) Freeze() *Container { if c == nil { return nil } + if c.flags&flagDirty != 0 { + if roaringParanoia { + panic("freezing dirty container") + } + // c.Repair won't work if this is already frozen, but in + // theory that can't happen? + c.Repair() + } // don't need to freeze if c.flags&flagFrozen != 0 { return c diff --git a/roaring/roaring.go b/roaring/roaring.go index d6dc93362..eabec3b86 100644 --- a/roaring/roaring.go +++ b/roaring/roaring.go @@ -1366,11 +1366,12 @@ func (b *Bitmap) unionInPlace(others ...*Bitmap) { tContainer := target.Containers.Get(iKey) // if the target's full, short-circuit out. if tContainer != nil { - if tContainer.N() == MaxContainerVal+1 { + tN, ok := tContainer.SafeN() + if ok && tN == MaxContainerVal+1 { bitmapIters.markItersWithKeyAsHandled(i, iKey) continue } - expectedN = int64(tContainer.N()) + expectedN = int64(tN) } // Check i and later iters for any max-range containers, and // find out how many there are. @@ -1445,8 +1446,7 @@ func (b *Bitmap) unionInPlace(others ...*Bitmap) { jKey, jContainer := iter.iter.Value() if iKey == jKey { - tContainer = tContainer.Thaw() - tContainer.unionInPlace(jContainer) + tContainer = tContainer.unionInPlace(jContainer) // "iter" is a local copy from the range // loop, not the actual slice member. itersToUnion[j].handled = true @@ -3191,15 +3191,24 @@ func (c *Container) optimize() *Container { // it is possible that the returned container will not actually be the // original container; in-place is a suggestion. func (c *Container) unionInPlace(other *Container) *Container { - if c == nil { - return other.Freeze() - } - if other == nil { - return c - } // short-circuit the trivial cases - if c.N() == MaxContainerVal+1 || other.N() == MaxContainerVal+1 { - return fullContainer + cN, cOk := c.SafeN() + if cOk { + if cN == MaxContainerVal+1 { + return fullContainer + } + if cN == 0 { + return other.Clone() + } + } + oN, oOk := other.SafeN() + if oOk { + if oN == MaxContainerVal+1 { + return fullContainer + } + if oN == 0 { + return c + } } switch c.typ() { case ContainerBitmap: @@ -3487,6 +3496,11 @@ func (c *Container) runToBitmap() *Container { } return nil } + if roaringParanoia { + if c.N() > 65536 { + panic(fmt.Sprintf("runToBitmap: container N %d", c.N())) + } + } // return early if empty if c.N() == 0 { @@ -3864,6 +3878,7 @@ func (c *Container) Repair() { } if c.isBitmap() { c.bitmapRepair() + c.setDirty(false) } } @@ -4536,6 +4551,7 @@ func unionBitmapRun(a, b *Container) *Container { // a will need to be repaired after the fact. func unionBitmapRunInPlace(a, b *Container) *Container { a = a.Thaw() + a.setDirty(true) bitmap := a.bitmap() statsHit("union/BitmapRun") for _, run := range b.runs() { @@ -4708,10 +4724,11 @@ func compareArrayArray(a1, a2 []uint16) error { // 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") + cn, c2n := c.N(), c2.N() + if cn != c2n { + return fmt.Errorf("containers are different lengths (%d vs %d)", cn, c2n) } - if c.N() == 0 { + if cn == 0 { return nil } switch typePair(c.typ(), c2.typ()) { @@ -4728,7 +4745,7 @@ func (c *Container) BitwiseCompare(c2 *Container) error { default: c3 := xor(c, c2) if c3.N() != 0 { - return fmt.Errorf("%d bits differenct between containers", c3.N()) + return fmt.Errorf("%d bits different between containers", c3.N()) } } return nil @@ -4753,6 +4770,7 @@ func unionArrayBitmap(a, b *Container) *Container { func unionBitmapArrayInPlace(a, b *Container) *Container { a = a.Thaw() bitmap := a.bitmap() + a.setDirty(true) for _, v := range b.array() { bitmap[v>>6] |= (uint64(1) << (v % 64)) } @@ -4800,6 +4818,7 @@ func unionBitmapBitmapInPlace(a, b *Container) *Container { ab[i+2] |= bb[i+2] ab[i+3] |= bb[i+3] } + a.setDirty(true) return a } @@ -6876,9 +6895,14 @@ func ConvertRunToBitmap(c *Container) *Container { return c.runToBitmap() } +// Optimize yields a container with the same bits as c, but +// adjusted to the smallest-storage type by Roaring rules (thus, +// runs where that's smaller, otherwise arrays for N < 4096 and +// bitmaps for N >= 4096). func Optimize(c *Container) *Container { return c.optimize() } + func Union(a, b *Container) (c *Container) { c = union(a, b) // c can be have arrays that are too big, and need @@ -6890,10 +6914,16 @@ func Difference(a, b *Container) *Container { return difference(a, b) } +// Add yields a container identical to c, but with the given bit set; added +// is true if the bit wasn't previously set. It is unspecified whether +// the original container is modified. func (c *Container) Add(v uint16) (newC *Container, added bool) { return c.add(v) } +// Add yields a container identical to c, but with the given bit cleared; +// removed is true if the bit was previously set. It is unspecified whether +// the original container is modified. func (c *Container) Remove(v uint16) (c2 *Container, removed bool) { return c.remove(v) } @@ -6906,6 +6936,10 @@ func (c *Container) CountRange(start, end int32) (n int32) { return c.countRange(start, end) } +// UnionInPlace yields a container containing all the bits set in either +// c or other. It may, or may not, modify c. The resulting container's +// count, as returned by c.N(), may be incorrect; see (*Container).Repair(). +// Do not freeze a container produced by this operation before repairing it. func (c *Container) UnionInPlace(other *Container) (r *Container) { return c.unionInPlace(other) } diff --git a/roaring/roaring_internal_test.go b/roaring/roaring_internal_test.go index b10964cb5..c75df2140 100644 --- a/roaring/roaring_internal_test.go +++ b/roaring/roaring_internal_test.go @@ -4375,26 +4375,28 @@ func BenchmarkUnionRunRunInPlace(bm *testing.B) { func TestUnionRunRunInPlaceBitwiseCompare(t *testing.T) { runs := []struct { name string - run []Interval16 + fn func() []Interval16 }{ - {name: "FirstBitSet", run: runFirstBitSet()}, - {name: "LastBitSet", run: runLastBitSet()}, - {name: "FirstBitUnset", run: runFirstBitUnset()}, - {name: "LastBitUnset", run: runLastBitUnset()}, - {name: "InnerBitsSet", run: runInnerBitsSet()}, - {name: "OuterBitsSet", run: runOuterBitsSet()}, - {name: "OddBitsSet", run: runOddBitsSet()}, - {name: "EvenBitsSet", run: runEvenBitsSet()}, + {name: "FirstBitSet", fn: runFirstBitSet}, + {name: "LastBitSet", fn: runLastBitSet}, + {name: "FirstBitUnset", fn: runFirstBitUnset}, + {name: "LastBitUnset", fn: runLastBitUnset}, + {name: "InnerBitsSet", fn: runInnerBitsSet}, + {name: "OuterBitsSet", fn: runOuterBitsSet}, + {name: "OddBitsSet", fn: runOddBitsSet}, + {name: "EvenBitsSet", fn: runEvenBitsSet}, } for _, a := range runs { for _, b := range runs { t.Run(a.name+"-"+b.name, func(t *testing.T) { - arun := doContainer(ContainerRun, a.run) - brun := doContainer(ContainerRun, b.run) + arun := doContainer(ContainerRun, a.fn()) + abm := doContainer(ContainerRun, a.fn()).runToBitmap() + brun := doContainer(ContainerRun, b.fn()) - out1 := unionBitmapRunInPlace(arun.runToBitmap(), brun) + out1 := unionBitmapRunInPlace(abm, brun) out2 := unionRunRunInPlace(arun, brun) + out1.Repair() err := out1.BitwiseCompare(out2.runToBitmap()) if err != nil {