From fbb648b1fbc926bb9513385602a779861eb7cda4 Mon Sep 17 00:00:00 2001 From: Seebs Date: Thu, 24 Sep 2020 16:23:55 -0500 Subject: [PATCH] enforce append-like semantics for *Container more consistently The copy-on-write/rowCache changes require that functions that modify containers be able to generate new containers. Once that became possible, some significant pool of other operations started relying on it -- for instance, operations might return a new container even though they're in theory "in place" operations. I developed a tool for checking for unused function return values (github.com/molecula/noticeme), and ran it on this, and picked out the places where `*Container` values were generated but not used, and some of them seem to be potentially-real bugs, and a few are probably harmless. Updated code to make those diagnostics go away. --- go.sum | 1 + roaring/btree_test.go | 4 +- roaring/containers_btree.go | 2 +- roaring/roaring.go | 98 ++++++++++++++++++-------------- roaring/roaring_internal_test.go | 59 ++++++++++--------- roaring/unmarshal_binary.go | 4 +- 6 files changed, 89 insertions(+), 79 deletions(-) diff --git a/go.sum b/go.sum index 39084cbd8..231a844ad 100644 --- a/go.sum +++ b/go.sum @@ -328,6 +328,7 @@ golang.org/x/tools v0.0.0-20190114222345-bf090417da8b/go.mod h1:n7NCudcB/nEzxVGm golang.org/x/tools v0.0.0-20190226205152-f727befe758c/go.mod h1:9Yl7xja0Znq3iFh3HoIrodX9oNMXvdceNzlUR8zjMvY= golang.org/x/tools v0.0.0-20190311212946-11955173bddd/go.mod h1:LCzVGOaR6xXOjkQ3onu1FJEFr0SW1gC7cKk1uF8kGRs= golang.org/x/tools v0.0.0-20190524140312-2c0ae7006135/go.mod h1:RgjU9mgBXZiqYHBnxXauZ1Gv1EHHAz9KjViQ78xBX0Q= +golang.org/x/tools v0.0.0-20191119224855-298f0cb1881e h1:aZzprAO9/8oim3qStq3wc1Xuxx4QmAGriC4VU4ojemQ= golang.org/x/tools v0.0.0-20191119224855-298f0cb1881e/go.mod h1:b+2E5dAYhXwXZwtnZ6UAqBI28+e2cm9otk0dWdXHAEo= golang.org/x/xerrors v0.0.0-20190717185122-a985d3407aa7/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0= golang.org/x/xerrors v0.0.0-20191011141410-1b5146add898/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0= diff --git a/roaring/btree_test.go b/roaring/btree_test.go index 65f05d0c4..7a952a937 100644 --- a/roaring/btree_test.go +++ b/roaring/btree_test.go @@ -440,7 +440,7 @@ func benchmarkGetSeq(b *testing.B, n int) { b.ReportAllocs() for i := 0; i < b.N; i++ { for j := 0; j < n; j++ { - r.Get(uint64(j)) + _, _ = r.Get(uint64(j)) } } b.StopTimer() @@ -518,7 +518,7 @@ func benchmarkGetRnd(b *testing.B, n int) { b.ReportAllocs() for i := 0; i < b.N; i++ { for _, v := range a { - r.Get(uint64(v)) + _, _ = r.Get(uint64(v)) } } b.StopTimer() diff --git a/roaring/containers_btree.go b/roaring/containers_btree.go index c31f0d746..8c967743b 100644 --- a/roaring/containers_btree.go +++ b/roaring/containers_btree.go @@ -188,7 +188,7 @@ func (btc *bTreeContainers) Repair() { // (new-container, write). If write is true, the container is used to // replace the given container. func (btc *bTreeContainers) Update(key uint64, fn func(*Container, bool) (*Container, bool)) { - btc.tree.Put(key, fn) + _, _ = btc.tree.Put(key, fn) btc.lastKey = ^uint64(0) btc.lastContainer = nil } diff --git a/roaring/roaring.go b/roaring/roaring.go index 89c9618bf..af9ce78a2 100644 --- a/roaring/roaring.go +++ b/roaring/roaring.go @@ -1568,7 +1568,7 @@ func (b *Bitmap) Shift(n int) (*Bitmap, error) { } o, carry := shift(ci) if lastCarry { - o.add(0) + o, _ = o.add(0) } if o.N() > 0 { output.Containers.Put(ki, o) @@ -4543,7 +4543,7 @@ func unionRunRun(a, b *Container) *Container { } output.setN(n) if len(output.runs()) > runMaxSize { - output.runToBitmap() + output = output.runToBitmap() } return output } @@ -5072,14 +5072,14 @@ func differenceArrayArray(a, b *Container) *Container { for i, j := 0, 0; i < na; { va := aa[i] if j >= nb { - output.add(va) + output, _ = output.add(va) i++ continue } vb := ab[j] if va < vb { - output.add(va) + output, _ = output.add(va) i++ } else if va > vb { j++ @@ -6482,14 +6482,15 @@ func (b *Bitmap) DifferenceInPlace(others ...*Bitmap) { if targetKey == iKey { // 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) + // Note: This Thaw() may be unnecessary, but some of the + // differenceInPlace code may be assuming the container is + // always writable. + curContainer = curContainer.Thaw().differenceInPlace(iContainer) if curContainer.N() == 0 { removeContainerKeys = append(removeContainerKeys, targetKey) break + } else { + b.Containers.Put(targetKey, curContainer) } } iIter.hasNext = iIter.iter.Next() @@ -6504,43 +6505,44 @@ func (b *Bitmap) DifferenceInPlace(others ...*Bitmap) { target.Containers.Repair() } -func (c *Container) differenceInPlace(other *Container) { +func (c *Container) differenceInPlace(other *Container) *Container { if other == nil { - return + return c } if other.isArray() { if c.isArray() { - differenceArrayArrayInPlace(c, other) + return differenceArrayArrayInPlace(c, other) } else if c.isBitmap() { - differenceBitmapArrayInPlace(c, other) + return differenceBitmapArrayInPlace(c, other) } else if c.isRun() { - differenceRunArrayInPlace(c, other) + return differenceRunArrayInPlace(c, other) } } else if other.isBitmap() { if c.isArray() { - differenceArrayBitmapInPlace(c, other) + return differenceArrayBitmapInPlace(c, other) } else if c.isBitmap() { - differenceBitmapBitmapInPlace(c, other) + return differenceBitmapBitmapInPlace(c, other) } else if c.isRun() { - differenceRunBitmapInPlace(c, other) + return differenceRunBitmapInPlace(c, other) } } else if other.isRun() { if c.isArray() { - differenceArrayRunInPlace(c, other) + return differenceArrayRunInPlace(c, other) } else if c.isBitmap() { - differenceBitmapRunInPlace(c, other) + return differenceBitmapRunInPlace(c, other) } else if c.isRun() { - differenceRunRunInPlace(c, other) + return differenceRunRunInPlace(c, other) } } + return c } -func differenceArrayArrayInPlace(c, other *Container) { +func differenceArrayArrayInPlace(c, other *Container) *Container { statsHit("differenceInPlace/ArrayArray") aa, ab := c.array(), other.array() na, nb := len(aa), len(ab) if na == 0 || nb == 0 { - return + return c } n := 0 for i, j := 0, 0; i < na; { @@ -6565,15 +6567,16 @@ func differenceArrayArrayInPlace(c, other *Container) { } aa = aa[:n] c.setArray(aa) + return c } -func differenceArrayBitmapInPlace(c, other *Container) { +func differenceArrayBitmapInPlace(c, other *Container) *Container { statsHit("differenceInPlace/ArrayBitmap") aa := c.array() n := 0 bitmap := other.bitmap() if len(aa) == 0 || len(bitmap) == 0 { - return + return c } for _, va := range aa { bmidx := va / 64 @@ -6588,16 +6591,17 @@ func differenceArrayBitmapInPlace(c, other *Container) { } aa = aa[:n] c.setArray(aa) + return c } -func differenceArrayRunInPlace(c, other *Container) { +func differenceArrayRunInPlace(c, other *Container) *Container { statsHit("differenceInPlace/ArrayRun") i := 0 // array index j := 0 // run index aa, rb := c.array(), other.runs() if len(aa) == 0 || len(rb) == 0 { - return + return c } n := 0 @@ -6632,14 +6636,15 @@ func differenceArrayRunInPlace(c, other *Container) { } aa = aa[:n] c.setArray(aa) + return c } -func differenceBitmapArrayInPlace(c, other *Container) { +func differenceBitmapArrayInPlace(c, other *Container) *Container { statsHit("differenceInPlace/BitmapArray") bitmap := c.bitmap() ab := other.array() if len(bitmap) == 0 || len(ab) == 0 { - return + return c } n := c.N() @@ -6651,18 +6656,19 @@ func differenceBitmapArrayInPlace(c, other *Container) { } c.setN(n) if n < ArrayMaxSize { - c.bitmapToArray() // With This Work + c = c.bitmapToArray() // With This Work } + return c } -func differenceBitmapBitmapInPlace(c, other *Container) { +func differenceBitmapBitmapInPlace(c, other *Container) *Container { statsHit("differenceInPlace/BitmapBitmap") // local variables added to prevent BCE checks in loop // see https://go101.org/article/bounds-check-elimination.html a := c.bitmap() b := other.bitmap() if len(a) == 0 || len(b) == 0 { - return + return c } var ( @@ -6677,25 +6683,27 @@ func differenceBitmapBitmapInPlace(c, other *Container) { } c.setN(n) if n < ArrayMaxSize { - c.bitmapToArray() // Will this work? + c = c.bitmapToArray() } + return c } -func differenceBitmapRunInPlace(c, other *Container) { +func differenceBitmapRunInPlace(c, other *Container) *Container { statsHit("differenceInPlace/BitmapRun") if len(c.bitmap()) == 0 { - return + return c } for _, run := range other.runs() { c.bitmapZeroRange(uint64(run.Start), uint64(run.Last)+1) } + return c } -func differenceRunArrayInPlace(c, other *Container) { +func differenceRunArrayInPlace(c, other *Container) *Container { statsHit("differenceInPlace/RunArray") ra, ab := c.runs(), other.array() if len(ra) == 0 || len(ab) == 0 { - return + return c } runs := make([]Interval16, 0, len(ra)) bidx := 0 @@ -6745,14 +6753,14 @@ RUNLOOP: for _, run := range runs { c.n += int32(run.Last-run.Start) + 1 } - c.optimize() + return c.optimize() } -func differenceRunBitmapInPlace(c, other *Container) { +func differenceRunBitmapInPlace(c, other *Container) *Container { statsHit("differenceInPlace/RunBitmap") ra := c.runs() if len(ra) == 0 || len(other.bitmap()) == 0 { - return + return c } // If a is full, difference is the flip of b. if len(ra) > 0 && ra[0].Start == 0 && ra[0].Last == 65535 { @@ -6765,7 +6773,7 @@ func differenceRunBitmapInPlace(c, other *Container) { c.setMapped(false) c.setBitmap(bitmap) c.setN(c.count()) - return + return c } runs := make([]Interval16, 0, len(ra)) for _, inputRun := range ra { @@ -6811,18 +6819,19 @@ func differenceRunBitmapInPlace(c, other *Container) { c.n += int32(run.Last-run.Start) + 1 } if c.N() < ArrayMaxSize && int32(len(runs)) > c.N()/2 { - c.runToArray() + c = c.runToArray() } else if len(runs) > runMaxSize { - c.runToBitmap() + c = c.runToBitmap() } + return c } -func differenceRunRunInPlace(c, other *Container) { +func differenceRunRunInPlace(c, other *Container) *Container { statsHit("differenceInPlace/RunRun") ra, rb := c.runs(), other.runs() if len(ra) == 0 || len(rb) == 0 { - return + return c } apos := 0 // current a-run index bpos := 0 // current b-run index @@ -6882,6 +6891,7 @@ func differenceRunRunInPlace(c, other *Container) { for _, run := range runs { c.n += int32(run.Last-run.Start) + 1 } + return c } //RBF exports to be reconsidered as we progress diff --git a/roaring/roaring_internal_test.go b/roaring/roaring_internal_test.go index 1a459fd8a..b432f1858 100644 --- a/roaring/roaring_internal_test.go +++ b/roaring/roaring_internal_test.go @@ -135,19 +135,19 @@ func TestRunCountRange(t *testing.T) { if cnt != 0 { t.Fatalf("should get 0 from empty container, but got: %v", cnt) } - c.add(5) - c.add(6) - c.add(7) + c, _ = c.add(5) + c, _ = c.add(6) + c, _ = c.add(7) cnt = RunCountRange(c.runs(), 2, 9) if cnt != 3 { t.Fatalf("should get 3 from interval within range, but got: %v", cnt) } - c.add(8) - c.add(9) - c.add(10) - c.add(11) + c, _ = c.add(8) + c, _ = c.add(9) + c, _ = c.add(10) + c, _ = c.add(11) cnt = RunCountRange(c.runs(), 4, 8) if cnt != 3 { @@ -199,17 +199,17 @@ func TestRunCountRange(t *testing.T) { t.Fatalf("should get 6 from interval equal to range, but got: %v", cnt) } - c.add(17) - c.add(19) - c.add(18) + c, _ = c.add(17) + c, _ = c.add(19) + c, _ = c.add(18) cnt = RunCountRange(c.runs(), 1, 22) if cnt != 10 { t.Fatalf("should get 10 from multiple ranges in interval, but got: %v", cnt) } - c.add(13) - c.add(14) + c, _ = c.add(13) + c, _ = c.add(14) cnt = RunCountRange(c.runs(), 6, 18) if cnt != 9 { @@ -227,17 +227,17 @@ func TestRunContains(t *testing.T) { if c.runContains(5) { t.Fatalf("empty run container should not contain 5") } - c.add(5) + c, _ = c.add(5) if !c.runContains(5) { t.Fatalf("run container with 5 should contain 5") } - c.add(6) - c.add(7) + c, _ = c.add(6) + c, _ = c.add(7) - c.add(9) - c.add(10) - c.add(11) + c, _ = c.add(9) + c, _ = c.add(10) + c, _ = c.add(11) if !c.runContains(10) { t.Fatalf("run container with 10 in second run should contain 10") @@ -282,7 +282,7 @@ func TestIntersectionCountArrayBitmap3(t *testing.T) { if res.N() != res.count() || res.N() != MaxContainerVal+1 { t.Fatalf("test #2 intersectCountBitmapRun fail orig: %v new: %v exp: %v", res.N(), res.count(), MaxContainerVal+1) } - b.bitmapToRun(0) + b = b.bitmapToRun(0) res = intersectRunRun(a, b) n := intersectionCountRunRun(a, b) if res.N() != res.count() || res.N() != MaxContainerVal+1 || res.N() != int32(n) { @@ -613,7 +613,7 @@ func TestIntersectBitmapRunBitmap(t *testing.T) { b.setN(4097) ret := intersectBitmapRun(a, b) if ret.isArray() { - ret.arrayToBitmap() + ret = ret.arrayToBitmap() } if !reflect.DeepEqual(ret.bitmap(), exp) { t.Fatalf("test #%v expected %v, but got %v", i, exp, ret.bitmap()) @@ -710,9 +710,9 @@ func TestUnionMixed(t *testing.T) { res := union(tt.c1, tt.c2) // convert to array for comparison if res.isBitmap() { - res.bitmapToArray() + res = res.bitmapToArray() } else if res.isRun() { - res.runToArray() + res = res.runToArray() } if !reflect.DeepEqual(res.array(), tt.exp) { t.Fatalf("test %s expected %v, but got %v", tt.name, tt.exp, res.array()) @@ -1304,11 +1304,11 @@ func TestBitmapToRun(t *testing.T) { for i, test := range tests { a := NewContainerBitmap(-1, test.bitmap) x := a.bitmap() - a.bitmapToRun(0) + a = a.bitmapToRun(0) if !reflect.DeepEqual(a.runs(), test.exp) { t.Fatalf("test #%v expected %v, but got %v", i, test.exp, a.runs()) } - a.runToBitmap() + a = a.runToBitmap() if !reflect.DeepEqual(a.bitmap(), x) { t.Fatalf("test #%v expected %v, but got %v", i, a.bitmap(), x) } @@ -1633,7 +1633,7 @@ func MakeBitmap(start []uint64) []uint64 { func MakeLastBitSet() []uint64 { obj := NewFileBitmap(65535) c := obj.container(0) - c.arrayToBitmap() + c = c.arrayToBitmap() return c.bitmap() } @@ -2924,8 +2924,7 @@ func unionInPlaceWrapper(a, b *Container) *Container { func differenceInPlaceWrapper(a, b *Container) *Container { a = a.Clone() - // this should probably return its new value, but currently does not - a.differenceInPlace(b) + a = a.differenceInPlace(b) return a } @@ -3859,7 +3858,7 @@ func BenchmarkUnionBitmapBitmapInPlace(b *testing.B) { b1 := newTestBitmapContainer() b2 := newTestBitmapContainer() for n := 0; n < b.N; n++ { - unionBitmapBitmapInPlace(b1, b2) + b1 = unionBitmapBitmapInPlace(b1, b2) } } @@ -4356,7 +4355,7 @@ func BenchmarkUnionRunRunInPlace(bm *testing.B) { brun := doContainer(ContainerRun, br.fn()) abmp := arun.runToBitmap() - unionBitmapRunInPlace(abmp, brun) + _ = unionBitmapRunInPlace(abmp, brun) } }) @@ -4365,7 +4364,7 @@ func BenchmarkUnionRunRunInPlace(bm *testing.B) { arun := doContainer(ContainerRun, ar.fn()) brun := doContainer(ContainerRun, br.fn()) - unionRunRunInPlace(arun, brun) + _ = unionRunRunInPlace(arun, brun) } }) } diff --git a/roaring/unmarshal_binary.go b/roaring/unmarshal_binary.go index 87e693187..72d73e961 100644 --- a/roaring/unmarshal_binary.go +++ b/roaring/unmarshal_binary.go @@ -60,7 +60,7 @@ func (b *Bitmap) UnmarshalBinary(data []byte) (err error) { } newC.setMapped(true) if !b.preferMapping { - newC.unmapOrClone() + newC = newC.unmapOrClone() } b.Containers.Put(itrKey, newC) itrKey, itrCType, itrN, itrLen, itrPointer, itrErr = itr.Next() @@ -152,7 +152,7 @@ func InspectBinary(data []byte, mapped bool, info *BitmapInfo) (b *Bitmap, mappe } newC.setMapped(true) if !mapped { - newC.unmapOrClone() + newC = newC.unmapOrClone() } newC.flags |= flagPristine if newC.flags&flagMapped != 0 {