From 8ab9174a096ca4d50ff195db2ed7e9a9e78f96d0 Mon Sep 17 00:00:00 2001 From: Seebs Date: Mon, 14 Sep 2020 13:49:02 -0500 Subject: [PATCH] export SetMapped from roaring, use it in Tx stores Thaw() is supposed to always provide writable storage, which it does by ensuring that containers aren't frozen, but also by cloning or copying their data if the data is marked as being memory-mapped. But only the roaring backend had the ability to mark data as memory-mapped, because that wasn't exported. Fixed this, and added corresponding code to badger, lmdb, and rbf. --- badger.go | 13 ++++++------- lmdb.go | 13 ++++++------- rbf/cursorx.go | 16 ++++++++++------ roaring/container_stash.go | 7 +++++++ roaring/roaring.go | 29 ++--------------------------- 5 files changed, 31 insertions(+), 47 deletions(-) diff --git a/badger.go b/badger.go index 3af0ce477..39500f262 100644 --- a/badger.go +++ b/badger.go @@ -1628,7 +1628,7 @@ func (tx *BadgerTx) ImportRoaringBits(index, field, view string, shard uint64, i return } -func (tx *BadgerTx) toContainer(typ byte, v []byte) (r *roaring.Container) { +func (tx *BadgerTx) toContainer(typ byte, v []byte) (c *roaring.Container) { if len(v) == 0 { return nil @@ -1669,29 +1669,28 @@ func (tx *BadgerTx) toContainer(typ byte, v []byte) (r *roaring.Container) { switch typ { case roaring.ContainerArray: - c := roaring.NewContainerArray(toArray16(w)) + c = roaring.NewContainerArray(toArray16(w)) if tx.doAllocZero { // tx.acMu was acquired above, and Unlock deferred. tx.ourContainers = append(tx.ourContainers, c) } - return c case roaring.ContainerBitmap: - c := roaring.NewContainerBitmap(-1, toArray64(w)) + c = roaring.NewContainerBitmap(-1, toArray64(w)) if tx.doAllocZero { // tx.acMu was acquired above, and Unlock deferred. tx.ourContainers = append(tx.ourContainers, c) } - return c case roaring.ContainerRun: - c := roaring.NewContainerRun(toInterval16(w)) + c = roaring.NewContainerRun(toInterval16(w)) if tx.doAllocZero { // tx.acMu was acquired above, and Unlock deferred. tx.ourContainers = append(tx.ourContainers, c) } - return c default: panic(fmt.Sprintf("unknown container: %v", typ)) } + c.SetMapped(true) + return c } // StringifiedBadgerKeys returns a string with all the container diff --git a/lmdb.go b/lmdb.go index cf1db9f43..e18907d55 100644 --- a/lmdb.go +++ b/lmdb.go @@ -1520,20 +1520,19 @@ func (tx *LMDBTx) toContainer(typ byte, v []byte) (r *roaring.Container) { return ToContainer(typ, w) } -func ToContainer(typ byte, w []byte) (r *roaring.Container) { +func ToContainer(typ byte, w []byte) (c *roaring.Container) { switch typ { case roaring.ContainerArray: - c := roaring.NewContainerArray(toArray16(w)) - return c + c = roaring.NewContainerArray(toArray16(w)) case roaring.ContainerBitmap: - c := roaring.NewContainerBitmap(-1, toArray64(w)) - return c + c = roaring.NewContainerBitmap(-1, toArray64(w)) case roaring.ContainerRun: - c := roaring.NewContainerRun(toInterval16(w)) - return c + c = roaring.NewContainerRun(toInterval16(w)) default: panic(fmt.Sprintf("unknown container: %v", typ)) } + c.SetMapped(true) + return c } // StringifiedLMDBKeys returns a string with all the container diff --git a/rbf/cursorx.go b/rbf/cursorx.go index 7d2d9332c..8f3c6ea96 100644 --- a/rbf/cursorx.go +++ b/rbf/cursorx.go @@ -150,22 +150,25 @@ func (c *Cursor) CurrentPageType() int { return cell.Type } -func toContainer(l leafCell, tx *Tx) *roaring.Container { +func toContainer(l leafCell, tx *Tx) (c *roaring.Container) { orig := l.Data var cpMaybe []byte + var mapped bool if EnableRowCache || DoAllocZero { // make a copy, otherwise the rowCache will see corrupted data // or mmapped data that may disappear. cpMaybe = make([]byte, len(orig)) copy(cpMaybe, orig) + mapped = false } else { // not a copy cpMaybe = orig + mapped = true } switch l.Type { case ContainerTypeArray: - return roaring.NewContainerArray(toArray16(cpMaybe)) + c = roaring.NewContainerArray(toArray16(cpMaybe)) case ContainerTypeBitmapPtr: _, bm, _ := tx.leafCellBitmap(toPgno(cpMaybe)) cloneMaybe := bm @@ -173,13 +176,14 @@ func toContainer(l leafCell, tx *Tx) *roaring.Container { cloneMaybe = make([]uint64, len(bm)) copy(cloneMaybe, bm) } - return roaring.NewContainerBitmap(l.N, cloneMaybe) + c = roaring.NewContainerBitmap(l.N, cloneMaybe) case ContainerTypeBitmap: - return roaring.NewContainerBitmap(l.N, toArray64(cpMaybe)) + c = roaring.NewContainerBitmap(l.N, toArray64(cpMaybe)) case ContainerTypeRLE: - return roaring.NewContainerRun(toInterval16(cpMaybe)) + c = roaring.NewContainerRun(toInterval16(cpMaybe)) } - return nil + c.SetMapped(mapped) + return c } type Nodetype int diff --git a/roaring/container_stash.go b/roaring/container_stash.go index e08f823a6..e87a65fb8 100644 --- a/roaring/container_stash.go +++ b/roaring/container_stash.go @@ -309,6 +309,13 @@ func (c *Container) setMapped(mapped bool) { } } +// SetMapped marks a container as "mapped"; do this if you're setting a +// container's storage to something that it shouldn't write to, like mmapped +// memory. +func (c *Container) SetMapped(mapped bool) { + c.setMapped(mapped) +} + // 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) { diff --git a/roaring/roaring.go b/roaring/roaring.go index 52e806f52..384149ea6 100644 --- a/roaring/roaring.go +++ b/roaring/roaring.go @@ -3300,33 +3300,8 @@ func (c *Container) arrayRemove(v uint16) (*Container, bool) { } c = c.Thaw() array = c.array() - - const needCopyOnWriteDueToReadOnlyMmap = true - - // TODO(jea) quick benchmarks don't show less performance if needCopyOnWriteDueToReadOnlyMmap - // is true, but we may need more rigorous measurement. - // - // Don't have a COW: - // all benchmarks in roaring/ - // ok github.com/pilosa/pilosa/v2/roaring 217.138s - // - // ok, have a COW: - // all benchmarks in roaring/ - // ok github.com/pilosa/pilosa/v2/roaring 214.295s - - if needCopyOnWriteDueToReadOnlyMmap { - n := len(array) - array2 := make([]uint16, n-1) - copy(array2, array[:i]) - copy(array2[i:], array[i+1:]) - c.setArray(array2) - } else { - // seg fault here with read-only mmap; go 1.14.7 linux. - // example: cap = 7 i = 0 len = 7 - // the append tries to write read-only memory? - array = append(array[:i], array[i+1:]...) - c.setArray(array) - } + array = append(array[:i], array[i+1:]...) + c.setArray(array) return c, true }