From 636f1325649e1b505dd6bc6e6c8563da5aed0ac5 Mon Sep 17 00:00:00 2001 From: Seebs Date: Fri, 31 May 2019 15:39:13 -0500 Subject: [PATCH 1/4] add a test case which breaks the rowcache code It turns out that frozen containers which have mmapped data are only safe *until the data gets unmapped*. Which it does on a snapshot. --- fragment_internal_test.go | 39 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+) diff --git a/fragment_internal_test.go b/fragment_internal_test.go index 19dc3f587..c159bb1a1 100644 --- a/fragment_internal_test.go +++ b/fragment_internal_test.go @@ -26,6 +26,7 @@ import ( "os" "reflect" "sort" + "sync/atomic" "testing" "testing/quick" @@ -104,6 +105,44 @@ func TestFragment_ClearBit(t *testing.T) { } } +// What about rowcache timing. +func TestFragment_RowcacheMap(t *testing.T) { + var done int64 + f := mustOpenFragment("i", "f", viewStandard, 0, "") + defer f.Clean(t) + + ch := make(chan struct{}) + + for i := 0; i < f.MaxOpN; i++ { + f.setBit(0, uint64(i*32)) + } + // force snapshot so we get a mmapped row... + f.snapshot() + row := f.row(0) + segment := row.Segments()[0] + bitmap := segment.data + + // request information from the frozen bitmap we got back + go func() { + for atomic.LoadInt64(&done) == 0 { + for i := 0; i < f.MaxOpN; i++ { + _ = bitmap.Contains(uint64(i * 32)) + } + } + close(ch) + }() + + // modify the original bitmap, until it causes a snapshot, which + // then invalidates the other map... + for j := 0; j < 5; j++ { + for i := 0; i < f.MaxOpN; i++ { + f.setBit(0, uint64(i*32+j+1)) + } + } + atomic.StoreInt64(&done, 1) + <-ch +} + // Ensure a fragment can clear a row. func TestFragment_ClearRow(t *testing.T) { f := mustOpenFragment("i", "f", viewStandard, 0, "") From 973579e662da41344880a4ff4a558cd89c94e845 Mon Sep 17 00:00:00 2001 From: Seebs Date: Fri, 31 May 2019 16:17:06 -0500 Subject: [PATCH 2/4] on freeze, unmap mapped containers It turns out that calling syscall.Munmap() is a thing which can change any container holding a pointer into the mapped space, but which wouldn't detect frozen containers. So we need to copy storage for such things. This negates some of the memory wins of the rowcache code, but makes it not crashy. --- roaring/container_stash.go | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/roaring/container_stash.go b/roaring/container_stash.go index 46725238f..fe03f9d2e 100644 --- a/roaring/container_stash.go +++ b/roaring/container_stash.go @@ -256,11 +256,20 @@ func (c *Container) setMapped(mapped bool) { } // Freeze returns an unmodifiable container identical to c. This might -// be c, now marked unmodifiable, or might be a new container. +// 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. func (c *Container) Freeze() *Container { if c == nil { return nil } + // don't need to freeze + if c.flags&flagFrozen != 0 { + return c + } + // unmapOrClone should unmap-in-place because the existing + // container isn't frozen (or we'd already have returned it). + c = c.unmapOrClone() c.flags |= flagFrozen return c } From 372c369e7c67f344dca3d6ac55efbfe9e505b39a Mon Sep 17 00:00:00 2001 From: Seebs Date: Tue, 4 Jun 2019 08:55:35 -0500 Subject: [PATCH 3/4] Optimize needs to use the new container logic When calling `.optimize`, need to grab the new container which may be different from the original container. --- roaring/roaring.go | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/roaring/roaring.go b/roaring/roaring.go index 8930981e0..f09667104 100644 --- a/roaring/roaring.go +++ b/roaring/roaring.go @@ -971,11 +971,9 @@ func (b *Bitmap) countEmptyContainers() int { // Optimize converts array and bitmap containers to run containers as necessary. func (b *Bitmap) Optimize() { - citer, _ := b.Containers.Iterator(0) - for citer.Next() { - _, c := citer.Value() - c.optimize() - } + b.Containers.UpdateEvery(func(c *Container, existed bool) (*Container, bool) { + return c.optimize(), true + }) } type errWriter struct { @@ -3519,7 +3517,7 @@ RUNLOOP: } } output := NewContainerRun(runs) - output.optimize() + output = output.optimize() return output } From 77cd21e89f36cddc47a09988fc083d33f5f00e29 Mon Sep 17 00:00:00 2001 From: Seebs Date: Tue, 4 Jun 2019 09:16:01 -0500 Subject: [PATCH 4/4] don't check errors we don't care about in a test --- fragment_internal_test.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/fragment_internal_test.go b/fragment_internal_test.go index c159bb1a1..e45d3cce8 100644 --- a/fragment_internal_test.go +++ b/fragment_internal_test.go @@ -114,10 +114,10 @@ func TestFragment_RowcacheMap(t *testing.T) { ch := make(chan struct{}) for i := 0; i < f.MaxOpN; i++ { - f.setBit(0, uint64(i*32)) + _, _ = f.setBit(0, uint64(i*32)) } // force snapshot so we get a mmapped row... - f.snapshot() + _ = f.snapshot() row := f.row(0) segment := row.Segments()[0] bitmap := segment.data @@ -136,7 +136,7 @@ func TestFragment_RowcacheMap(t *testing.T) { // then invalidates the other map... for j := 0; j < 5; j++ { for i := 0; i < f.MaxOpN; i++ { - f.setBit(0, uint64(i*32+j+1)) + _, _ = f.setBit(0, uint64(i*32+j+1)) } } atomic.StoreInt64(&done, 1)