From ecacbf65d484039aaba2aa99850f01bb6d64fe2a Mon Sep 17 00:00:00 2001 From: Seebs Date: Tue, 8 Sep 2020 11:44:56 -0500 Subject: [PATCH 1/3] Document copy-on-write semantics, at all. The copy-on-write semantics were previously documented only in the 125-line commit log from the patch which introduced them. Add documentation for them in a few likely places. --- roaring/container_stash.go | 24 +++++++++++++++++++++++- roaring/roaring.go | 15 +++++++++++++++ 2 files changed, 38 insertions(+), 1 deletion(-) diff --git a/roaring/container_stash.go b/roaring/container_stash.go index 6cdd53287..e18725d6f 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 @@ -264,7 +284,9 @@ func (c *Container) setMapped(mapped bool) { // 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 diff --git a/roaring/roaring.go b/roaring/roaring.go index d6dc93362..88733db2e 100644 --- a/roaring/roaring.go +++ b/roaring/roaring.go @@ -6876,9 +6876,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 +6895,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 +6917,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) } From 17ba2e35a902a9dae568bd44df49d53bc35c528c Mon Sep 17 00:00:00 2001 From: Seebs Date: Tue, 8 Sep 2020 12:21:01 -0500 Subject: [PATCH 2/3] call helper functions every time to get new run slices If you just stash the results of the function when defining the test cases, the outcome is in part that you are reusing the same slices for multiple things. So, for instance, if you perform a union on the OddBitsSet slice, with the EvenBitsSet slice, the result is to overwrite the first entry in that slice with the 0-ffff run... But the original slice still exists, and then we reuse it and get a slice with a bit count of around 98,000. The underlying issue is that doContainer() is calling NewContainerRun(), which is simply using the provided slice, not copying it -- which is intentional, but the test has to be careful about it. We call repair on the one we think should be a bitmap. Theoretically maybe we should also repair the other one in case unionRunRun some day starts returning unrepaired bitmaps, which in principle it's allowed to do... --- roaring/roaring_internal_test.go | 26 ++++++++++++++------------ 1 file changed, 14 insertions(+), 12 deletions(-) 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 { From c079d4764befce5927fa1a4d3cad49171a716c20 Mon Sep 17 00:00:00 2001 From: Seebs Date: Tue, 8 Sep 2020 13:16:54 -0500 Subject: [PATCH 3/3] Check for possibly-dirty N values in containers modified in-place After documenting the semantics, I noticed an arguable hole in them, which is that you could Freeze() a dirty container, and then Repair() wouldn't work on it. On further study, I added a roaringparanoia check for attempts to access the N of dirty containers. It turns out there's several such. But also, it turns out, there's circumstances where unionInPlace is relying on the assumption that N is valid, which it isn't always for dirty containers. Also, there's at least one case where we rely on the assumption that forcibly thawing a container, then calling unionInPlace on it, always modifies that container. But that's not supposed to be true for an empty container -- an empty container might be better handled by just returning the container it's being unioned with. So, we drop the unnecessary thaw (all the *InPlace ops are already thawing if/when they need to), but we use the return from unionInPlace. --- roaring/container_stash.go | 59 +++++++++++++++++++++++++++++++++++--- roaring/roaring.go | 51 +++++++++++++++++++++----------- 2 files changed, 90 insertions(+), 20 deletions(-) diff --git a/roaring/container_stash.go b/roaring/container_stash.go index e18725d6f..e08f823a6 100644 --- a/roaring/container_stash.go +++ b/roaring/container_stash.go @@ -74,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 { @@ -232,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 } @@ -281,6 +309,21 @@ 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 @@ -291,6 +334,14 @@ 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 88733db2e..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 }