Merge pull request #818 from seebs/roaringDoc

Roaring documentation updates and fixes resulting from them

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

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)
 }

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 {