feat: add roaring bitmaps in posing lists - #446
Conversation
|
@seqbenchbot up 336-compaction mixed |
|
@seqbenchbot down ec638c8c |
|
Nice, @cheb0 The benchmark with identificator Show summary
Have a great time! |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #446 +/- ##
==========================================
+ Coverage 71.33% 71.49% +0.16%
==========================================
Files 233 235 +2
Lines 18999 19328 +329
==========================================
+ Hits 13552 13818 +266
- Misses 4419 4465 +46
- Partials 1028 1045 +17 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
d3f3922 to
4af844b
Compare
🔴 Performance DegradationSome benchmarks have degraded compared to the previous run. Show table
|
🔴 Performance DegradationSome benchmarks have degraded compared to the previous run. Show table
|
There was a problem hiding this comment.
It's gonna be interesting to see how bitmaps will affect sealing latency (and system in overall):
benchstat ~/main.txt ~/bitmaps.txt
goos: linux
goarch: amd64
pkg: github.com/ozontech/seq-db/fracmanager
cpu: 12th Gen Intel(R) Core(TM) i5-12600K
│ /home/dkharms/main.txt │ /home/dkharms/bitmaps.txt │
│ sec/op │ sec/op vs base │
Sealing_NoSort-4 459.1m ± 2% 468.2m ± 2% +1.97% (p=0.005 n=10)
│ /home/dkharms/main.txt │ /home/dkharms/bitmaps.txt │
│ B/op │ B/op vs base │
Sealing_NoSort-4 23.05Mi ± 0% 26.06Mi ± 0% +13.05% (p=0.000 n=10)
│ /home/dkharms/main.txt │ /home/dkharms/bitmaps.txt │
│ allocs/op │ allocs/op vs base │
Sealing_NoSort-4 5.322k ± 0% 8.309k ± 0% +56.11% (p=0.000 n=10)There was a problem hiding this comment.
So far I see they slow down quite literally everything except places where you actually intersect/union them :)
| return b.copyLIDsFromBitmap(t, dst) | ||
| } | ||
|
|
||
| func (b *Block) copyLIDsFromBitmap(ref int32, buf []uint32) []uint32 { |
There was a problem hiding this comment.
| func (b *Block) copyLIDsFromBitmap(ref int32, buf []uint32) []uint32 { | |
| func (b *Block) appendLIDsFromBitmapTo(ref int32, buf []uint32) []uint32 { |
There was a problem hiding this comment.
As I am getting deeper in this pull request I just have several questions about roaring bitmaps in general:
- Are they concurrent-safe for read-only use (getting max/min, cardinality)?
- Are they concurrent-safe for (possibly) mutating operations (like
Or(bm1, bm2)) or there are different APIs for performing such operations in-place or returning a new copy instead?
| type Block struct { | ||
| LIDs []uint32 | ||
| Offsets []uint32 | ||
| types []int32 // determines LID list type: delta-encoded (non-negative value) or bitmap (negative value). nil for delta-encoded blocks |
There was a problem hiding this comment.
Have you measured whether materializing types actually buys anything vs. a binary search over bitmapIndexes? Like it's pretty easy to grasp but the liability of performing checks like b.types == nil is error-prone (IMHO).
And this decision is not scalable since you encode the type of block into signedness of integer (so there could be at most two different types). So if we are going to introduce different format for storing lids we would still have to rewrite this logic.
It could be easily rewritten as something like:
func (b *Block) chunkRef(i int) (ChunkType, int) {
k, found := slices.BinarySearch(b.bitmapIndexes, uint32(i))
if found {
return ChunkTypeBitmap, k
}
return ChunkTypeDelta, i - k
}
There was a problem hiding this comment.
Doing a binary search on every Next() call is way too expensive, IMHO.
| // BitmapThreshold specifies minimum number of LIDs in the lid list | ||
| // which are serialized as bitmap. LIDs lists with more elements use bitmap encoding, | ||
| // while smaller lists use delta encoding. | ||
| BitmapThreshold int `config:"bitmap_threshold" default:"65536"` |
There was a problem hiding this comment.
Should we add some kind of validation that BitmapThreshold cannot be larger than BlockSize?
| return size | ||
| } | ||
|
|
||
| func (p *BlockPacker) Pack(b *UnpackedBlock, dst []byte) []byte { |
There was a problem hiding this comment.
Just curious. Have you tried to measure performance metrics when we include whole posting list for token when it exceeds 65k? Of course it heavily depends on density of lids but still it is quite interesting.
However it might break something if we use block capacity as a divider somewhere (I do not remember)...
| NextBatch(need int) LIDBatch | ||
| // NextBatchGeq returns next batch (LIDs >= minLID). Returns nil when exhausted. | ||
| NextBatchGeq(nextLID LID) LIDBatch | ||
| NextBatchGeq(need int, nextLID LID) LIDBatch |
There was a problem hiding this comment.
Seems like need argument is not used anywhere. And in #479 it was deleted again.
There was a problem hiding this comment.
Yes, I deleted it laster. As part of two PRs need will not be added.
Co-authored-by: Daniil <dkharmsd@gmail.com>
Co-authored-by: Daniil <dkharmsd@gmail.com>
🔴 Performance DegradationSome benchmarks have degraded compared to the previous run. Show table
|
| // [offsets: delta-bitpack []uint32] — slice boundaries in the delta-encoded LIDs array | ||
| // [lids: delta-bitpack []uint32] — concatenated delta-encoded LID values | ||
| // | ||
| // Each list i in [0, listsCount) is either a roaring bitmap (when i appears in bitmapIndexes) |
There was a problem hiding this comment.
typo?
Each list iS in [0, listsCount) is either a roaring bitmap (when iT appears in bitmapIndexes)
| } | ||
|
|
||
| func (b *Block) copyLIDsFromBitmap(ref int32, buf []uint32) []uint32 { | ||
| bitmap := b.bitmaps[-ref-1] |
There was a problem hiding this comment.
nit: It would be clearer if we passed the direct bitmap index to this function and performed the -t-1 calculation on the caller side.
return b.copyLIDsFromBitmap(-t-1, dst)
| minLID, maxLID uint32, | ||
| ) *IteratorAsc { | ||
| it := &IteratorAsc{ | ||
| Cursor: *NewLIDsCursor(table, loader, startIndex, tid, counter, minLID, maxLID), |
There was a problem hiding this comment.
nit: Perhaps we could avoid the pointer dereferencing complexity here?
--- a/frac/sealed/lids/cursor.go
+++ b/frac/sealed/lids/cursor.go
@@ -29,8 +29,8 @@ func NewLIDsCursor(
tid uint32,
counter Counter,
minLID, maxLID uint32,
-) *Cursor {
- return &Cursor{
+) Cursor {
+ return Cursor{
table: table,
loader: loader,| if it.minLID > last { // fast path: out-of-bounds 2 | ||
| return nil, false // stop reading blocks | ||
| first := it.batch.Min() | ||
| if it.maxLID < first { |
There was a problem hiding this comment.
nit: We seem to have lost the comments here. It wasn't very clear even with them, but without them, it's quite hard to follow.
| } | ||
|
|
||
| // narrowLIDsRange cuts LIDs between minLID and maxLID. Returns updated tryNextBlock flag. | ||
| func (it *BatchedIteratorAsc) narrowLIDsRange(tryNextBlock bool) bool { |
There was a problem hiding this comment.
nit: It's the same implementation as in the regular iterator. We could just reuse a single shared one.
| return tryNextBlock | ||
| } | ||
|
|
||
| func (it *BatchedIteratorAsc) loadNextLIDsBlock() { |
There was a problem hiding this comment.
nit: This implementation differs from the regular iterator by missing the following calls:
it.tryNextBlock = it.narrowLIDsRange(it.tryNextBlock)
it.counter.AddLIDsCount(it.batch.Len())
However, these are always called right alongside loadNextLIDsBlock.
So, maybe we should move them inside loadNextLIDsBlock? That way, both implementations would be unified with the regular iterator.
| } | ||
|
|
||
| func (it *bitmapReverseIter) Next() (uint32, bool) { | ||
| prev := it.bm.PreviousValue(it.pos - 1) |
There was a problem hiding this comment.
What if we use ReverseIterator inside Next()? Would it be faster?
5 participants
Description