From 269837414e289e77231a0de57d0b5e274f6902b1 Mon Sep 17 00:00:00 2001 From: Seebs Date: Mon, 12 Apr 2021 16:13:16 -0500 Subject: [PATCH 1/2] rbf/intoContainer: ensure correct N, avoid recounting The remake container logic (used to avoid allocating extra containers while applying filters) relied on roaring recomputing N, which it did for bitmaps but didn't do for runs. Fix this both ways; it would now do that for runs, but also we add "with explicit N" variants and use those since we have a correct count already, and don't need it. This means fewer popcounts on bitmaps, and working at all on runs. --- rbf/cursorx.go | 4 ++-- roaring/container_stash.go | 28 ++++++++++++++++++++++++++++ 2 files changed, 30 insertions(+), 2 deletions(-) diff --git a/rbf/cursorx.go b/rbf/cursorx.go index 0ca291ff1..f1c8f338a 100644 --- a/rbf/cursorx.go +++ b/rbf/cursorx.go @@ -197,9 +197,9 @@ func intoContainer(l leafCell, tx *Tx, replacing *roaring.Container, target []by } c = roaring.RemakeContainerBitmap(replacing, cloneMaybe) case ContainerTypeBitmap: - c = roaring.RemakeContainerBitmap(replacing, toArray64(cpMaybe)) + c = roaring.RemakeContainerBitmapN(replacing, toArray64(cpMaybe), int32(l.BitN)) case ContainerTypeRLE: - c = roaring.RemakeContainerRun(replacing, toInterval16(cpMaybe)) + c = roaring.RemakeContainerRunN(replacing, toInterval16(cpMaybe), int32(l.BitN)) } // Note: If the "roaringparanoia" build tag isn't set, this // should be optimized away entirely. Otherwise it's moderately diff --git a/roaring/container_stash.go b/roaring/container_stash.go index 2cde05743..7dd6eae83 100644 --- a/roaring/container_stash.go +++ b/roaring/container_stash.go @@ -125,6 +125,8 @@ func NewContainer() *Container { return NewContainerArray(nil) } +// RemakeContainerBitmap overwrites the contents of c, which must not be +// frozen, with a provided bitmap, and computes a correct N. func RemakeContainerBitmap(c *Container, bitmap []uint64) *Container { *c = Container{typeID: ContainerBitmap} c.setBitmap(bitmap) @@ -132,15 +134,41 @@ func RemakeContainerBitmap(c *Container, bitmap []uint64) *Container { return c } +// RemakeContainerBitmapN uses the provided n instead of counting bits. The +// provided container must not be frozen. +func RemakeContainerBitmapN(c *Container, bitmap []uint64, n int32) *Container { + *c = Container{typeID: ContainerBitmap} + c.setBitmap(bitmap) + c.n = n + return c +} + +// RemakeContainerArray populates c with an array container using the provided +// array. It must not be used on a frozen container. func RemakeContainerArray(c *Container, array []uint16) *Container { *c = Container{typeID: ContainerArray} c.setArray(array) return c } +// RemakeContainerRun repopulates c with the provided intervals. c must not +// be frozen. func RemakeContainerRun(c *Container, intervals []Interval16) *Container { *c = Container{typeID: ContainerRun} c.setRuns(intervals) + c.n = 0 + for _, r := range intervals { + c.n += int32(r.Last - r.Start + 1) + } + return c +} + +// RemakeContainerRunN repopulates c with the provided intervals, but +// assumes the provided n is accurate. c must not be frozen. +func RemakeContainerRunN(c *Container, intervals []Interval16, n int32) *Container { + *c = Container{typeID: ContainerRun} + c.setRuns(intervals) + c.n = n return c } From 2833365aae444b00c98a846806f0f68a3a63f9f3 Mon Sep 17 00:00:00 2001 From: Seebs Date: Mon, 12 Apr 2021 16:14:45 -0500 Subject: [PATCH 2/2] reuse the findExisting filter between fields, drop separate hack for existence The existence field wasn't working because runs were broken for filters in RBF. Fixing that allows us to simplify the logic. Also, we reuse the findExisting filter because the filter's cached collection of containers can be reused between things, allowing us to reduce allocations when there's a lot of views. --- executor.go | 48 ++++++++++++++++++++---------------------------- 1 file changed, 20 insertions(+), 28 deletions(-) diff --git a/executor.go b/executor.go index 58d99f90e..593be48dc 100644 --- a/executor.go +++ b/executor.go @@ -8351,13 +8351,17 @@ func (e *executor) executeDeleteRecordFromShard(ctx context.Context, qcx *Qcx, i if len(row.segments) == 0 { //nothing to remove return false, nil } + columns := row.segments[0].data //should only be one segment + if columns.Count() == 0 { + return false, nil + } // Fetch index. idx := e.Holder.Index(index) if idx == nil { return false, newNotFoundError(ErrIndexNotFound, index) } - columns := row.segments[0].data //should only be one segment + columnIDs := make([]uint64, 0) none := make([]uint64, 0) // no bits will be set @@ -8367,22 +8371,27 @@ func (e *executor) executeDeleteRecordFromShard(ctx context.Context, qcx *Qcx, i } defer finisher(&err) changed := false - clearFragment := func(frag *fragment) (bool, error) { - toClear := columnIDs[:0] - rowSet := make(map[uint64]struct{}) + colCounts := make([]int, 0) + toClear := columnIDs[:0] + rowSet := make(map[uint64]struct{}) + callback := func(pos uint64) error { + toClear = append(toClear, pos) + rowID := pos / ShardWidth + rowSet[rowID] = struct{}{} + return nil + } + findExisting := roaring.NewBitmapBitmapFilter(columns, callback) - callback := func(pos uint64) error { - toClear = append(toClear, pos) - rowID := pos / ShardWidth - rowSet[rowID] = struct{}{} - return nil - } - findExisting := roaring.NewBitmapBitmapFilter(columns, callback) + clearFragment := func(frag *fragment) (bool, error) { + // re-zero these + toClear = columnIDs[:0] + rowSet = make(map[uint64]struct{}) err = tx.ApplyFilter(frag.index(), frag.field(), frag.view(), frag.shard, 0, findExisting) if err != nil { return false, err } + colCounts = append(colCounts, len(toClear)) // this will be the remove part if len(toClear) > 0 { err = frag.importPositions(tx, none, toClear, rowSet) @@ -8409,22 +8418,5 @@ func (e *executor) executeDeleteRecordFromShard(ctx context.Context, qcx *Qcx, i } } } - if idx.trackExistence { - for _, view := range idx.existenceFld.views() { - frag, ok := view.fragments[shard] - if !ok { - continue - } - for _, bit := range columns.Slice() { - c, err := frag.clearBit(tx, 0, bit) - if err != nil { - return false, nil - } - if c { - changed = true - } - } - } - } return changed, nil }