From 3e0ce32d006f5a1a1102327443334d4313bb28ba Mon Sep 17 00:00:00 2001 From: Jaden Weiss Date: Tue, 30 Jun 2020 16:38:16 -0400 Subject: [PATCH] roaring cleanup --- roaring/container_stash.go | 131 +++++++++++++----------------------- roaring/containers_btree.go | 26 ------- roaring/containers_slice.go | 23 ------- roaring/roaring.go | 62 ++++++----------- roaring/unmarshal_binary.go | 55 ++++++++------- 5 files changed, 98 insertions(+), 199 deletions(-) diff --git a/roaring/container_stash.go b/roaring/container_stash.go index a43b3eee8..0f616f0f2 100644 --- a/roaring/container_stash.go +++ b/roaring/container_stash.go @@ -79,8 +79,8 @@ func (c *Container) String() string { case containerArray: return fmt.Sprintf("<%s%sarray container, N=%d>", froze, space, c.N()) case containerBitmap: - return fmt.Sprintf("<%s%sbitmap container, N=%d, len %dx uint64>", - froze, space, c.N(), len(c.bitmap())) + return fmt.Sprintf("<%s%sbitmap container, N=%d>", + froze, space, c.N()) case containerRun: return fmt.Sprintf("<%s%srun container, N=%d, len %dx interval>", froze, space, c.N(), len(c.runs())) @@ -93,9 +93,7 @@ func (c *Container) String() string { // may later become more interesting. func NewContainer() *Container { statsHit("NewContainer") - c := &Container{typeID: containerArray, len: 0, cap: stashedArraySize} - c.pointer = (*uint16)(unsafe.Pointer(&c.data[0])) - return c + return NewContainerArray(nil) } // NewContainerBitmap makes a bitmap container using the provided bitmap, or @@ -107,14 +105,13 @@ func NewContainerBitmap(n int, bitmap []uint64) *Container { if bitmap == nil { return NewContainerBitmapN(nil, 0) } - // pad to required length - if len(bitmap) < bitmapN { - bm2 := make([]uint64, bitmapN) - copy(bm2, bitmap) - bitmap = bm2 - } c := &Container{typeID: containerBitmap} - c.setBitmap(bitmap) + if len(bitmap) != bitmapN { + // adjust to required length + c.setBitmapCopy(bitmap) + } else { + c.setBitmap(bitmap) + } // set n based on bitmap contents. if n < 0 { c.bitmapRepair() @@ -131,14 +128,13 @@ func NewContainerBitmapN(bitmap []uint64, n int32) *Container { if bitmap == nil { bitmap = make([]uint64, bitmapN) } - // pad to required length - if len(bitmap) < bitmapN { - var bm [bitmapN]uint64 - copy(bm[:], bitmap) - bitmap = bm[:] - } c := &Container{typeID: containerBitmap, n: n} - c.setBitmap(bitmap) + if len(bitmap) != bitmapN { + // adjust to required length + c.setBitmapCopy(bitmap) + } else { + c.setBitmap(bitmap) + } return c } @@ -164,9 +160,7 @@ func NewContainerArrayCopy(set []uint16) *Container { // This is deprecated. It never worked in the first place. // The provided value of n is ignored and instead derived from the set length. func NewContainerArrayN(set []uint16, n int32) *Container { - c := &Container{typeID: containerArray} - c.setArray(set) - return c + return NewContainerArray(set) } // NewContainerRun creates a new run container using a provided (possibly nil) @@ -286,10 +280,8 @@ func (c *Container) Freeze() *Container { // Thaw returns a modifiable container identical to c. This may be c, or it // may be a new container with distinct backing store. func (c *Container) Thaw() *Container { - if roaringParanoia { - if c == nil { - panic("trying to thaw a nil container") - } + if c == nil { + panic("trying to thaw a nil container") } if c.flags&(flagFrozen|flagMapped) == 0 { return c @@ -308,45 +300,11 @@ func (c *Container) unmapOrClone() *Container { // mapped: we want to unmap the storage. switch c.typeID { case containerArray: - // mapped flag is wrong here - if c.pointer == &c.data[0] { - return c - } - // maybe it fits in storage - if c.len <= int32(len(c.data)) { - copy(c.data[:], c.array()) - c.pointer, c.cap = &c.data[0], stashedArraySize - return c - } - arr := make([]uint16, c.len) - copy(arr, c.array()) - if cap(arr) > 1<<16 { - arr = arr[: len(arr) : 1<<16] - } - c.pointer, c.cap = &arr[0], int32(cap(arr)) + c.setArrayMaybeCopy(c.array(), true) case containerRun: - // mapped flag is wrong here - if c.pointer == &c.data[0] { - return c - } - oldRuns := c.runs() - // maybe it fits in storage - if c.len <= stashedRunSize { - c.pointer, c.cap = &c.data[0], stashedRunSize - copy(c.runs(), oldRuns) - return c - } - runs := make([]interval16, c.len) - copy(runs, oldRuns) - if cap(runs) > 1<<15 { - runs = runs[: len(runs) : 1<<15] - } - c.pointer, c.cap = &runs[0].start, int32(cap(runs)) + c.setRunsMaybeCopy(c.runs(), true) case containerBitmap: - oldBitmap := c.bitmap() - var bitmap [1024]uint64 - copy(bitmap[:], oldBitmap) - c.pointer, c.len, c.cap = (*uint16)(unsafe.Pointer(&bitmap)), bitmapN, bitmapN + c.setBitmapCopy(c.bitmap()) default: panic(fmt.Sprintf("can't thaw invalid container, type %d", c.typeID)) } @@ -355,10 +313,10 @@ func (c *Container) unmapOrClone() *Container { // array yields the data viewed as a slice of uint16 values. func (c *Container) array() []uint16 { + if c == nil { + panic("attempt to read a nil container's array") + } if roaringParanoia { - if c == nil { - panic("attempt to read a nil container's array") - } if c.typeID != containerArray { panic("attempt to read non-array's array") } @@ -390,7 +348,7 @@ func (c *Container) setArrayMaybeCopy(array []uint16, doCopy bool) { c.flags &^= flagMapped // this is no longer using a hypothetical mmapped input array return } - if &array[0] == c.pointer { + if &array[0] == c.pointer && !doCopy { // nothing to do but update length c.len = int32(len(array)) c.n = c.len @@ -414,10 +372,10 @@ func (c *Container) setArray(array []uint16) { // bitmap yields the data viewed as a slice of uint64s holding bits. func (c *Container) bitmap() []uint64 { + if c == nil { + panic("attempt to read nil container's bitmap") + } if roaringParanoia { - if c == nil { - panic("attempt to read nil container's bitmap") - } if c.typeID != containerBitmap { panic("attempt to read non-bitmap's bitmap") } @@ -497,6 +455,13 @@ func splatRun(into *[1024]uint64, from interval16) { copy(into[fillStart:fillEnd+1], fillerBitmap[:]) } +// setBitmapCopy stores a copy of a bitmap as data. +func (c *Container) setBitmapCopy(bitmap []uint64) { + var bitmapCopy [bitmapN]uint64 + copy(bitmapCopy[:], bitmap) + c.setBitmap(bitmapCopy[:]) +} + // setBitmap stores a set of uint64s as data. func (c *Container) setBitmap(bitmap []uint64) { if c == nil || c.frozen() { @@ -516,10 +481,10 @@ func (c *Container) setBitmap(bitmap []uint64) { // runs yields the data viewed as a slice of intervals. func (c *Container) runs() []interval16 { + if c == nil { + panic("attempt to read nil container's runs") + } if roaringParanoia { - if c == nil { - panic("attempt to read nil container's runs") - } if c.typeID != containerRun { panic("attempt to read non-run's runs") } @@ -555,7 +520,7 @@ func (c *Container) setRunsMaybeCopy(runs []interval16, doCopy bool) { c.flags &^= flagMapped // this is no longer using a hypothetical mmapped input array return } - if &runs[0].start == c.pointer { + if &runs[0].start == c.pointer && !doCopy { // nothing to do but update length c.len = int32(len(runs)) return @@ -626,30 +591,24 @@ func (c *Container) Update(typ byte, n int32, mapped bool) { // isArray returns true if the container is an array container. func (c *Container) isArray() bool { - if roaringParanoia { - if c == nil { - panic("calling isArray on nil container") - } + if c == nil { + panic("calling isArray on nil container") } return c.typeID == containerArray } // isBitmap returns true if the container is a bitmap container. func (c *Container) isBitmap() bool { - if roaringParanoia { - if c == nil { - panic("calling isBitmap on nil container") - } + if c == nil { + panic("calling isBitmap on nil container") } return c.typeID == containerBitmap } // isRun returns true if the container is a run-length-encoded container. func (c *Container) isRun() bool { - if roaringParanoia { - if c == nil { - panic("calling isRun on nil container") - } + if c == nil { + panic("calling isRun on nil container") } return c.typeID == containerRun } diff --git a/roaring/containers_btree.go b/roaring/containers_btree.go index 280fd5edc..257a756fa 100644 --- a/roaring/containers_btree.go +++ b/roaring/containers_btree.go @@ -68,32 +68,6 @@ func (btc *bTreeContainers) Put(key uint64, c *Container) { btc.tree.Set(key, c) } -func (u updater) update(oldV *Container, exists bool) (*Container, bool) { - // update the existing container - if exists { - oldV = oldV.UpdateOrMake(u.typ, u.n, u.mapped) - return oldV, true - } - cont := NewContainer() - cont.setTyp(u.typ) - cont.setN(u.n) - cont.setMapped(u.mapped) - return cont, true -} - -// this struct is added to prevent the closure locals from being escaped out to the heap -type updater struct { - key uint64 - n int32 - typ byte - mapped bool -} - -func (btc *bTreeContainers) PutContainerValues(key uint64, typ byte, n int, mapped bool) { - a := updater{key, int32(n), typ, mapped} - btc.tree.Put(key, a.update) -} - func (btc *bTreeContainers) Remove(key uint64) { btc.tree.Delete(key) if key == btc.lastKey { diff --git a/roaring/containers_slice.go b/roaring/containers_slice.go index 98fe91fd3..1eaa65cf0 100644 --- a/roaring/containers_slice.go +++ b/roaring/containers_slice.go @@ -47,29 +47,6 @@ func (sc *sliceContainers) Put(key uint64, c *Container) { sc.lastContainer = c } -func (sc *sliceContainers) PutContainerValues(key uint64, typ byte, n int, mapped bool) { - i := search64(sc.keys, key) - if i < 0 { - c := NewContainer() - c.setTyp(typ) - c.setN(int32(n)) - c.setMapped(mapped) - sc.insertAt(key, c, -i-1) - } else { - // if the container already exists, and is frozen, this may - // result in copying its data, which is sort of pointless - // because PutContainerValues almost always gets called - // because we're reading new data from a file -- but also - // that means this case probably never happens. - c := sc.containers[i].Thaw() - c.setTyp(typ) - c.setN(int32(n)) - c.setMapped(mapped) - sc.containers[i] = c - } - -} - func (sc *sliceContainers) Remove(key uint64) { statsHit("sliceContainers/Remove") i := search64(sc.keys, key) diff --git a/roaring/roaring.go b/roaring/roaring.go index b87f4d6af..24e94221b 100644 --- a/roaring/roaring.go +++ b/roaring/roaring.go @@ -21,7 +21,6 @@ import ( "hash/fnv" "io" "math/bits" - "reflect" "sort" "unsafe" @@ -125,11 +124,6 @@ type Containers interface { // Put adds the container at key. Put(key uint64, c *Container) - // PutContainerValues updates an existing container at key. - // If a container does not exist for key, a new one is allocated. - // TODO(2.0) make n int32 - PutContainerValues(key uint64, typ byte, n int, mapped bool) - // Remove takes the container at key out. Remove(key uint64) @@ -236,8 +230,7 @@ func NewSliceBitmap(a ...uint64) *Bitmap { } // NewFileBitmap returns a Bitmap with an initial set of values, used for file storage. -// By default, this is a copy of NewBitmap, but is replaced with B+Tree in server/enterprise.go -var NewFileBitmap func(a ...uint64) *Bitmap = NewBTreeBitmap +var NewFileBitmap = NewBTreeBitmap // Clone returns a heap allocated copy of the bitmap. // Note: The OpWriter IS NOT copied to the new bitmap. @@ -296,6 +289,7 @@ func (b *Bitmap) Add(a ...uint64) (changed bool, err error) { // AddN adds values to the bitmap, appending them all to the op log in a batched // write. It returns the number of changed bits. +// The input slice may be reordered, and the set of changed bits will end up in a[:changed]. func (b *Bitmap) AddN(a ...uint64) (changed int, err error) { if len(a) == 0 { return 0, nil @@ -468,7 +462,7 @@ func (b *Bitmap) Count() (n uint64) { return b.Containers.Count() } -// Any returns "b.Count() > 0"... but faster than doing that. +// Any checks whether there are any set bits within the bitmap. func (b *Bitmap) Any() bool { iter, _ := b.Containers.Iterator(0) // TODO (jaffee) I'm not sure if it's possible/legal to have an empty @@ -872,10 +866,7 @@ func (c *Container) intersectInPlace(other *Container) *Container { } } - if roaringParanoia { - panic(fmt.Sprintf("invalid intersect op: unknown types %d/%d", c.typ(), other.typ())) - } - return nil + panic(fmt.Errorf("invalid intersect op: unknown types %d/%d", c.typ(), other.typ())) } func (c *Container) copyInPlace(other *Container) *Container { @@ -883,19 +874,19 @@ func (c *Container) copyInPlace(other *Container) *Container { case containerArray: c.setTyp(containerArray) c.setArrayMaybeCopy(other.array(), true) - c.setN(other.N()) case containerBitmap: - bmp := make([]uint64, bitmapN) - copy(bmp, other.bitmap()) c.setTyp(containerBitmap) - c.setBitmap(bmp) + c.setBitmapCopy(other.bitmap()) c.setN(other.N()) case containerRun: c.setTyp(containerRun) c.setRunsMaybeCopy(other.runs(), true) c.setN(other.N()) + + default: + panic(fmt.Errorf("invalid container type: %v", c.typ())) } return c @@ -987,6 +978,9 @@ func intersectBitmapBitmapInPlace(a, b *Container) *Container { n := int32(0) for i := 0; i < bitmapN; i += 4 { // unrolling is still effective in go + // TODO: the generated machine code is extremely bad here, because we are forcing the compiler to reload ab immediately after storing it + // The body of the loop has a total of 8 branches: 4 bounds checks + 4 feature checks. + // We could substantially improve this by converting the entire bitmap to [256][4]uint64. ptr := (*[4]uint64)(unsafe.Pointer(&bb[i])) ab[i] &= ptr[0] ab[i+1] &= ptr[1] @@ -1024,7 +1018,6 @@ func intersectBitmapArrayInPlace(a, b *Container) *Container { array = array[:n] a.setTyp(containerArray) a.setArray(array) - a.setN(n) return a } @@ -2397,20 +2390,16 @@ func BitmapsToRoaring(bitmaps []*Bitmap) []byte { binary.LittleEndian.PutUint16(header[10:12], uint16(n-1)) binary.LittleEndian.PutUint32(offset[0:4], uint32(dataOffset+int(offsetEnd))) nextData := data[dataOffset:] - switch c.typeID { + switch c.typeID { // TODO: make this work on big endian machines case containerArray: - asUint16 := *(*[]uint16)(unsafe.Pointer(&reflect.SliceHeader{Data: uintptr(unsafe.Pointer(&nextData[0])), Len: int(c.len), Cap: int(c.len)})) - copy(asUint16, c.array()) - dataOffset += 2 * int(c.len) + dataOffset += 2 * copy((*[1 << 16]uint16)(unsafe.Pointer(&nextData[0]))[:], c.array()) case containerBitmap: - asUint64 := *(*[]uint64)(unsafe.Pointer(&reflect.SliceHeader{Data: uintptr(unsafe.Pointer(&nextData[0])), Len: 1024, Cap: 1024})) - copy(asUint64, c.bitmap()) + copy((*[1024]uint64)(unsafe.Pointer(&nextData[0]))[:], c.bitmap()) dataOffset += 8192 case containerRun: - asInterval16 := *(*[]interval16)(unsafe.Pointer(&reflect.SliceHeader{Data: uintptr(unsafe.Pointer(&nextData[2])), Len: int(c.len), Cap: int(c.len)})) - copy(asInterval16, c.runs()) binary.LittleEndian.PutUint16(nextData[0:2], uint16(c.len)) - dataOffset += int(4*c.len) + 2 + dataOffset += 2 + dataOffset += 4 * copy((*[1 << 15]interval16)(unsafe.Pointer(&nextData[2]))[:], c.runs()) } } } @@ -3156,10 +3145,7 @@ func (c *Container) unionInPlace(other *Container) *Container { return unionBitmapRunInPlace(c, other) } } - if roaringParanoia { - panic(fmt.Sprintf("invalid union op: unknown types %d/%d", c.typ(), other.typ())) - } - return c + panic(fmt.Errorf("invalid union op: unknown types %d/%d", c.typ(), other.typ())) } func (c *Container) arrayContains(v uint16) bool { @@ -5455,10 +5441,8 @@ func (op *op) size() int { case opTypeAddRoaring, opTypeRemoveRoaring: return 1 + 8 + 4 + 4 + len(op.roaring) } - if roaringParanoia { - panic(fmt.Sprintf("op size() called on unknown op type %d", op.typ)) - } - return 0 + + panic(fmt.Errorf("op size() called on unknown op type %d", op.typ)) } // size returns the size needed to encode the op, in bytes. for @@ -5474,10 +5458,8 @@ func (op *op) encodeSize() int { case opTypeAddRoaring, opTypeRemoveRoaring: return 1 + 8 + 4 + 4 } - if roaringParanoia { - panic(fmt.Sprintf("op encodeSize() called on unknown op type %d", op.typ)) - } - return 0 + + panic(fmt.Errorf("op encodeSize() called on unknown op type %d", op.typ)) } // count returns the number of bits the operation mutates. @@ -5490,7 +5472,7 @@ func (op *op) count() int { case 4, 5: return op.opN default: - panic(fmt.Sprintf("unknown operation type: %d", op.typ)) + panic(fmt.Errorf("unknown operation type: %d", op.typ)) } } diff --git a/roaring/unmarshal_binary.go b/roaring/unmarshal_binary.go index aeab0ef2f..f95f2a41f 100644 --- a/roaring/unmarshal_binary.go +++ b/roaring/unmarshal_binary.go @@ -47,14 +47,18 @@ func (b *Bitmap) UnmarshalBinary(data []byte) (err error) { itrKey, itrCType, itrN, itrLen, itrPointer, itrErr = itr.Next() for itrErr == nil { - newC := &Container{ - typeID: itrCType, - n: int32(itrN), - len: int32(itrLen), - cap: int32(itrLen), - pointer: itrPointer, - flags: flagMapped, + var newC *Container + switch itrCType { + case containerArray: + newC = NewContainerArray((*[4096]uint16)(unsafe.Pointer(itrPointer))[:itrLen:itrLen]) + case containerRun: + newC = NewContainerRunN((*[2048]interval16)(unsafe.Pointer(itrPointer))[:itrLen:itrLen], int32(itrN)) + case containerBitmap: + newC = NewContainerBitmapN((*[1024]uint64)(unsafe.Pointer(itrPointer))[:1024:itrLen], int32(itrN)) + default: + panic("invalid container type") } + newC.setMapped(true) if !b.preferMapping { newC.unmapOrClone() } @@ -126,14 +130,27 @@ func InspectBinary(data []byte, mapped bool, info *BitmapInfo) (b *Bitmap, mappe itrKey, itrCType, itrN, itrLen, itrPointer, itrErr = itr.Next() for itrErr == nil { - newC := &Container{ - typeID: itrCType, - n: int32(itrN), - len: int32(itrLen), - cap: int32(itrLen), - pointer: itrPointer, - flags: flagMapped, + var size int + switch itrCType { + case containerArray: + size = int(itrN) * 2 + case containerBitmap: + size = 8192 + case containerRun: + size = itrLen*interval16Size + runCountHeaderSize } + var newC *Container + switch itrCType { + case containerArray: + newC = NewContainerArray((*[4096]uint16)(unsafe.Pointer(itrPointer))[:itrLen:itrLen]) + case containerRun: + newC = NewContainerRunN((*[2048]interval16)(unsafe.Pointer(itrPointer))[:itrLen:itrLen], int32(itrN)) + case containerBitmap: + newC = NewContainerBitmapN((*[1024]uint64)(unsafe.Pointer(itrPointer))[:1024:itrLen], int32(itrN)) + default: + panic("invalid container type") + } + newC.setMapped(true) if !mapped { newC.unmapOrClone() } @@ -141,16 +158,6 @@ func InspectBinary(data []byte, mapped bool, info *BitmapInfo) (b *Bitmap, mappe if newC.flags&flagMapped != 0 { mappedAny = true } - var size int - b.Containers.Put(itrKey, newC) - switch itrCType { - case containerArray: - size = int(newC.n) * 2 - case containerBitmap: - size = 8192 - case containerRun: - size = itrLen*interval16Size + runCountHeaderSize - } info.Containers = append(info.Containers, ContainerInfo{ N: newC.n, Mapped: newC.flags&flagMapped != 0,