WIP [dbnode] Refactor posting list operations to be lazily evaluated and zero copy#2791
Open
robskillington wants to merge 119 commits intomasterfrom
Open
WIP [dbnode] Refactor posting list operations to be lazily evaluated and zero copy#2791robskillington wants to merge 119 commits intomasterfrom
robskillington wants to merge 119 commits intomasterfrom
Conversation
arnikola
approved these changes
Oct 22, 2020
Comment on lines
+49
to
+51
| var ( | ||
| union = make([]postings.List, 0, len(s.searchers)) | ||
| ) |
Collaborator
There was a problem hiding this comment.
nit: don't need var block here
arnikola
reviewed
Oct 27, 2020
Collaborator
arnikola
left a comment
arnikola
left a comment
There was a problem hiding this comment.
I had a cursory glance over but didn't go super deep on it, figuring a lot of it was very similar to Pilosa's implementation; if this is a bad assumption can go back with more discerning review
| "github.com/uber-go/tally" | ||
| ) | ||
|
|
||
| // LatencyBuckets are a set of latency buckets useful for measuring things. |
| func (i *multiBitmap) IsEmpty() bool { | ||
| iter := i.Iterator() | ||
| hasAny := iter.Next() | ||
| _ = iter.Err() |
Collaborator
There was a problem hiding this comment.
Hm, should this return false if iter.Err() is non-nil? Otherwise no need to call Err
Comment on lines
+253
to
+259
| type containerIterator interface { | ||
| NextContainer() bool | ||
| ContainerKey() uint64 | ||
| ContainerUnion(ctx containerOpContext, target *bitmapContainer) | ||
| ContainerIntersect(ctx containerOpContext, target *bitmapContainer) | ||
| ContainerNegate(ctx containerOpContext, target *bitmapContainer) | ||
| } |
Collaborator
There was a problem hiding this comment.
nit: do these need to be exported? If so, may need comments (and may be worth exporting containerIterator?)
| i.bitmap.Reset(true) | ||
|
|
||
| intersects := i.filter(i.multiContainerIter.containerIters, multiContainerOpIntersect) | ||
| negates := i.filter(i.multiContainerIter.containerIters, multiContainerOpNegate) |
Collaborator
There was a problem hiding this comment.
Doesn't this overwrite the i.filtered in intersects?
Collaborator
Author
There was a problem hiding this comment.
ah yes, TY!
| func (i *multiBitmapContainersIterator) NextContainer() bool { | ||
| if len(i.iters) != 0 { | ||
| // Exhausted. | ||
| return true |
Collaborator
There was a problem hiding this comment.
Should this be
if len(i.iters) == 0 {
return false
}
|
|
||
| // Tell any other callers that we're done loading | ||
| if entry.loadingCh != nil { | ||
| close(entry.loadingCh) |
arnikola
reviewed
Oct 27, 2020
| } | ||
| // Only valid if all intersecting actually matched, | ||
| // if zero intersecting then postings does not contain ID. | ||
| return len(i.intersect) > 0 |
Collaborator
There was a problem hiding this comment.
Nit; can move this to 181, before checking intersectNegate lists
arnikola
reviewed
Oct 27, 2020
| case elem.multiBitmap != nil: | ||
| it = elem.multiBitmap.containerIterator() | ||
|
|
||
| case elem.bitmap != nil: |
Collaborator
There was a problem hiding this comment.
nit: needs to fail if both set?
robskillington
changed the title
robskillington
changed the title
arnikola
reviewed
Nov 3, 2020
|
|
||
| require.False(t, first.Equal(second)) | ||
| } | ||
| // func TestRoaringPostingsListEqualWithOtherNonRoaring(t *testing.T) { |
arnikola
reviewed
Nov 3, 2020
| parameters := gopter.DefaultTestParameters() | ||
| seed := time.Now().UnixNano() | ||
| parameters.MinSuccessfulTests = 100 | ||
| // seed := time.Now().UnixNano() |
Collaborator
There was a problem hiding this comment.
nit; can probably resume using rng seed
arnikola
reviewed
Nov 3, 2020
Comment on lines
+50
to
+52
| // simpleReader, err := simpleSeg.Reader() | ||
| // require.NoError(t, err) | ||
| // simpleExec := executor.NewExecutor([]index.Reader{simpleReader}) |
arnikola
reviewed
Nov 3, 2020
| require.NoError(t, err) | ||
| matchedDocs, err := collectDocs(dOrg) | ||
| require.NoError(t, err) | ||
| require.NoError(t, err, fmt.Sprintf("query: %v\n", q.String())) |
Collaborator
There was a problem hiding this comment.
nit; require.NoError(t, err, q.String()) may be easier?
arnikola
reviewed
Nov 3, 2020
| if okA && okB && countA != countB { | ||
| return false | ||
| } | ||
|
|
Collaborator
There was a problem hiding this comment.
If using countfast here, we should also make sure that
if !okA && !okB {
return EqualIterator(a.Iterator(), b.Iterator())
}
return false
arnikola
reviewed
Nov 3, 2020
| intersects []postings.List, | ||
| negates []postings.List, | ||
| ) (postings.List, error) { | ||
| intersect := make([]readOnlyIterable, 0, len(intersects)) |
Collaborator
There was a problem hiding this comment.
nit: prealloc a slice of iterables := make([]readOnlyIterable, 0, len(intersects) + len(negates) and have both intersect and negate be taken from that, e.g. intersect, negate := iterables[:len(intersects)], iterables[len(intersects):]
arnikola
reviewed
Nov 3, 2020
Comment on lines
+152
to
+163
| Contains(id postings.ID) bool | ||
| ContainerIterator() containerIterator | ||
| } | ||
|
|
||
| type containerIterator interface { | ||
| NextContainer() bool | ||
| ContainerKey() uint64 | ||
| ContainerUnion(ctx containerOpContext, target *bitmapContainer) | ||
| ContainerIntersect(ctx containerOpContext, target *bitmapContainer) | ||
| ContainerNegate(ctx containerOpContext, target *bitmapContainer) | ||
| Err() error | ||
| Close() |
arnikola
reviewed
Nov 3, 2020
| func (i *multiBitmap) IsEmpty() bool { | ||
| iter := i.Iterator() | ||
| hasAny := iter.Next() | ||
| _ = iter.Err() |
Collaborator
There was a problem hiding this comment.
do these need to check err?
…indexed snapshot during a GC run
robskillington
force-pushed
the
r/reduce-index-query-allocs
branch
from
c0e6836 to
3699014
Compare
…-index-query-allocs-rebase
…ding to rebuild all cached searches again
…terministic query key, also returned optimized form
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does / why we need it:
This reduces a significant amount of all index query allocations by implementing the conjunction search as a lazy iterator over existing postings lists. It also caches regexp compilations with an LRU cache and allows setting different sizes for the regexp cache.
This change completely re-implements roaring bitmaps for query execution, this means pilosa roaring bitmaps are only used and index segment build time and all queries execute new read only roaring bitmaps which are completely mmap backed without any bitmap container/other allocations involved.
Special notes for your reviewer:
Does this PR introduce a user-facing and/or backwards incompatible change?:
Does this PR require updating code package or user-facing documentation?: