perf: implement batch execution AND/OR/NAND - #479
Conversation
5891414 to
85278eb
Compare
85278eb to
c5804e0
Compare
dkharms
added
the
performance
c5804e0 to
28330f0
Compare
cheb0
changed the title
28330f0 to
1116aa2
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 329-lid-bitmaps #479 +/- ##
===================================================
+ Coverage 71.49% 71.54% +0.05%
===================================================
Files 235 238 +3
Lines 19328 19858 +530
===================================================
+ Hits 13818 14208 +390
- Misses 4465 4590 +125
- Partials 1045 1060 +15 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
🔴 Performance DegradationSome benchmarks have degraded compared to the previous run. Show table
|
| Enabled bool `config:"enabled"` | ||
| // CostThreshold is the minimum estimated non-batched execution cost required to enable batch-at-a-time query | ||
| // evaluation. Suggestion is to use value which is greater than 3 x LID block size. | ||
| CostThreshold int `config:"cost_threshold" default:"50000"` |
There was a problem hiding this comment.
Default 50000 contradicts the comment right above:
3 x LID block size
| } | ||
|
|
||
| func (b *batcherNode) NextBatch() LIDBatch { | ||
| batch := b.batch[:0] |
There was a problem hiding this comment.
This is a little bit error-prone because at first I thought that batcherNode can wrap any other node, not only the root node (incorrect usage will lead to corruptions).
Maybe we should rename this type to specify that this type must wrap the root?
| @@ -14,9 +14,9 @@ type Node interface { | |||
| type BatchedNode interface { | |||
| fmt.Stringer | |||
| // NextBatch returns next batch. Returns nil when exhausted. | |||
There was a problem hiding this comment.
It returns emptyBatch, not nil. Same for NextBatchGeq comment.
|
|
||
| lidsBatch := lidsBuf[:n] | ||
| total += n | ||
| remaining -= n |
There was a problem hiding this comment.
We can silently drop some batches when lids that are mapped to the same id are returned.
Here we substract the lids count but here we perform deduplication.
Here is the small reproduction (it's LLM written but it is straight to the point):
package processor
import (
"context"
"testing"
"github.com/stretchr/testify/require"
"github.com/ozontech/seq-db/metric/stopwatch"
"github.com/ozontech/seq-db/node"
"github.com/ozontech/seq-db/seq"
)
type stubIDsIndex struct {
ids []seq.ID
}
func (s *stubIDsIndex) LessOrEqual(lid seq.LID, id seq.ID) bool { panic("not used") }
func (s *stubIDsIndex) GetMID(lid seq.LID) seq.MID { return s.ids[lid].MID }
func (s *stubIDsIndex) GetRID(lid seq.LID) seq.RID { return s.ids[lid].RID }
func (s *stubIDsIndex) Len() int { return len(s.ids) }
func (s *stubIDsIndex) GetMIDs(lids []node.LID, out []seq.MID) []seq.MID {
for _, lid := range lids {
out = append(out, s.ids[lid.Unpack()].MID)
}
return out
}
func (s *stubIDsIndex) GetRIDs(lids []node.LID, out []seq.RID) []seq.RID {
for _, lid := range lids {
out = append(out, s.ids[lid.Unpack()].RID)
}
return out
}
func TestIterateEvalTreeDuplicateIDsAtBatchBoundary(t *testing.T) {
// LID 0 is unused; IDs descend as LID grows; LIDs 1 and 2 share one seq.ID.
idx := &stubIDsIndex{ids: []seq.ID{
1: {MID: 600, RID: 1},
2: {MID: 600, RID: 1}, // duplicate of LID 1
3: {MID: 500, RID: 1},
4: {MID: 400, RID: 1},
5: {MID: 300, RID: 1},
6: {MID: 200, RID: 1},
}}
// Single batch with 6 LIDs, longer than the limit.
evalTree := node.NewStaticBatched([]uint32{1, 2, 3, 4, 5, 6}, seq.DocsOrderDesc.IsReverse())
params := SearchParams{
Limit: 3,
Order: seq.DocsOrderDesc,
}
total, ids, _, _, err := iterateEvalTree(context.Background(), params, idx, evalTree, nil, stopwatch.New())
require.NoError(t, err)
// 6 LIDs exist and the limit is not yet satisfied after the duplicate,
// so the search must keep scanning: expected IDs are 600, 500, 400.
got := make([]seq.MID, 0, len(ids))
for _, id := range ids {
got = append(got, id.ID.MID)
}
require.Equal(t, []seq.MID{600, 500, 400}, got)
require.Equal(t, 4, total) // 4 LIDs scanned to produce 3 distinct IDs
}
|
|
||
| tokenTable := ti.tokenTableLoader.Load() | ||
| for i, tid := range tids { | ||
| if tid == 0 { |
There was a problem hiding this comment.
nit: This shouldn't really happen. If it does, there's a bug somewhere. Better to panic here than to silently ignore it, right?
| return tids, nil | ||
| } | ||
|
|
||
| func (ti *sealedTokenIndex) GetFreqsByTIDs(tids []uint32, field string) []uint32 { |
There was a problem hiding this comment.
nit: It seems like it would be more optimal to fetch both the token and its frequency in a single block load inside GetTIDsByTokenExpr() — but that's more complicated than fetching them separately.
| if len(dst) == 0 || len(tmp) == 0 { | ||
| return 0 | ||
| } | ||
| n := it.it.NextMany(tmp[:min(len(dst), len(tmp))]) |
There was a problem hiding this comment.
I don't like that here — and in other implementations — tmp affects the size of dst. That makes responsibility blurry. Let's call NextMany multiple times if tmp is smaller than dst.
| n.ptr = len(n.data) | ||
| } | ||
|
|
||
| if n.batch.IsEmpty() { |
There was a problem hiding this comment.
Looks like an unnecessary check here
| n.ptr = -1 | ||
| } | ||
|
|
||
| if n.batch.IsEmpty() { |
There was a problem hiding this comment.
Looks like an unnecessary check here
|
|
||
| func (b *batcherNode) NextBatchGeq(nextID LID) LIDBatch { | ||
| batch := b.batch[:0] | ||
| polled := 0 |
There was a problem hiding this comment.
Same here — batch and pooled don't really make sense in this context
| } | ||
| childCosts[i] = c | ||
| } | ||
| if len(childCosts) != 2 { |
There was a problem hiding this comment.
nit: Looks like a second check for 'not'. Is it definitely needed here?
| if err != nil { | ||
| return 0, err | ||
| } | ||
| if len(tids) > maxBatchedTIDsPerLeaf { |
There was a problem hiding this comment.
I just don't fully understand the criteria for using batches here
| return 0, nil | ||
| } | ||
|
|
||
| freqs := ti.GetFreqsByTIDs(tids, field) |
There was a problem hiding this comment.
What do you think — could this method be merged with GetTIDsByTokenExpr in the future to extract data in a single pass?
| return nil, errBatchingUnsupported | ||
| } | ||
|
|
||
| cache := make(leafTIDsCache) |
There was a problem hiding this comment.
Just thinking out loud — maybe in the future we could create an intermediate structure for the query execution plan, like AST → PlanTree, and this populated plan would contain all the TIDs, costs, and logical nodes, which would then be used to build the LIDs processing tree. That way we wouldn't have to make the same index lookups (GetTIDsByTokenExpr) repeatedly through the cache.
5 participants
Description
Batch execution allows to iterate over inverted index batch by batch instead of lid by lid.
Batches are either of slice or roaring bitmap type. When batches are intersected/unioned they are converted to bitmaps.
Enabling
rangenode can have at most 5 tids resolvedIteration cost evaluation allows to enable batching when it's really worth it. In that case we know some good amount of LID blocks will be directly used as bitmaps and not be converted.
Results
Major problems
There are problems I found while working on batch execution. Can be partially addressed in future.
LIDBatch- interface dispatch overhead is now present on inverted index which has partially affected hot queries performance.NextGEQ(single function)nodeOrBatchedMulti) can yield a large batch of size more than LID block. Truncating a batch can cost CPU and increase query execution time.