From 67e3dc4a089af02574b92005f2ab0ac5e21b92f7 Mon Sep 17 00:00:00 2001 From: Seebs Date: Mon, 26 Nov 2018 13:10:21 -0600 Subject: [PATCH 1/5] roaring: improve SliceAscending/SliceDescending tests Two changes: First, make SliceDescending set the entire slice, not all-but-one bits. Second, add tests that are "striped", so it's writing to 8 parts of the slice sequentially, rather than just going up or down the whole thing, because that gives us some cheap indication of cache-locality impact, which turns out to be possibly significant. --- roaring/roaring_test.go | 35 +++++++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/roaring/roaring_test.go b/roaring/roaring_test.go index f5c016ea3..a55393f1b 100644 --- a/roaring/roaring_test.go +++ b/roaring/roaring_test.go @@ -1348,5 +1348,40 @@ func BenchmarkSliceDescending(b *testing.B) { for col := uint64(pilosa.ShardWidth); col > uint64(0); col-- { bm.Add(col) } + bm.Add(0) + } +} + +func BenchmarkSliceAscendingStriped(b *testing.B) { + for n := 0; n < b.N; n++ { + bm := roaring.NewFileBitmap() + l := uint64(pilosa.ShardWidth / 8) + for col := uint64(0); col < l; col++ { + bm.Add(l*0 + col) + bm.Add(l*1 + col) + bm.Add(l*2 + col) + bm.Add(l*3 + col) + bm.Add(l*4 + col) + bm.Add(l*5 + col) + bm.Add(l*6 + col) + bm.Add(l*7 + col) + } + } +} + +func BenchmarkSliceDescendingStriped(b *testing.B) { + for n := 0; n < b.N; n++ { + bm := roaring.NewFileBitmap() + l := uint64(pilosa.ShardWidth / 8) + for col := uint64(l); col < l+1; col-- { + bm.Add(l*7 + col) + bm.Add(l*6 + col) + bm.Add(l*5 + col) + bm.Add(l*4 + col) + bm.Add(l*3 + col) + bm.Add(l*2 + col) + bm.Add(l*1 + col) + bm.Add(l*0 + col) + } } } From 3f6c17f4338996f28d65b3c51d20ad217d5cc8f0 Mon Sep 17 00:00:00 2001 From: Seebs Date: Mon, 26 Nov 2018 13:10:26 -0600 Subject: [PATCH 2/5] roaring: use DirectAdd rather than op.apply for cheap performance win MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Calling op.apply on an op we know to be an add ends up noticably increasing the cost of the operation; this trivial change gets about a 5-10% reduction in reported runtime of benchmarks doing a lot of adds. (The other IntersectionCount benchmarks don't actually use Add most of the time, so it doesn't show up in them.) name old time/op new time/op delta GetBenchData-8 4.25ms ± 0% 3.91ms ± 2% -8.04% (p=0.002 n=6+6) Bitmap_IntersectionCount_ArrayArray-8 20.9µs ± 2% 18.7µs ± 3% -10.18% (p=0.004 n=5+6) SliceAscending-8 24.7ms ± 0% 21.8ms ± 0% -11.74% (p=0.004 n=5+6) SliceDescending-8 29.8ms ± 0% 27.0ms ± 0% -9.56% (p=0.004 n=5+6) SliceAscendingStriped-8 32.0ms ± 0% 29.5ms ± 0% -8.07% (p=0.008 n=5+5) SliceDescendingStriped-8 39.3ms ± 1% 36.8ms ± 1% -6.27% (p=0.002 n=6+6) --- roaring/roaring.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/roaring/roaring.go b/roaring/roaring.go index 9c4274df9..c0d4cb996 100644 --- a/roaring/roaring.go +++ b/roaring/roaring.go @@ -159,9 +159,8 @@ func (b *Bitmap) Add(a ...uint64) (changed bool, err error) { } // Apply to the in-memory bitmap. - if op.apply(b) { + if b.DirectAdd(v) { changed = true - } } From 44f53a5f1de9af5793e1017b12ae4b510fa9cb75 Mon Sep 17 00:00:00 2001 From: Travis Turner Date: Mon, 14 Jan 2019 16:14:27 -0600 Subject: [PATCH 3/5] raise an error on Rows() query against a time field with noStandardView:true --- executor.go | 8 ++++++++ executor_test.go | 11 +++++++++++ 2 files changed, 19 insertions(+) diff --git a/executor.go b/executor.go index f68fec726..dffb0702e 100644 --- a/executor.go +++ b/executor.go @@ -1138,6 +1138,14 @@ func (e *executor) executeRowsShard(_ context.Context, index string, c *pql.Call if f == nil { return nil, ErrFieldNotFound } + + // Rows query does not currently support a `time` field that has + // `noStandardView: true`. + // TODO https://github.com/pilosa/pilosa/issues/1783 + if f.Type() == FieldTypeTime && f.options.NoStandardView { + return nil, errors.New("Rows() query on time field with no standard view is not supported") + } + frag := e.Holder.fragment(index, fieldName, viewStandard, shard) if frag == nil { return make(RowIDs, 0), nil diff --git a/executor_test.go b/executor_test.go index 0d42af8c5..950dc40a1 100644 --- a/executor_test.go +++ b/executor_test.go @@ -3059,6 +3059,17 @@ func TestExecutor_Execute_Rows(t *testing.T) { } } +func TestExecutor_Execute_RowsTime(t *testing.T) { + c := test.MustRunCluster(t, 1) + defer c.Close() + c.CreateField(t, "i", pilosa.IndexOptions{}, "t", pilosa.OptFieldTypeTime(pilosa.TimeQuantum("YMD"), true)) + + exp := "executing: Rows() query on time field with no standard view is not supported" + if _, err := c[0].API.Query(context.Background(), &pilosa.QueryRequest{Index: "i", Query: `Rows(field=t)`}); err == nil || err.Error() != exp { + t.Fatalf("expected error: %s", exp) + } +} + func TestExecutor_Execute_Query_Error(t *testing.T) { c := test.MustRunCluster(t, 1) defer c.Close() From 6f21eb32de0d7d2924cabbda292d20fd59f4bab3 Mon Sep 17 00:00:00 2001 From: Matthew Jaffee Date: Thu, 17 Jan 2019 14:47:35 -0600 Subject: [PATCH 4/5] Apply suggestions from code review Co-Authored-By: travisturner --- executor.go | 2 +- executor_test.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/executor.go b/executor.go index dffb0702e..940120ec5 100644 --- a/executor.go +++ b/executor.go @@ -1143,7 +1143,7 @@ func (e *executor) executeRowsShard(_ context.Context, index string, c *pql.Call // `noStandardView: true`. // TODO https://github.com/pilosa/pilosa/issues/1783 if f.Type() == FieldTypeTime && f.options.NoStandardView { - return nil, errors.New("Rows() query on time field with no standard view is not supported") + return nil, errors.New("Rows() query on time field with no standard view is not currently supported") } frag := e.Holder.fragment(index, fieldName, viewStandard, shard) diff --git a/executor_test.go b/executor_test.go index 950dc40a1..3716e625f 100644 --- a/executor_test.go +++ b/executor_test.go @@ -3064,7 +3064,7 @@ func TestExecutor_Execute_RowsTime(t *testing.T) { defer c.Close() c.CreateField(t, "i", pilosa.IndexOptions{}, "t", pilosa.OptFieldTypeTime(pilosa.TimeQuantum("YMD"), true)) - exp := "executing: Rows() query on time field with no standard view is not supported" + exp := "executing: Rows() query on time field with no standard view is not currently supported" if _, err := c[0].API.Query(context.Background(), &pilosa.QueryRequest{Index: "i", Query: `Rows(field=t)`}); err == nil || err.Error() != exp { t.Fatalf("expected error: %s", exp) } From fc1de2dba9074176d687d944e7dbae6e5fed5300 Mon Sep 17 00:00:00 2001 From: Travis Turner Date: Fri, 18 Jan 2019 14:49:58 -0600 Subject: [PATCH 5/5] cluster.Nodes() just needs a read lock --- cluster.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cluster.go b/cluster.go index e966c57a1..cbdb9f547 100644 --- a/cluster.go +++ b/cluster.go @@ -609,8 +609,8 @@ func (c *cluster) addNodeBasicSorted(node *Node) bool { // Nodes returns a copy of the slice of nodes in the cluster. Safe for // concurrent use, result may be modified. func (c *cluster) Nodes() []*Node { - c.mu.Lock() - defer c.mu.Unlock() + c.mu.RLock() + defer c.mu.RUnlock() ret := make([]*Node, len(c.nodes)) copy(ret, c.nodes) return ret