From 55ff03a2d678c834c229c2c2bdbc48d7c9927935 Mon Sep 17 00:00:00 2001 From: Seebs Date: Fri, 5 Jun 2020 12:09:40 -0500 Subject: [PATCH 1/8] Lower scale of some random-value tests The random-value tests can be pathological, and in particular, the test of arbitrarily-spaced values is in effect O(N^2), and with race testing on, that test *alone* can take ten minutes to run, but it's not really all that exciting. We just reduce a bunch of values and/or test fewer things for these, which doesn't significantly alter coverage, but reduces test runtime on my laptop with `-race` from 21 minutes to a bit under 5. --- roaring/naive_test.go | 5 ++++- roaring/roaring_test.go | 18 +++++++++--------- 2 files changed, 13 insertions(+), 10 deletions(-) diff --git a/roaring/naive_test.go b/roaring/naive_test.go index 154d0f8f5..c1accfb5e 100644 --- a/roaring/naive_test.go +++ b/roaring/naive_test.go @@ -15,6 +15,7 @@ package roaring import ( + "math/rand" "reflect" "testing" ) @@ -110,13 +111,15 @@ func TestUnionSlice(t *testing.T) { } func TestMaxInSlice(t *testing.T) { + // arbitrary, we just want to get the same values every time + r := rand.New(rand.NewSource(23)) a := []uint64{1, 4, 9, 5, 24, 13} v := maxInSlice(a) if uint64(24) != v { t.Fatalf("expected %v, but got %v", uint64(24), v) } - for i := uint64(1000); i <= uint64(100000); i++ { + for i := uint64(1000); i <= uint64(100000); i += uint64(r.Intn(35)) + 1 { a = append(a, i) if v = maxInSlice(a); v != i { t.Fatalf("expected %v, but got %v", i, v) diff --git a/roaring/roaring_test.go b/roaring/roaring_test.go index 4f079102a..e31cbf2ee 100644 --- a/roaring/roaring_test.go +++ b/roaring/roaring_test.go @@ -866,7 +866,7 @@ func TestBitmap_UnionInPlaceProp(t *testing.T) { seed = time.Now().UnixNano() source = rand.NewSource(seed) rng = rand.New(source) - numTests = 100 + numTests = 20 maxNumIntsPerBatch = 100 maxNumBatches = 100 maxRangePercent = 2 @@ -1425,10 +1425,10 @@ func TestBitmap_Shift(t *testing.T) { } func TestBitmap_Quick_Array1(t *testing.T) { testBitmapQuick(t, 1000, 1000, 2000) } -func TestBitmap_Quick_Array2(t *testing.T) { testBitmapQuick(t, 10000, 0, 1000) } -func TestBitmap_Quick_Bitmap1(t *testing.T) { testBitmapQuick(t, 10000, 0, 10000) } -func TestBitmap_Quick_Bitmap2(t *testing.T) { testBitmapQuick(t, 10000, 10000, 20000) } -func TestBitmap_Quick_LargeValue(t *testing.T) { testBitmapQuick(t, 10000, 0, math.MaxInt64) } +func TestBitmap_Quick_Array2(t *testing.T) { testBitmapQuick(t, 1000, 0, 1000) } +func TestBitmap_Quick_Bitmap1(t *testing.T) { testBitmapQuick(t, 1000, 0, 10000) } +func TestBitmap_Quick_Bitmap2(t *testing.T) { testBitmapQuick(t, 1000, 10000, 20000) } +func TestBitmap_Quick_LargeValue(t *testing.T) { testBitmapQuick(t, 1000, 0, math.MaxInt64) } // Ensure a bitmap can perform basic operations on randomly generated values. func testBitmapQuick(t *testing.T, n int, min, max uint64) { @@ -1504,20 +1504,20 @@ func TestBitmap_Marshal_Quick_Array1(t *testing.T) { testBitmapMarshalQuick(t, 1000, 1000, 2000, false) } func TestBitmap_Marshal_Quick_Array2(t *testing.T) { - testBitmapMarshalQuick(t, 10000, 0, 1000, false) + testBitmapMarshalQuick(t, 1000, 0, 1000, false) } func TestBitmap_Marshal_Quick_Bitmap1(t *testing.T) { - testBitmapMarshalQuick(t, 10000, 0, 10000, false) + testBitmapMarshalQuick(t, 1000, 0, 10000, false) } func TestBitmap_Marshal_Quick_Bitmap2(t *testing.T) { - testBitmapMarshalQuick(t, 10000, 10000, 20000, false) + testBitmapMarshalQuick(t, 1000, 10000, 20000, false) } func TestBitmap_Marshal_Quick_LargeValue(t *testing.T) { testBitmapMarshalQuick(t, 100, 0, math.MaxInt64, false) } func TestBitmap_Marshal_Quick_Bitmap_Sorted(t *testing.T) { - testBitmapMarshalQuick(t, 10000, 0, 10000, true) + testBitmapMarshalQuick(t, 1000, 0, 10000, true) } // TODO update for RLE From 1460756b3f047cb82b0c19283bf20e5bbb6b9306 Mon Sep 17 00:00:00 2001 From: Seebs Date: Fri, 5 Jun 2020 12:24:07 -0500 Subject: [PATCH 2/8] Provide option for adjusting node timeouts, set it for tests. There's no reason to have 10-20 seconds of delays for testing this, because in testing, we're running things on the local machine and don't need to worry about significant network lag. Make retry count and delay settable options, and set them lower. Moves the Replica2 test in server/server_test.go from ~21s to ~2s. --- cluster.go | 25 +++++++++++++++---------- cluster_internal_test.go | 21 ++++++++++++++++----- server.go | 18 ++++++++++++++++++ test/pilosa.go | 14 +++++++------- 4 files changed, 56 insertions(+), 22 deletions(-) diff --git a/cluster.go b/cluster.go index b3fc86e88..eacad25f7 100644 --- a/cluster.go +++ b/cluster.go @@ -62,9 +62,8 @@ const ( resizeJobActionAdd = "ADD" resizeJobActionRemove = "REMOVE" - confirmDownRetries = 10 - confirmDownSleep = 1 - confirmDownTimeout = 2 + defaultConfirmDownRetries = 10 + defaultConfirmDownSleep = 1 * time.Second ) // Node represents a node in the cluster. @@ -239,6 +238,9 @@ type cluster struct { // nolint: maligned logger logger.Logger InternalClient InternalClient + + confirmDownRetries int + confirmDownSleep time.Duration } // newCluster returns a new instance of Cluster with defaults. @@ -258,6 +260,9 @@ func newCluster() *cluster { InternalClient: newNopInternalClient(), logger: logger.NopLogger, + + confirmDownRetries: defaultConfirmDownRetries, + confirmDownSleep: defaultConfirmDownSleep, } } @@ -1923,7 +1928,7 @@ func (c *cluster) considerTopology() error { // band aid to protect against false nodeLeave events from memberlist // the test is the lightest weight endpoint of the node in question /version // TODO provide more robust solution to false nodeLeave events -func confirmNodeDown(uri URI, log logger.Logger) bool { +func (c *cluster) confirmNodeDown(uri URI) bool { u := url.URL{ Scheme: uri.Scheme, Host: uri.HostPort(), @@ -1931,11 +1936,11 @@ func confirmNodeDown(uri URI, log logger.Logger) bool { } req, err := http.NewRequest("GET", u.String(), nil) if err != nil { - log.Printf("bad request:%s %s", u.String(), err) + c.logger.Printf("bad request:%s %s", u.String(), err) return false } - for i := 0; i < confirmDownRetries; i++ { - ctx, cancel := context.WithTimeout(context.Background(), confirmDownTimeout*time.Second) + for i := 0; i < c.confirmDownRetries; i++ { + ctx, cancel := context.WithTimeout(context.Background(), c.confirmDownSleep*2) defer cancel() resp, err := http.DefaultClient.Do(req.WithContext(ctx)) var bod []byte @@ -1946,8 +1951,8 @@ func confirmNodeDown(uri URI, log logger.Logger) bool { } } - log.Printf("NodeLeave confirm with %s %d. err: '%v' bod: '%s'", uri.HostPort(), i, err, bod) - time.Sleep(confirmDownSleep * time.Second) + c.logger.Printf("NodeLeave confirm with %s %d. err: '%v' bod: '%s'", uri.HostPort(), i, err, bod) + time.Sleep(c.confirmDownSleep) } return true } @@ -1975,7 +1980,7 @@ func (c *cluster) ReceiveEvent(e *NodeEvent) (err error) { // not already removed by a removeNode request. We treat this as the // host being temporarily unavailable, and expect it to come back // up. - if confirmNodeDown(e.Node.URI, c.logger) { + if c.confirmNodeDown(e.Node.URI) { if c.removeNodeBasicSorted(e.Node.ID) { c.Topology.nodeStates[e.Node.ID] = nodeStateDown // put the cluster into STARTING if we've lost a number of nodes diff --git a/cluster_internal_test.go b/cluster_internal_test.go index de87ad5ae..b9e714326 100644 --- a/cluster_internal_test.go +++ b/cluster_internal_test.go @@ -943,18 +943,22 @@ func TestCluster_confirmNodeDownUp(t *testing.T) { t.Error(err) } uri.Port = uint16(iport) - if confirmNodeDown(uri, logger.NewVerboseLogger(os.Stdout)) { + c := newCluster() + c.logger = logger.NewVerboseLogger(os.Stdout) + if c.confirmNodeDown(uri) { t.Errorf("expected node to be up") } } func TestCluster_confirmNodeDownTimeout(t *testing.T) { + sleep := 50 * time.Millisecond + retries := 5 if testing.Short() { t.Skip() } r := mux.NewRouter() r.HandleFunc("/version", http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - time.Sleep(confirmDownSleep * time.Second * confirmDownRetries) + time.Sleep(sleep * time.Duration(retries)) fmt.Fprintln(w, "ignored") })) server := httptest.NewServer(r) @@ -973,8 +977,11 @@ func TestCluster_confirmNodeDownTimeout(t *testing.T) { t.Error(err) } uri.Port = uint16(iport) - - if !confirmNodeDown(uri, logger.NewVerboseLogger(os.Stdout)) { + c := newCluster() + c.confirmDownSleep = sleep + c.confirmDownRetries = retries + c.logger = logger.NewVerboseLogger(os.Stdout) + if !c.confirmNodeDown(uri) { t.Errorf("expected node to be down") } } @@ -987,8 +994,12 @@ func TestCluster_confirmNodeDownDown(t *testing.T) { uri.Scheme = "http" uri.Host = "DoesntMatter" uri.Port = 6666 + c := newCluster() + c.confirmDownSleep = 50 * time.Millisecond + c.confirmDownRetries = 5 + c.logger = logger.NewVerboseLogger(os.Stdout) - if !confirmNodeDown(uri, logger.NewVerboseLogger(os.Stdout)) { + if !c.confirmNodeDown(uri) { t.Errorf("expected node to be down") } } diff --git a/server.go b/server.go index 9fbb30811..131a6971c 100644 --- a/server.go +++ b/server.go @@ -77,6 +77,8 @@ type Server struct { // nolint: maligned metricInterval time.Duration diagnosticInterval time.Duration maxWritesPerRequest int + confirmDownSleep time.Duration + confirmDownRetries int isCoordinator bool syncer holderSyncer @@ -229,6 +231,17 @@ func OptServerDiagnosticsInterval(dur time.Duration) ServerOption { } } +// OptServerNodeDownRetries is a functional option on Server +// used to specify the retries and sleep duration for node down +// checks. +func OptServerNodeDownRetries(retries int, sleep time.Duration) ServerOption { + return func(s *Server) error { + s.confirmDownRetries = retries + s.confirmDownSleep = sleep + return nil + } +} + // OptServerURI is a functional option on Server // used to set the server URI. func OptServerURI(uri *URI) ServerOption { @@ -330,6 +343,9 @@ func NewServer(opts ...ServerOption) (*Server, error) { metricInterval: 0, diagnosticInterval: 0, + confirmDownRetries: defaultConfirmDownRetries, + confirmDownSleep: defaultConfirmDownSleep, + resetTranslationSyncCh: make(chan struct{}), logger: logger.NopLogger, @@ -399,6 +415,8 @@ func NewServer(opts ...ServerOption) (*Server, error) { s.executor.MaxWritesPerRequest = s.maxWritesPerRequest s.cluster.broadcaster = s s.cluster.maxWritesPerRequest = s.maxWritesPerRequest + s.cluster.confirmDownRetries = s.confirmDownRetries + s.cluster.confirmDownSleep = s.confirmDownSleep s.holder.broadcaster = s err = s.loadAllExtensions() if err != nil { diff --git a/test/pilosa.go b/test/pilosa.go index 635e0f3d9..9b5f49a48 100644 --- a/test/pilosa.go +++ b/test/pilosa.go @@ -92,7 +92,7 @@ func NewCommandNode(isCoordinator bool, opts ...server.CommandOption) *Command { // We want tests to default to using the in-memory translate store, so we // prepend opts with that functional option. If a different translate store // has been specified, it will override this one. - opts = prependWithMemStore(opts) + opts = prependTestServerOpts(opts) m := newCommand(opts...) m.Config.Cluster.Disabled = false m.Config.Cluster.Coordinator = isCoordinator @@ -434,25 +434,25 @@ func MustRunCluster(tb testing.TB, size int, opts ...[]server.CommandOption) Clu return c } -// prependOpts applies prependWithMemStore to each of the ops (one per +// prependOpts applies prependTestServerOpts to each of the ops (one per // node, or one for the entire cluser). func prependOpts(opts [][]server.CommandOption) [][]server.CommandOption { if len(opts) == 0 { opts = [][]server.CommandOption{ - prependWithMemStore([]server.CommandOption{}), + prependTestServerOpts([]server.CommandOption{}), } } else { for i := range opts { - opts[i] = prependWithMemStore(opts[i]) + opts[i] = prependTestServerOpts(opts[i]) } } return opts } -// prependWithMemStore prepends opts with the OpenInMemTranslateStore. -func prependWithMemStore(opts []server.CommandOption) []server.CommandOption { +// prependTestServerOpts prepends opts with the OpenInMemTranslateStore. +func prependTestServerOpts(opts []server.CommandOption) []server.CommandOption { defaultOpts := []server.CommandOption{ - server.OptCommandServerOptions(pilosa.OptServerOpenTranslateStore(pilosa.OpenInMemTranslateStore)), + server.OptCommandServerOptions(pilosa.OptServerOpenTranslateStore(pilosa.OpenInMemTranslateStore), pilosa.OptServerNodeDownRetries(5, 100*time.Millisecond)), } return append(defaultOpts, opts...) } From 1952a43ed46b76cbcf00dd4ca5270a332b2be28a Mon Sep 17 00:00:00 2001 From: Seebs Date: Fri, 5 Jun 2020 12:29:32 -0500 Subject: [PATCH 3/8] Write fewer bits to test the rowcache behavior The failure mode in question was pretty predictable and tied to number of snapshots, not to number of bits written, so we can probably use a lot fewer bits and still get good results, but this is really slow under -race testing. --- fragment_internal_test.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/fragment_internal_test.go b/fragment_internal_test.go index b8a60d86a..9230519e7 100644 --- a/fragment_internal_test.go +++ b/fragment_internal_test.go @@ -111,6 +111,11 @@ func TestFragment_ClearBit(t *testing.T) { func TestFragment_RowcacheMap(t *testing.T) { var done int64 f := mustOpenFragment("i", "f", viewStandard, 0, "") + // Under -race, this test turns out to take a fairly long time + // to run with larger OpN, because we write 50,000 bits to + // the bitmap, and everything is being race-detected, and we don't + // actually need that many to get the result we care about. + f.MaxOpN = 2000 defer f.Clean(t) ch := make(chan struct{}) From 1484674a1c668d33ab9a3bfb1cefc172bf3925e0 Mon Sep 17 00:00:00 2001 From: Seebs Date: Fri, 5 Jun 2020 12:54:00 -0500 Subject: [PATCH 4/8] Add and use bitmap-to-slice-or-set comparison functions The generation of slices from things, and use of reflect.DeepEqual to compare the slices, is a lot more expensive than it needs to be. Omitting it removes most of the runtime of the marshal tests. --- roaring/roaring.go | 32 +++++++++++++++++++++++++++++++- roaring/roaring_test.go | 10 ++++------ 2 files changed, 35 insertions(+), 7 deletions(-) diff --git a/roaring/roaring.go b/roaring/roaring.go index 22d2821db..9a636a519 100644 --- a/roaring/roaring.go +++ b/roaring/roaring.go @@ -5756,7 +5756,37 @@ func xorBitmapRun(a, b *Container) *Container { return output } -// CompareEquality is used mostly in test cases to confirm that two bitmaps came +// CompareBitmapSlice checks whether a bitmap has the same values in it +// that a provided slice does. +func CompareBitmapSlice(b *Bitmap, vals []uint64) (bool, error) { + count := b.Count() + if count != uint64(len(vals)) { + return false, fmt.Errorf("length mismatch: bitmap has %d bits, slice has %d", count, len(vals)) + } + for _, v := range vals { + if !b.Contains(v) { + return false, fmt.Errorf("bitmap lacks expected value %d", v) + } + } + return true, nil +} + +// CompareBitmapMap checks whether a bitmap has the same values in it +// that a provided map[uint64]struct{} has as keys. +func CompareBitmapMap(b *Bitmap, vals map[uint64]struct{}) (bool, error) { + count := b.Count() + if count != uint64(len(vals)) { + return false, fmt.Errorf("length mismatch: bitmap has %d bits, map has %d", count, len(vals)) + } + for v := range vals { + if !b.Contains(v) { + return false, fmt.Errorf("bitmap lacks expected value %d", v) + } + } + return true, nil +} + +// BitwiseEqual is used mostly in test cases to confirm that two bitmaps came // out the same. It does not expect corresponding opN, or OpWriter, but expects // identical bit contents. It does not expect identical representations; a bitmap // container can be identical to an array container. It returns a boolean value, diff --git a/roaring/roaring_test.go b/roaring/roaring_test.go index e31cbf2ee..c8eb2617a 100644 --- a/roaring/roaring_test.go +++ b/roaring/roaring_test.go @@ -1570,14 +1570,12 @@ func testBitmapMarshalQuick(t *testing.T, n int, min, max uint64, sorted bool) { t.Fatal(err) } - // Verify the original bitmap has the correct set of values. - if exp, got := generator.Uint64SetSlice(set), bm.Slice(); !reflect.DeepEqual(exp, got) { - t.Fatalf("mismatch: %s\n\nexp=%+v\n\ngot=%+v\n\n", diff(exp, got), exp, got) + if _, err := roaring.CompareBitmapMap(bm, set); err != nil { + t.Fatalf("source mismatch: %v", err) } - // Verify the bitmap loaded with the ops log has the correct set of values. - if exp, got := generator.Uint64SetSlice(set), bm2.Slice(); !reflect.DeepEqual(exp, got) { - t.Fatalf("mismatch: %s\n\nexp=%+v\n\ngot=%+v\n\n", diff(exp, got), exp, got) + if _, err := roaring.CompareBitmapMap(bm2, set); err != nil { + t.Fatalf("unmarshalled mismatch: %v", err) } } From e223c79acef6e891d771ad0c8bf82811ca708c3c Mon Sep 17 00:00:00 2001 From: Seebs Date: Fri, 5 Jun 2020 13:07:16 -0500 Subject: [PATCH 5/8] Don't use a whole shard of values for Execute_All test. This is pretty expensive even for default shard width, and very expensive for ShardWidth = 1<<22, and we don't really get much extra benefit from having a million values instead of a hundred or so. --- executor_test.go | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/executor_test.go b/executor_test.go index dfb5d3af3..45389d2a0 100644 --- a/executor_test.go +++ b/executor_test.go @@ -3476,15 +3476,15 @@ func TestExecutor_Execute_All(t *testing.T) { t.Fatal(err) } - // Create an import request that sets a full shard, - // plus a couple bits set on either side of it, and - // a final bit set in a fourth shard. + // Create an import request that sets things on either end + // of a shard, plus a couple bits set on either side of it, + // and a final bit set in a fourth shard. // // shard0 shard1 shard2 shard3 // |----------|----------|----------|----------| - // | **|**********|** | * + // | **|** **|** | * // - bitCount := ShardWidth + 5 + bitCount := 100 + 5 req := &pilosa.ImportRequest{ Index: index.Name(), Field: fld.Name(), @@ -3492,10 +3492,14 @@ func TestExecutor_Execute_All(t *testing.T) { RowIDs: make([]uint64, bitCount), ColumnIDs: make([]uint64, bitCount), } - for i := 0; i < bitCount-1; i++ { + for i := 0; i < bitCount/2; i++ { req.RowIDs[i] = 10 req.ColumnIDs[i] = uint64(i + ShardWidth - 2) } + for i := bitCount / 2; i < bitCount-1; i++ { + req.RowIDs[i] = 10 + req.ColumnIDs[i] = uint64(i + (ShardWidth * 2) - bitCount + 5) + } req.RowIDs[bitCount-1] = 10 req.ColumnIDs[bitCount-1] = uint64((3 * ShardWidth) + 2) @@ -3521,7 +3525,7 @@ func TestExecutor_Execute_All(t *testing.T) { {qry: fmt.Sprintf("All(limit=2, offset=%d)", bitCount-5), expCols: req.ColumnIDs[bitCount-5 : bitCount-3], expCnt: 2}, {qry: "All(limit=2, offset=2)", expCols: req.ColumnIDs[2:4], expCnt: 2}, {qry: "All(limit=1, offset=1)", expCols: req.ColumnIDs[1:2], expCnt: 1}, - {qry: fmt.Sprintf("All(limit=%d, offset=2)", ShardWidth), expCols: req.ColumnIDs[2 : bitCount-3], expCnt: ShardWidth}, + {qry: fmt.Sprintf("All(limit=%d, offset=2)", bitCount-3), expCols: req.ColumnIDs[2 : bitCount-1], expCnt: uint64(bitCount - 3)}, } for i, test := range tests { if res, err := c[0].API.Query(context.Background(), &pilosa.QueryRequest{Index: "i", Query: test.qry}); err != nil { From 3f0c9925f4aa88a2b9e893c9027594f14267008e Mon Sep 17 00:00:00 2001 From: Seebs Date: Fri, 5 Jun 2020 13:09:33 -0500 Subject: [PATCH 6/8] Don't test quite so many values for BtreeSeek and BtreeDelete BtreeSeek is O(N^2) on its N, and there's not a ton of extra utility to testing a larger range of values, so we reduce N by a bit, cutting runtime from ~10s to <1s on my laptop. Also reduce the scale of the BtreeDelete1/BtreeDelete2 tests a bit because, again, lots of runtime for little marginal information. --- roaring/btree_test.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/roaring/btree_test.go b/roaring/btree_test.go index c3c02af3d..f0f528a7b 100644 --- a/roaring/btree_test.go +++ b/roaring/btree_test.go @@ -689,7 +689,7 @@ func TestBtreeDelete0(t *testing.T) { } func TestBtreeDelete1(t *testing.T) { - const N = 130000 + const N = 13000 for _, x := range []int{0, -1, 0x555555, 0xaaaaaa, 0x333333, 0xcccccc, 0x314159} { r := treeNew() set := r.Set @@ -788,7 +788,7 @@ func benchmarkDelRnd(b *testing.B, n int) { } func TestBtreeDelete2(t *testing.T) { - const N = 100000 + const N = 10000 for _, x := range []int{0, -1, 0x555555, 0xaaaaaa, 0x333333, 0xcccccc, 0x314159} { r := treeNew() set := r.Set @@ -1468,7 +1468,7 @@ func TestBtreePut(t *testing.T) { } func TestBtreeSeek(t *testing.T) { - const N = 1 << 13 + const N = 1 << 11 tr := treeNew() for i := 0; i < N; i++ { k := 2*i + 1 From 52aa3e2e23338ca184970190c0341805f9acebe8 Mon Sep 17 00:00:00 2001 From: Seebs Date: Fri, 5 Jun 2020 13:38:33 -0500 Subject: [PATCH 7/8] Improve container/bitmap comparison logic for testing We have a "deadcode" bitmapsEqual which is actually used in testing but probably shouldn't be, and we don't have a good container equality test. Problem is, equality tests are sort of slow in the things-are-equal case, which is the most common case, so we've got some moderately-specialized code here; specifically, special comparison code that takes advantage of knowing that if two containers have the same number of bits, you only have to check whether all the bits from one are present in the other, because that can't be true for differing containers with the same number of bits. This reduces the runtime for the ContainerCombinations case from about 24 seconds to a bit under 2 on my laptop, or from around 10 minutes to about 37 seconds with the race detector on. Also simplify the InPlaceWrapper functions not to invoke bitmaps, because it's not really necessary. --- roaring/roaring.go | 154 +++++++++++++++++-------------- roaring/roaring_helpers_test.go | 89 ++++++++++-------- roaring/roaring_internal_test.go | 103 ++++++++------------- roaring/roaring_test.go | 12 --- 4 files changed, 169 insertions(+), 189 deletions(-) diff --git a/roaring/roaring.go b/roaring/roaring.go index 9a636a519..db63b0edd 100644 --- a/roaring/roaring.go +++ b/roaring/roaring.go @@ -4552,50 +4552,95 @@ func (c *Container) bitmapZeroRange(i, j uint64) { c.setN(n) } -// equals reports whether two containers are equal. -func (c *Container) equals(c2 *Container) bool { - if c == nil || c2 == nil { - if c != c2 { - return false +func typePair(ct1, ct2 byte) int { + return int((ct1 << 4) | ct2) +} + +// compareArrayBitmap actually only verifies that everything in the array +// is in the bitmap. It's used only after comparing the N for the containers, +// so if there's anything in the bitmap that's not in the array, either there's +// something in the array that's not in the bitmap, or we didn't get here. +func compareArrayBitmap(a []uint16, b []uint64) error { + for _, v := range a { + w, bit := b[v>>6], v&63 + if w>>bit&1 == 0 { + return fmt.Errorf("value %d missing", v) } } - if c.Mapped() != c2.Mapped() || c.typ() != c2.typ() || c.N() != c2.N() { - return false + return nil +} + +// compareArrayRuns determines whether an array matches a provided +// set of runs. As with compareArrayBitmap, it only verifies presence +// of the array's values in the run collection. the run collection +// can't be empty; if it were, N would have been 0, and we wouldn't +// have gotten here. +func compareArrayRuns(a []uint16, r []interval16) error { + ri := 0 + ru := r[ri] + ri++ + for _, v := range a { + if v < ru.start { + return fmt.Errorf("value %d missing", v) + } + if v > ru.last { + if ri >= len(r) { + return fmt.Errorf("value %d missing", v) + } + ru = r[ri] + ri++ + // if they're identical, the array value must be + // the start of the next run. + if v != ru.start { + return fmt.Errorf("value %d missing", v) + } + } } - if c.typ() == containerArray { - ca, c2a := c.array(), c2.array() - if len(ca) != len(c2a) { - return false - } - for i := 0; i < len(ca); i++ { - if ca[i] != c2a[i] { - return false - } - } - } else if c.typ() == containerBitmap { - cb, c2b := c.bitmap(), c2.bitmap() - if len(cb) != len(c2b) { - return false - } - for i := 0; i < len(cb); i++ { - if cb[i] != c2b[i] { - return false - } - } - } else if c.typ() == containerRun { - cr, c2r := c.runs(), c2.runs() - if len(cr) != len(c2r) { - return false - } - for i := 0; i < len(cr); i++ { - if cr[i] != c2r[i] { - return false - } - } - } else { - panic(fmt.Sprintf("unknown container type: %v", c.typ())) + return nil +} + +// compareArrayArray reports whether everything in a1 is equal to everything +// in a2. +func compareArrayArray(a1, a2 []uint16) error { + if len(a1) != len(a2) { + return fmt.Errorf("unexpected length mismatch, %d vs %d", len(a1), len(a2)) } - return true + for i := range a1 { + if a1[i] != a2[i] { + return fmt.Errorf("item %d: %d vs %d", i, a1[i], a2[i]) + } + } + return nil +} + +// BitwiseCompare reports whether two containers are equal. It returns +// an error describing any difference it finds. This is mostly intended +// for use in tests that expect equality. +func (c *Container) BitwiseCompare(c2 *Container) error { + if c.N() != c2.N() { + return errors.New("containers are different lengths") + } + if c.N() == 0 { + return nil + } + switch typePair(c.typ(), c2.typ()) { + case typePair(containerArray, containerArray): + return compareArrayArray(c.array(), c2.array()) + case typePair(containerArray, containerBitmap): + return compareArrayBitmap(c.array(), c2.bitmap()) + case typePair(containerBitmap, containerArray): + return compareArrayBitmap(c2.array(), c.bitmap()) + case typePair(containerArray, containerRun): + return compareArrayRuns(c.array(), c2.runs()) + case typePair(containerRun, containerArray): + return compareArrayRuns(c2.array(), c.runs()) + default: + c3 := xor(c, c2) + if c3.N() != 0 { + return fmt.Errorf("%d bits differenct between containers", c3.N()) + } + } + return nil } func unionArrayBitmap(a, b *Container) *Container { @@ -5855,35 +5900,6 @@ func (b *Bitmap) BitwiseEqual(c *Bitmap) (bool, error) { return true, nil } -func bitmapsEqual(b, c *Bitmap) error { // nolint: deadcode - statsHit("bitmapsEqual") - if b.OpWriter != c.OpWriter { - return errors.New("opWriters not equal") - } - if b.opN != c.opN { - return errors.New("opNs not equal") - } - - biter, _ := b.Containers.Iterator(0) - citer, _ := c.Containers.Iterator(0) - bn, cn := biter.Next(), citer.Next() - for ; bn && cn; bn, cn = biter.Next(), citer.Next() { - bk, bc := biter.Value() - ck, cc := citer.Value() - if bk != ck { - return errors.New("keys not equal") - } - if !bc.equals(cc) { - return errors.New("containers not equal") - } - } - if bn && !cn || cn && !bn { - return errors.New("different numbers of containers") - } - - return nil -} - func popcount(x uint64) uint64 { return uint64(bits.OnesCount64(x)) } diff --git a/roaring/roaring_helpers_test.go b/roaring/roaring_helpers_test.go index 41d4059fb..f8de764fb 100644 --- a/roaring/roaring_helpers_test.go +++ b/roaring/roaring_helpers_test.go @@ -14,6 +14,8 @@ package roaring +import "sync" + /////////////////////////////////////////////////////////////////////////// var containerWidth uint64 = 65536 @@ -261,51 +263,56 @@ func doContainer(typ byte, data interface{}) *Container { return nil } +var makeCts sync.Once +var sampleTestContainers map[byte]map[string]*Container + func setupContainerTests() map[byte]map[string]*Container { - cts := make(map[byte]map[string]*Container) + makeCts.Do(func() { + sampleTestContainers = make(map[byte]map[string]*Container) - // array containers - cts[containerArray] = map[string]*Container{ - "empty": doContainer(containerArray, arrayEmpty()), - "full": doContainer(containerArray, arrayFull()), - "firstBitSet": doContainer(containerArray, arrayFirstBitSet()), - "lastBitSet": doContainer(containerArray, arrayLastBitSet()), - "firstBitUnset": doContainer(containerArray, arrayFirstBitUnset()), - "lastBitUnset": doContainer(containerArray, arrayLastBitUnset()), - "innerBitsSet": doContainer(containerArray, arrayInnerBitsSet()), - "outerBitsSet": doContainer(containerArray, arrayOuterBitsSet()), - "oddBitsSet": doContainer(containerArray, arrayOddBitsSet()), - "evenBitsSet": doContainer(containerArray, arrayEvenBitsSet()), - } + // array containers + sampleTestContainers[containerArray] = map[string]*Container{ + "empty": doContainer(containerArray, arrayEmpty()), + "full": doContainer(containerArray, arrayFull()), + "firstBitSet": doContainer(containerArray, arrayFirstBitSet()), + "lastBitSet": doContainer(containerArray, arrayLastBitSet()), + "firstBitUnset": doContainer(containerArray, arrayFirstBitUnset()), + "lastBitUnset": doContainer(containerArray, arrayLastBitUnset()), + "innerBitsSet": doContainer(containerArray, arrayInnerBitsSet()), + "outerBitsSet": doContainer(containerArray, arrayOuterBitsSet()), + "oddBitsSet": doContainer(containerArray, arrayOddBitsSet()), + "evenBitsSet": doContainer(containerArray, arrayEvenBitsSet()), + } - // bitmap containers - cts[containerBitmap] = map[string]*Container{ - "empty": doContainer(containerBitmap, bitmapEmpty()), - "full": doContainer(containerBitmap, bitmapFull()), - "firstBitSet": doContainer(containerBitmap, bitmapFirstBitSet()), - "lastBitSet": doContainer(containerBitmap, bitmapLastBitSet()), - "firstBitUnset": doContainer(containerBitmap, bitmapFirstBitUnset()), - "lastBitUnset": doContainer(containerBitmap, bitmapLastBitUnset()), - "innerBitsSet": doContainer(containerBitmap, bitmapInnerBitsSet()), - "outerBitsSet": doContainer(containerBitmap, bitmapOuterBitsSet()), - "oddBitsSet": doContainer(containerBitmap, bitmapOddBitsSet()), - "evenBitsSet": doContainer(containerBitmap, bitmapEvenBitsSet()), - } + // bitmap containers + sampleTestContainers[containerBitmap] = map[string]*Container{ + "empty": doContainer(containerBitmap, bitmapEmpty()), + "full": doContainer(containerBitmap, bitmapFull()), + "firstBitSet": doContainer(containerBitmap, bitmapFirstBitSet()), + "lastBitSet": doContainer(containerBitmap, bitmapLastBitSet()), + "firstBitUnset": doContainer(containerBitmap, bitmapFirstBitUnset()), + "lastBitUnset": doContainer(containerBitmap, bitmapLastBitUnset()), + "innerBitsSet": doContainer(containerBitmap, bitmapInnerBitsSet()), + "outerBitsSet": doContainer(containerBitmap, bitmapOuterBitsSet()), + "oddBitsSet": doContainer(containerBitmap, bitmapOddBitsSet()), + "evenBitsSet": doContainer(containerBitmap, bitmapEvenBitsSet()), + } - // run containers - cts[containerRun] = map[string]*Container{ - "empty": doContainer(containerRun, runEmpty()), - "full": doContainer(containerRun, runFull()), - "firstBitSet": doContainer(containerRun, runFirstBitSet()), - "lastBitSet": doContainer(containerRun, runLastBitSet()), - "firstBitUnset": doContainer(containerRun, runFirstBitUnset()), - "lastBitUnset": doContainer(containerRun, runLastBitUnset()), - "innerBitsSet": doContainer(containerRun, runInnerBitsSet()), - "outerBitsSet": doContainer(containerRun, runOuterBitsSet()), - "oddBitsSet": doContainer(containerRun, runOddBitsSet()), - "evenBitsSet": doContainer(containerRun, runEvenBitsSet()), - } + // run containers + sampleTestContainers[containerRun] = map[string]*Container{ + "empty": doContainer(containerRun, runEmpty()), + "full": doContainer(containerRun, runFull()), + "firstBitSet": doContainer(containerRun, runFirstBitSet()), + "lastBitSet": doContainer(containerRun, runLastBitSet()), + "firstBitUnset": doContainer(containerRun, runFirstBitUnset()), + "lastBitUnset": doContainer(containerRun, runLastBitUnset()), + "innerBitsSet": doContainer(containerRun, runInnerBitsSet()), + "outerBitsSet": doContainer(containerRun, runOuterBitsSet()), + "oddBitsSet": doContainer(containerRun, runOddBitsSet()), + "evenBitsSet": doContainer(containerRun, runEvenBitsSet()), + } + }) - return cts + return sampleTestContainers } diff --git a/roaring/roaring_internal_test.go b/roaring/roaring_internal_test.go index 7784064ab..3a0749cc8 100644 --- a/roaring/roaring_internal_test.go +++ b/roaring/roaring_internal_test.go @@ -2632,7 +2632,7 @@ func TestBitmapClone(t *testing.T) { } } c := b.Clone() - if err := bitmapsEqual(b, c); err != nil { + if _, err := b.BitwiseEqual(c); err != nil { t.Fatalf("Clone Objects not equal: %v\n", err) } d := func() *Bitmap { //anybody know how to declare a nil value? @@ -2735,34 +2735,46 @@ func getFunctionName(i interface{}) string { return y[0] } -// UnionInPlace is defined at the Bitmap level, but this wrapper lets us insert -// it into our ContainerCombinations tests so that it gets exercised on a wide -// variety of container data. func unionInPlaceWrapper(a, b *Container) *Container { - out := NewBitmap() - out.Containers.Put(0, a.Clone()) - B := NewBitmap() - B.Containers.Put(0, b) - out.UnionInPlace(B) - return out.Containers.Get(0) + ret := a.Clone().unionInPlace(b) + ret.Repair() + return ret } func differenceInPlaceWrapper(a, b *Container) *Container { - out := NewBitmap() - out.Containers.Put(0, a.Clone()) - B := NewBitmap() - B.Containers.Put(0, b) - out.DifferenceInPlace(B) - return out.Containers.Get(0) + a = a.Clone() + // this should probably return its new value, but currently does not + a.differenceInPlace(b) + return a } func intersectInPlaceWrapper(a, b *Container) *Container { - out := NewBitmap() - out.Containers.Put(0, a.Clone()) - B := NewBitmap() - B.Containers.Put(0, b) - out.IntersectInPlace(B) - return out.Containers.Get(0) + return a.Clone().intersectInPlace(b) +} + +func TestContainerBitwiseCompare(t *testing.T) { + cts := setupContainerTests() + + for t1, containers := range cts { + for name, c := range containers { + for t2, other := range cts { + for otherName, otherC := range other { + err := c.BitwiseCompare(otherC) + if err != nil { + if otherName == name { + t.Fatalf("container types %d/%d, contents %s: unexpected error %v", + t1, t2, name, err) + } + } else { + if name != otherName { + t.Fatalf("container types %d/%d, unexpected %s == %s", + t1, t2, name, otherName) + } + } + } + } + } + } } func TestContainerCombinations(t *testing.T) { @@ -3562,51 +3574,8 @@ func TestContainerCombinations(t *testing.T) { // Convert to all container types and check result. for _, ct := range containerTypes { - clone := ret.Clone() - if ct == containerArray { - if clone == nil { - clone = NewContainerArray(nil) - } else if clone.isBitmap() { - clone = clone.bitmapToArray() - } else if clone.isRun() { - clone = clone.runToArray() - } - if clone.N() != cts[ct][exp].N() { - t.Errorf("test %s expected array n=%d, but got n=%d", desc, cts[ct][exp].N(), clone.N()) - } - // Because xorRunRun resulting in an empty container returns an array container with a - // nil slice array, then we need to check len() on array first (look for 0). - if !(len(clone.array()) == 0 && len(cts[ct][exp].array()) == 0) && !reflect.DeepEqual(clone.array(), cts[ct][exp].array()) { - t.Errorf("test %s expected array %X, but got %X", desc, cts[ct][exp].array(), clone.array()) - } - } else if ct == containerBitmap { - if clone == nil { - clone = NewContainerBitmap(0, nil) - } else if clone.isArray() { - clone = clone.arrayToBitmap() - } else if clone.isRun() { - clone = clone.runToBitmap() - } - if clone.N() != cts[ct][exp].N() { - t.Errorf("test %s expected bitmap n=%d, but got n=%d", desc, cts[ct][exp].N(), clone.N()) - } - if !reflect.DeepEqual(clone.bitmap(), cts[ct][exp].bitmap()) { - t.Errorf("test %s expected bitmap %X, but got %X", desc, cts[ct][exp].bitmap(), clone.bitmap()) - } - } else if ct == containerRun { - if clone == nil { - clone = NewContainerRun(nil) - } else if clone.isArray() { - clone = clone.arrayToRun(0) - } else if clone.isBitmap() { - clone = clone.bitmapToRun(0) - } - if clone.N() != cts[ct][exp].N() { - t.Errorf("test %s expected runs n=%d, but got n=%d", desc, cts[ct][exp].N(), clone.N()) - } - if !reflect.DeepEqual(clone.runs(), cts[ct][exp].runs()) { - t.Errorf("test %s expected runs %X, but got %X", desc, cts[ct][exp].runs(), clone.runs()) - } + if err := ret.BitwiseCompare(cts[ct][exp]); err != nil { + t.Errorf("test %s: %v", desc, err) } } } diff --git a/roaring/roaring_test.go b/roaring/roaring_test.go index c8eb2617a..55395ea73 100644 --- a/roaring/roaring_test.go +++ b/roaring/roaring_test.go @@ -1837,18 +1837,6 @@ func getBenchData(tb testing.TB) *benchmarkSampleData { return data } -func diff(a, b []uint64) string { - if len(a) != len(b) { - return fmt.Sprintf("len: %d != %d", len(a), len(b)) - } - for i := range a { - if a[i] != b[i] { - return fmt.Sprintf("index %d: %d != %d", i, a[i], b[i]) - } - } - return "" -} - func TestBitmap_Intersect(t *testing.T) { bm0 := testBM() result := bm0.Intersect(bm0) From 44569fa21090a08b3ca692bdc8c48d71d45e19c2 Mon Sep 17 00:00:00 2001 From: Seebs Date: Fri, 5 Jun 2020 13:54:17 -0500 Subject: [PATCH 8/8] Reduce iterations in TestFragment_RowsIteration We don't really learn more from trying every multiple of 10,000 than we do from trying maybe 32 values, and it's worse at larger shard widths. --- fragment_internal_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/fragment_internal_test.go b/fragment_internal_test.go index 9230519e7..2a554bfc1 100644 --- a/fragment_internal_test.go +++ b/fragment_internal_test.go @@ -2760,9 +2760,9 @@ func TestFragment_RowsIteration(t *testing.T) { defer f.Clean(t) expectedRows := make([]uint64, 0) - for r := uint64(1); r < uint64(10000); r += 100 { + for r := uint64(1); r < uint64(10000); r += 250 { expectedRows = append(expectedRows, r) - for c := uint64(1); c < uint64(ShardWidth-1); c += 10000 { + for c := uint64(1); c < uint64(ShardWidth-1); c += (ShardWidth >> 5) { if _, err := f.setBit(r, c); err != nil { t.Fatal(err) }