From d26e221a914fb5d4ab82a6d3ac8828fe39c241b1 Mon Sep 17 00:00:00 2001 From: Seebs Date: Tue, 31 Mar 2020 11:03:29 -0500 Subject: [PATCH 1/4] don't call isArray on a nil *Container differenceInPlace wasn't checking for nil containers, which are theoretically valid empty containers. Also added a couple of other N==0 checks to streamline the higher-level operation. --- roaring/roaring.go | 27 ++++++++++++++++++--------- 1 file changed, 18 insertions(+), 9 deletions(-) diff --git a/roaring/roaring.go b/roaring/roaring.go index 0503d7c38..c399b8df6 100644 --- a/roaring/roaring.go +++ b/roaring/roaring.go @@ -5540,6 +5540,10 @@ func (b *Bitmap) DifferenceInPlace(others ...*Bitmap) { // Go through all the containers and remove the other bits for targetItr.Next() { targetKey, curContainer := targetItr.Value() + // no point in subtracting things from an empty container. + if curContainer.N() == 0 { + removeContainerKeys = append(removeContainerKeys, targetKey) + } // Loop until every iters current value has been handled. for _, iIter := range bitmapIters { if !iIter.hasNext { @@ -5554,16 +5558,18 @@ func (b *Bitmap) DifferenceInPlace(others ...*Bitmap) { break } } - if targetKey == iKey { - if curContainer.frozen() { - curContainer = curContainer.Clone() - b.Containers.Put(targetKey, curContainer) - } - curContainer.differenceInPlace(iContainer) - if curContainer.N() == 0 { - removeContainerKeys = append(removeContainerKeys, iKey) - break + // note: a nil container is valid, and has N == 0. + if iContainer.N() != 0 { + if curContainer.frozen() { + curContainer = curContainer.Clone() + b.Containers.Put(targetKey, curContainer) + } + curContainer.differenceInPlace(iContainer) + if curContainer.N() == 0 { + removeContainerKeys = append(removeContainerKeys, targetKey) + break + } } iIter.hasNext = iIter.iter.Next() } @@ -5578,6 +5584,9 @@ func (b *Bitmap) DifferenceInPlace(others ...*Bitmap) { } func (c *Container) differenceInPlace(other *Container) { + if other == nil { + return + } if other.isArray() { if c.isArray() { differenceArrayArrayInPlace(c, other) From 1ac00291f3398a87c079cee63ea080ae18b327fa Mon Sep 17 00:00:00 2001 From: Seebs Date: Tue, 31 Mar 2020 11:36:24 -0500 Subject: [PATCH 2/4] Add test for the weird remapping/cache interaction. This test is really a test of a very specific bit of the internals of containers_btree/containers_slice, but we can't easily test it from there because they don't have all the logic for remapping files. The underlying issue is that they maintain a single-item "most recent container" cache, and this wasn't getting updated during the remap operations, happening through containers.UpdateEvery. The fix is probably just to make sure that UpdateEvery invalidates the cache. --- fragment_internal_test.go | 56 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 56 insertions(+) diff --git a/fragment_internal_test.go b/fragment_internal_test.go index 20262f66a..0b7ed8fd4 100644 --- a/fragment_internal_test.go +++ b/fragment_internal_test.go @@ -25,6 +25,8 @@ import ( "math/rand" "os" "reflect" + "runtime" + "runtime/debug" "sort" "sync/atomic" "testing" @@ -3583,6 +3585,60 @@ func TestFragmentConcurrentReadWrite(t *testing.T) { t.Logf("%d", acc) } +func TestRemapCache(t *testing.T) { + f := mustOpenFragment("i", "f", viewStandard, 0, "") + // request a panic that doesn't kill the program on fault + wouldFault := debug.SetPanicOnFault(true) + defer func() { + debug.SetPanicOnFault(wouldFault) + if r := recover(); r != nil { + if err, ok := r.(error); ok { + // special case: if we caught a page fault, we diagnose that directly. sadly, + // we can't see the actual values that were used to generate this, probably. + if err.Error() == "runtime error: invalid memory address or nil pointer dereference" { + t.Fatalf("segfault trapped during remap test (expected failure mode)") + } + } + t.Fatalf("unexpected panic: %v", r) + } + }() + // create a container + _, err := f.storage.Add(65537) + if err != nil { + t.Fatalf("storage add: %v", err) + } + // cause the container to be mapped + err = f.Snapshot() + if err != nil { + t.Fatalf("storage snapshot: %v", err) + } + // freeze the row + _ = f.row(0) + // add a bit that isn't in that container, so that container doesn't + // change + _, err = f.storage.Add(2) + if err != nil { + t.Fatalf("storage add: %v", err) + } + // make the original container be the most recent, thus cached, container + _, err = f.bit(0, 65537) + if err != nil { + t.Fatalf("storage bit check: %v", err) + } + // force snapshot, remapping the containers + err = f.Snapshot() + if err != nil { + t.Fatalf("storage snapshot: %v", err) + } + // get rid of the old mapping + runtime.GC() + // try to read that container again + _, err = f.bit(0, 65537) + if err != nil { + t.Fatalf("storage bit check: %v", err) + } +} + func TestFragment_Bug_Q2DoubleDelete(t *testing.T) { f := mustOpenFragment("i", "f", viewStandard, 0, "") b := []byte{60, 48, 0, 0, 1, 0, 0, 0, 16, 0, 0, 0, 0, 0, 0, 0, 1, 0, 0, 0, 24, 0, 0, 0, 1, 0} From 28b9d6d7fc5e803031636f0b6f14f72f4de4c113 Mon Sep 17 00:00:00 2001 From: Seebs Date: Mon, 30 Mar 2020 17:03:31 -0500 Subject: [PATCH 3/4] ditch lastKey cache on UpdateEvery UpdateEvery can change every key, and I think it strongly suggests no reasonable expectation of repeated access to a previously-accessed key, but also it can change the containers and replace them. We were avoiding caching mapped containers in some but not all cases, and that was causing segfaults. But really, the *problem* is that the remap operation wasn't clearing (or updating) the cache. Cleaning that up allows us to take advantage of the caching performance advantage even when working with read-only/mapped bitmaps. The only way to hit this: * Have mmapped containers to begin with. * Do reads so those containers get frozen. * Access, either reading or writing, a specific container with key K. * Snapshot, so the bitmap gets its containers replaced. * Remember, they have to be frozen -- if they aren't frozen, we'll update the containers in place. * Now have GC run so it actually unmaps the data. * Now try to write to the container with key K *before reading or writing any other key*. You have to get through the whole snapshot and GC process without any other reads or writes. * You get the cached value. You try to use it. You explode. The sliceContainers code was also setting lastKey to 0 in some cases, but also setting lastContainer to nil, so this wouldn't have caused problems, but just to be careful, I've standardized on ^uint64(0) for everything. --- roaring/containers_btree.go | 10 +++------- roaring/containers_slice.go | 10 +++++++--- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/roaring/containers_btree.go b/roaring/containers_btree.go index d00bf3247..62e08b9c6 100644 --- a/roaring/containers_btree.go +++ b/roaring/containers_btree.go @@ -62,12 +62,6 @@ func (btc *bTreeContainers) Put(key uint64, c *Container) { // Get can result in the tree containing a different container // than we'll get on next lookup. btc.lastKey, btc.lastContainer = key, c - // If a mapped container is added to the tree, reset the - // lastContainer cache so that the cache is not pointing - // at a read-only mmap. - if c.Mapped() { - btc.lastKey = ^uint64(0) - } btc.tree.Set(key, c) } @@ -119,7 +113,6 @@ func (btc *bTreeContainers) GetOrCreate(key uint64) *Container { btc.lastContainer = cont return cont } - btc.lastContainer = v return btc.lastContainer } @@ -229,6 +222,9 @@ func (btc *bTreeContainers) UpdateEvery(fn func(uint64, *Container, bool) (*Cont // currently not handling the error from this, but in practice it has // to be io.EOF. _ = e.Every(fn) + // invalidate cache. + btc.lastKey = ^uint64(0) + btc.lastContainer = nil } type btcIterator struct { diff --git a/roaring/containers_slice.go b/roaring/containers_slice.go index 610fd43a2..06a735b6d 100644 --- a/roaring/containers_slice.go +++ b/roaring/containers_slice.go @@ -43,7 +43,8 @@ func (sc *sliceContainers) Put(key uint64, c *Container) { } else { sc.containers[i] = c } - + sc.lastKey = key + sc.lastContainer = c } func (sc *sliceContainers) PutContainerValues(key uint64, typ byte, n int, mapped bool) { @@ -159,7 +160,7 @@ func (sc *sliceContainers) Reset() { sc.keys = sc.keys[:0] sc.containers = sc.containers[:0] sc.lastContainer = nil - sc.lastKey = 0 + sc.lastKey = ^uint64(0) } func (sc *sliceContainers) ResetN(n int) { @@ -171,7 +172,7 @@ func (sc *sliceContainers) ResetN(n int) { sc.containers = sc.containers[:0] } sc.lastContainer = nil - sc.lastKey = 0 + sc.lastKey = ^uint64(0) } func (sc *sliceContainers) seek(key uint64) (int, bool) { @@ -227,6 +228,9 @@ func (sc *sliceContainers) UpdateEvery(fn func(uint64, *Container, bool) (*Conta sc.containers[i] = nc } } + // invalidate cache. + sc.lastKey = ^uint64(0) + sc.lastContainer = nil } type sliceIterator struct { From 76e747055945e39fc6f151fbf3099ffaf19dfdc0 Mon Sep 17 00:00:00 2001 From: Seebs Date: Tue, 31 Mar 2020 12:02:45 -0500 Subject: [PATCH 4/4] make mergeBlock use transactions mergeBlock was bypassing the transaction setup stuff, which means that if we ran out of open files, mergeBlock wouldn't generate ops log entries (!), also it didn't update the cache (!). This came up because it also didn't enjoy the "catch your segfaults and issue a diagnostic" behavior offered by the generation code. Switch to computing positions directly and calling importPositions, which does a transaction. --- fragment.go | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/fragment.go b/fragment.go index f3c4d503b..28c524025 100644 --- a/fragment.go +++ b/fragment.go @@ -1862,21 +1862,20 @@ func (f *fragment) mergeBlock(id int, data []pairSet) (sets, clears []pairSet, e } } - // Set local bits. + rowSet := make(map[uint64]struct{}, len(sets[0].columnIDs)) + // compute positions directly, replacing columnIDs with the computed + // positions for i := range sets[0].columnIDs { - if _, err := f.unprotectedSetBit(sets[0].rowIDs[i], (f.shard*ShardWidth)+sets[0].columnIDs[i]); err != nil { - return nil, nil, errors.Wrap(err, "setting") - } + rowSet[sets[0].rowIDs[i]] = struct{}{} + sets[0].columnIDs[i] += sets[0].rowIDs[i] * ShardWidth } - - // Clear local bits. for i := range clears[0].columnIDs { - if _, err := f.unprotectedClearBit(clears[0].rowIDs[i], (f.shard*ShardWidth)+clears[0].columnIDs[i]); err != nil { - return nil, nil, errors.Wrap(err, "clearing") - } + rowSet[clears[0].rowIDs[i]] = struct{}{} + clears[0].columnIDs[i] += clears[0].rowIDs[i] * ShardWidth } + err = f.importPositions(sets[0].columnIDs, clears[0].columnIDs, rowSet) - return sets[1:], clears[1:], nil + return sets[1:], clears[1:], err } // bulkImport bulk imports a set of bits and then snapshots the storage.