Repository: blevesearch/bleve
Pull request #1438 (2020-08-13 01:23:50)

MB-40916: Do not recycle unadorned term field readers for optimized conjunctions/disjunctions
When a conjunction or a disjunction query is eligible to be optimized
to conjunctionUnadorned or disjunctionUnadorned, roaring.Bitmaps are
created by AND-ing or OR-ing the individual bitmaps. These'll need
to be released - for GC.

abhinavdangeti: Hey @mschoch , let me know your thoughts on the latest patch set. Cheers.
sreekanth-cb: For the optimised TFR, not resetting the iterator's slice of before adding them back for recycling is sub optimal and then it also brings us back to the original point of effective re usability here. 

Looks like we aren't re cycling much stuffs here with optimised TFRs and making it to follow the current design of reusability is throwing an equal amount of design/layering concerns..

mschoch: @sreekanth-cb agreed.  I now think we just shouldn't recycle these.  So, what is the cleanest way to accomplish that without it's own set of design/layering concerns.
sreekanth-cb: @mschoch , IndexSnapshotTermFieldReader's Close() implementation deciding it's own re-usability isn't looking that bad to me.  We aren't touching the older style of TFR reusability, rather TFRs are smarter enough to just don't stack these special cases for reuse.

For example, having an extra heuristic (as we just thrown away one :) ) decider at line: 183.5 at snapshot_index_tfr.go 
https://github.com/blevesearch/bleve/blob/master/index/scorch/snapshot_index_tfr.go#L183

i.isReusable() / anyBetterNamedFunc() { 
   i.snapshot.recycleTermFieldReader(i)
}

But I know, you already disliked this idea earlier. So essentially I don't have any other cleaner idea.. 

mschoch: So, to me, adding another boolean to the struct reinforces the reality that you have 2 different types hiding inside one Go type.  Additionally, the optimized case has uninitialized fields (dicts, postings).  These are intermediate structures used in the creation of a normal TFR, that aid in faster reuse, but do not exist in the optimized case.

A cleaner solution would be if a TFR was recyclable or not based on some internal property.  For example, a clean solution (that probably won't work for other reasons) would be `if i.snapshot != nil { /* then recycle */ }`  That doesn't work in practice because the snapshot is checked for other purposes during optimization itself, but it would be cleaner because the snapshot is actually required to perform the recycling, so the check makes sense even without knowing anything about optimized TFRs.

The recycling could key on the nil dicts and postings, but I don't like that either as that seems a consequence of the way the optimized TFR is built, not some essential property of recycling.

I don't have a suggestion yet, just sharing my thinking.
mschoch: OK, how about this:

We add a `recycle bool` to the TFR struct, and the only place you're allowed to set it to true is inside: `func (i *IndexSnapshot) allocTermFieldReaderDicts(field string)`  We can't enforce that, but the idea being that recycle is always false, unless you got it from the code managing the pool.
abhinavdangeti: Ya that sounds ok to me. So this will take us back to adding a conditional to Close() before recycling, but I agree the - recycle boolean flag looks a little more neater with it being set only within allocTermFieldReaderDicts.

Patch:
diff --git a/go.mod b/go.mod
index 2b23a510..09bf5373 100644
--- a/go.mod
+++ b/go.mod
@@ -12,7 +12,6 @@ require (
 	github.com/blevesearch/zap/v12 v12.0.9
 	github.com/blevesearch/zap/v13 v13.0.1
 	github.com/blevesearch/zap/v14 v14.0.0
-	github.com/couchbase/ghistogram v0.1.0 // indirect
 	github.com/couchbase/moss v0.1.0
 	github.com/couchbase/vellum v1.0.1
 	github.com/golang/protobuf v1.3.2
diff --git a/index/scorch/optimize.go b/index/scorch/optimize.go
index f23fdfb8..b3d736f7 100644
--- a/index/scorch/optimize.go
+++ b/index/scorch/optimize.go
@@ -44,7 +44,7 @@ func (s *IndexSnapshotTermFieldReader) Optimize(kind string,
 		return s.optimizeDisjunctionUnadorned(octx, 0)
 	}
 
-	return octx, nil
+	return nil, nil
 }
 
 var OptimizeDisjunctionUnadornedMinChildCardinality = uint64(256)
@@ -165,16 +165,8 @@ func (o *OptimizeTFRConjunctionUnadorned) Finish() (rv index.Optimized, err erro
 
 	// We use an artificial term and field because the optimized
 	// termFieldReader can represent multiple terms and fields.
-	oTFR := &IndexSnapshotTermFieldReader{
-		term:               OptimizeTFRConjunctionUnadornedTerm,
-		field:              OptimizeTFRConjunctionUnadornedField,
-		snapshot:           o.snapshot,
-		iterators:          make([]segment.PostingsIterator, len(o.snapshot.segment)),
-		segmentOffset:      0,
-		includeFreq:        false,
-		includeNorm:        false,
-		includeTermVectors: false,
-	}
+	oTFR := o.snapshot.unadornedTermFieldReader(
+		OptimizeTFRConjunctionUnadornedTerm, OptimizeTFRConjunctionUnadornedField)
 
 	var actualBMs []*roaring.Bitmap // Collected from regular posting lists.
 
@@ -283,7 +275,7 @@ func (s *IndexSnapshotTermFieldReader) optimizeDisjunctionUnadorned(
 	octx index.OptimizableContext, minChildCardinality uint64) (index.OptimizableContext, error) {
 	if octx == nil {
 		octx = &OptimizeTFRDisjunctionUnadorned{
-			snapshot: s.snapshot,
+			snapshot:            s.snapshot,
 			minChildCardinality: minChildCardinality,
 		}
 	}
@@ -349,16 +341,8 @@ func (o *OptimizeTFRDisjunctionUnadorned) Finish() (rv index.Optimized, err erro
 
 	// We use an artificial term and field because the optimized
 	// termFieldReader can represent multiple terms and fields.
-	oTFR := &IndexSnapshotTermFieldReader{
-		term:               OptimizeTFRDisjunctionUnadornedTerm,
-		field:              OptimizeTFRDisjunctionUnadornedField,
-		snapshot:           o.snapshot,
-		iterators:          make([]segment.PostingsIterator, len(o.snapshot.segment)),
-		segmentOffset:      0,
-		includeFreq:        false,
-		includeNorm:        false,
-		includeTermVectors: false,
-	}
+	oTFR := o.snapshot.unadornedTermFieldReader(
+		OptimizeTFRDisjunctionUnadornedTerm, OptimizeTFRDisjunctionUnadornedField)
 
 	var docNums []uint32            // Collected docNum's from 1-hit posting lists.
 	var actualBMs []*roaring.Bitmap // Collected from regular posting lists.
@@ -405,3 +389,22 @@ func (o *OptimizeTFRDisjunctionUnadorned) Finish() (rv index.Optimized, err erro
 	atomic.AddUint64(&o.snapshot.parent.stats.TotTermSearchersStarted, uint64(1))
 	return oTFR, nil
 }
+
+// ----------------------------------------------------------------
+
+func (i *IndexSnapshot) unadornedTermFieldReader(
+	term []byte, field string) *IndexSnapshotTermFieldReader {
+	// This IndexSnapshotTermFieldReader will not be recycled, more
+	// conversation here: https://github.com/blevesearch/bleve/pull/1438
+	return &IndexSnapshotTermFieldReader{
+		term:               term,
+		field:              field,
+		snapshot:           i,
+		iterators:          make([]segment.PostingsIterator, len(i.segment)),
+		segmentOffset:      0,
+		includeFreq:        false,
+		includeNorm:        false,
+		includeTermVectors: false,
+		recycle:            false,
+	}
+}
diff --git a/index/scorch/segment/unadorned.go b/index/scorch/segment/unadorned.go
index 56e75234..db06562d 100644
--- a/index/scorch/segment/unadorned.go
+++ b/index/scorch/segment/unadorned.go
@@ -24,7 +24,6 @@ var reflectStaticSizeUnadornedPostingsIteratorBitmap int
 var reflectStaticSizeUnadornedPostingsIterator1Hit int
 var reflectStaticSizeUnadornedPosting int
 
-
 func init() {
 	var pib UnadornedPostingsIteratorBitmap
 	reflectStaticSizeUnadornedPostingsIteratorBitmap = int(reflect.TypeOf(pib).Size())
@@ -34,7 +33,7 @@ func init() {
 	reflectStaticSizeUnadornedPosting = int(reflect.TypeOf(up).Size())
 }
 
-type UnadornedPostingsIteratorBitmap struct{
+type UnadornedPostingsIteratorBitmap struct {
 	actual   roaring.IntPeekable
 	actualBM *roaring.Bitmap
 }
@@ -72,15 +71,15 @@ func (i *UnadornedPostingsIteratorBitmap) Size() int {
 	return reflectStaticSizeUnadornedPostingsIteratorBitmap
 }
 
-func (i *UnadornedPostingsIteratorBitmap)  ActualBitmap() *roaring.Bitmap {
+func (i *UnadornedPostingsIteratorBitmap) ActualBitmap() *roaring.Bitmap {
 	return i.actualBM
 }
 
-func (i *UnadornedPostingsIteratorBitmap)  DocNum1Hit() (uint64, bool) {
+func (i *UnadornedPostingsIteratorBitmap) DocNum1Hit() (uint64, bool) {
 	return 0, false
 }
 
-func (i *UnadornedPostingsIteratorBitmap)  ReplaceActual(actual *roaring.Bitmap) {
+func (i *UnadornedPostingsIteratorBitmap) ReplaceActual(actual *roaring.Bitmap) {
 	i.actualBM = actual
 	i.actual = actual.Iterator()
 }
@@ -88,13 +87,13 @@ func (i *UnadornedPostingsIteratorBitmap)  ReplaceActual(actual *roaring.Bitmap)
 func NewUnadornedPostingsIteratorFromBitmap(bm *roaring.Bitmap) PostingsIterator {
 	return &UnadornedPostingsIteratorBitmap{
 		actualBM: bm,
-		actual: bm.Iterator(),
+		actual:   bm.Iterator(),
 	}
 }
 
 const docNum1HitFinished = math.MaxUint64
 
-type UnadornedPostingsIterator1Hit struct{
+type UnadornedPostingsIterator1Hit struct {
 	docNum uint64
 }
 
@@ -158,4 +157,4 @@ func (p UnadornedPosting) Locations() []Location {
 
 func (p UnadornedPosting) Size() int {
 	return reflectStaticSizeUnadornedPosting
-}
\ No newline at end of file
+}
diff --git a/index/scorch/segment_plugin.go b/index/scorch/segment_plugin.go
index 4729e6bd..b830b2c0 100644
--- a/index/scorch/segment_plugin.go
+++ b/index/scorch/segment_plugin.go
@@ -81,11 +81,11 @@ func chooseSegmentPlugin(forcedSegmentType string,
 
 func (s *Scorch) loadSegmentPlugin(forcedSegmentType string,
 	forcedSegmentVersion uint32) error {
-		segPlugin, err := chooseSegmentPlugin(forcedSegmentType,
-			forcedSegmentVersion)
-		if err != nil {
-			return err
-		}
-		s.segPlugin = segPlugin
-		return nil
+	segPlugin, err := chooseSegmentPlugin(forcedSegmentType,
+		forcedSegmentVersion)
+	if err != nil {
+		return err
+	}
+	s.segPlugin = segPlugin
+	return nil
 }
diff --git a/index/scorch/snapshot_index.go b/index/scorch/snapshot_index.go
index 47cc809b..9d17bcb2 100644
--- a/index/scorch/snapshot_index.go
+++ b/index/scorch/snapshot_index.go
@@ -303,9 +303,12 @@ func (i *IndexSnapshot) newDocIDReader(results chan *asynchSegmentResult) (index
 	var err error
 	for count := 0; count < len(i.segment); count++ {
 		asr := <-results
-		if asr.err != nil && err != nil {
-			err = asr.err
-		} else {
+		if asr.err != nil {
+			if err == nil {
+				// returns the first error encountered
+				err = asr.err
+			}
+		} else if err == nil {
 			rv.iterators[asr.index] = asr.docs.Iterator()
 		}
 	}
@@ -511,10 +514,20 @@ func (i *IndexSnapshot) allocTermFieldReaderDicts(field string) (tfr *IndexSnaps
 		}
 	}
 	i.m2.Unlock()
-	return &IndexSnapshotTermFieldReader{}
+	return &IndexSnapshotTermFieldReader{
+		recycle: true,
+	}
 }
 
 func (i *IndexSnapshot) recycleTermFieldReader(tfr *IndexSnapshotTermFieldReader) {
+	if !tfr.recycle {
+		// Do not recycle an optimized unadorned term field reader (used for
+		// ConjunctionUnadorned or DisjunctionUnadorned), during when a fresh
+		// roaring.Bitmap is built by AND-ing or OR-ing individual bitmaps,
+		// and we'll need to release them for GC. (See MB-40916)
+		return
+	}
+
 	i.parent.rootLock.RLock()
 	obsolete := i.parent.root != i
 	i.parent.rootLock.RUnlock()
diff --git a/index/scorch/snapshot_index_tfr.go b/index/scorch/snapshot_index_tfr.go
index 61537fc4..239f68fb 100644
--- a/index/scorch/snapshot_index_tfr.go
+++ b/index/scorch/snapshot_index_tfr.go
@@ -45,6 +45,7 @@ type IndexSnapshotTermFieldReader struct {
 	includeTermVectors bool
 	currPosting        segment.Posting
 	currID             index.IndexInternalID
+	recycle            bool
 }
 
 func (i *IndexSnapshotTermFieldReader) Size() int {
diff --git a/search/searcher/search_disjunction_heap.go b/search/searcher/search_disjunction_heap.go
index 00b09fcb..7f0a5a00 100644
--- a/search/searcher/search_disjunction_heap.go
+++ b/search/searcher/search_disjunction_heap.go
@@ -310,7 +310,7 @@ func (s *DisjunctionHeapSearcher) Optimize(kind string, octx index.OptimizableCo
 		}
 	}
 
-	return octx, nil
+	return nil, nil
 }
 
 // heap impl
diff --git a/search/searcher/search_disjunction_slice.go b/search/searcher/search_disjunction_slice.go
index 464878bc..dc566ade 100644
--- a/search/searcher/search_disjunction_slice.go
+++ b/search/searcher/search_disjunction_slice.go
@@ -294,5 +294,5 @@ func (s *DisjunctionSliceSearcher) Optimize(kind string, octx index.OptimizableC
 		}
 	}
 
-	return octx, nil
+	return nil, nil
 }
diff --git a/search/searcher/search_multi_term.go b/search/searcher/search_multi_term.go
index 85aacc17..06a265c7 100644
--- a/search/searcher/search_multi_term.go
+++ b/search/searcher/search_multi_term.go
@@ -33,7 +33,7 @@ func NewMultiTermSearcher(indexReader index.IndexReader, terms []string,
 		}
 	}
 
-	qsearchers, err  := makeBatchSearchers(indexReader, terms, field, boost, options)
+	qsearchers, err := makeBatchSearchers(indexReader, terms, field, boost, options)
 	if err != nil {
 		return nil, err
 	}
@@ -57,7 +57,7 @@ func NewMultiTermSearcherBytes(indexReader index.IndexReader, terms [][]byte,
 		}
 	}
 
-	qsearchers, err  := makeBatchSearchersBytes(indexReader, terms, field, boost, options)
+	qsearchers, err := makeBatchSearchersBytes(indexReader, terms, field, boost, options)
 	if err != nil {
 		return nil, err
 	}
@@ -112,7 +112,7 @@ func optimizeMultiTermSearcher(indexReader index.IndexReader, terms []string,
 				}
 			}
 		}
-		finalSearcher, err =  optimizeCompositeSearcher("disjunction:unadorned-force",
+		finalSearcher, err = optimizeCompositeSearcher("disjunction:unadorned-force",
 			indexReader, batch, options)
 		// all searchers in batch should be closed, regardless of error or optimization failure
 		// either we're returning, or continuing and only finalSearcher is needed for next loop
@@ -177,7 +177,7 @@ func optimizeMultiTermSearcherBytes(indexReader index.IndexReader, terms [][]byt
 				}
 			}
 		}
-		finalSearcher, err =  optimizeCompositeSearcher("disjunction:unadorned-force",
+		finalSearcher, err = optimizeCompositeSearcher("disjunction:unadorned-force",
 			indexReader, batch, options)
 		// all searchers in batch should be closed, regardless of error or optimization failure
 		// either we're returning, or continuing and only finalSearcher is needed for next loop
diff --git a/search/searcher/search_term.go b/search/searcher/search_term.go
index c1af74c7..e07d2533 100644
--- a/search/searcher/search_term.go
+++ b/search/searcher/search_term.go
@@ -137,5 +137,5 @@ func (s *TermSearcher) Optimize(kind string, octx index.OptimizableContext) (
 		return o.Optimize(kind, octx)
 	}
 
-	return octx, nil
+	return nil, nil
 }


Test patch:
diff --git a/index/scorch/scorch_test.go b/index/scorch/scorch_test.go
index e22fafa2..5f0ec1b7 100644
--- a/index/scorch/scorch_test.go
+++ b/index/scorch/scorch_test.go
@@ -2458,7 +2458,6 @@ func TestIndexSeekBackwardsStats(t *testing.T) {
 		t.Fatalf("error closing term field reader: %v", err)
 	}
 
-
 	if idx.(*Scorch).stats.TotTermSearchersStarted != idx.(*Scorch).stats.TotTermSearchersFinished {
 		t.Errorf("expected term searchers started %d to equal term searchers finished %d",
 			idx.(*Scorch).stats.TotTermSearchersStarted,
diff --git a/index_test.go b/index_test.go
index 1662c60e..c535456b 100644
--- a/index_test.go
+++ b/index_test.go
@@ -2133,3 +2133,101 @@ func testBatchRaceBug1149(t *testing.T, i Index) {
 	}
 	b.Reset()
 }
+
+func TestOptimisedConjunctionSearchHits(t *testing.T) {
+	defer func() {
+		err := os.RemoveAll("testidx")
+		if err != nil {
+			t.Fatal(err)
+		}
+	}()
+
+	idx, err := NewUsing("testidx", NewIndexMapping(), "scorch", "scorch", nil)
+	if err != nil {
+		t.Fatal(err)
+	}
+	doca := map[string]interface{}{
+		"country":    "united",
+		"name":       "Mercure Hotel",
+		"directions": "B560 and B56 Follow signs to the M56",
+	}
+	docb := map[string]interface{}{
+		"country":    "united",
+		"name":       "Mercure Altrincham Bowdon Hotel",
+		"directions": "A570 and A57 Follow signs to the M56 Manchester Airport",
+	}
+	docc := map[string]interface{}{
+		"country":    "india united",
+		"name":       "Sonoma Hotel",
+		"directions": "Northwest",
+	}
+	docd := map[string]interface{}{
+		"country":    "United Kingdom",
+		"name":       "Cresta Court Hotel",
+		"directions": "junction of A560 and A56",
+	}
+
+	b := idx.NewBatch()
+	err = b.Index("a", doca)
+	if err != nil {
+		t.Error(err)
+	}
+	err = b.Index("b", docb)
+	if err != nil {
+		t.Error(err)
+	}
+	err = b.Index("c", docc)
+	if err != nil {
+		t.Error(err)
+	}
+	err = b.Index("d", docd)
+	if err != nil {
+		t.Error(err)
+	}
+	// execute the batch
+	err = idx.Batch(b)
+	if err != nil {
+		log.Fatal(err)
+	}
+
+	mq := NewMatchQuery("united")
+	mq.SetField("country")
+
+	cq := NewConjunctionQuery(mq)
+
+	mq1 := NewMatchQuery("hotel")
+	mq1.SetField("name")
+	cq.AddQuery(mq1)
+
+	mq2 := NewMatchQuery("56")
+	mq2.SetField("directions")
+	mq2.SetFuzziness(1)
+	cq.AddQuery(mq2)
+
+	req := NewSearchRequest(cq)
+	req.Score = "none"
+
+	res, err := idx.Search(req)
+	if err != nil {
+		t.Fatal(err)
+	}
+	hitsWithOutScore := res.Total
+
+	req = NewSearchRequest(cq)
+	req.Score = ""
+
+	res, err = idx.Search(req)
+	if err != nil {
+		t.Fatal(err)
+	}
+	hitsWithScore := res.Total
+
+	if hitsWithOutScore != hitsWithScore {
+		t.Errorf("expected %d hits without score, got %d", hitsWithScore, hitsWithOutScore)
+	}
+
+	err = idx.Close()
+	if err != nil {
+		t.Fatal(err)
+	}
+}
diff --git a/search/searcher/search_term_range_test.go b/search/searcher/search_term_range_test.go
index ec8935c3..22cf065d 100644
--- a/search/searcher/search_term_range_test.go
+++ b/search/searcher/search_term_range_test.go
@@ -280,5 +280,4 @@ func TestTermRangeSearchTooManyTerms(t *testing.T) {
 	if !reflect.DeepEqual(got, want) {
 		t.Errorf("expected: %#v, got %#v", want, got)
 	}
-
-}
\ No newline at end of file
+}