Skip to content
This repository has been archived by the owner on May 3, 2024. It is now read-only.

Use tree instead of heap for k-way merge #530

Merged
merged 5 commits into from Jul 18, 2023
Merged

Conversation

MBkkt
Copy link
Contributor

@MBkkt MBkkt commented Jun 25, 2023

Motivation

Segment tree (tournament tree) always makes at most log_2(size) count of comparison.
Pessimistic heap (our previous approach) makes X count of comparison:

  1. X = log_2(size + 1) - 1 when iterator exhausted
  2. log_2(size + 1) <= X <= 2 * log_2(size + 1) - 2 otherwise

Also tree layout more cache friendly than heap layout and our code was worse than described.

Do exist drawbacks?

Yes.

  1. Tree needs twice memory.
  2. Tree a little worse in case a lot of segments with single document.

I don't think these are issues because

  1. Even in case of 1000 segments it will be just 8000 additional bytes
  2. It's very rare and already fast enough case and other stuff gives more overhead than tree in such case

Results

I expect speedup consolidation and querying inverted indexes with primary sort

Benchmark iresearch:

./bin/iresearch-benchmarks -m put --in /home/mbkkt/projects/iresearch/iresearch.deps/benchmark_resources/benchmark.data --index-dir iresearch.data_sorted --max-lines=35000000 --format 1_5simd --commit-period=10000 --batch-size=50000 --consolidation-threads 1 --threads=8 --analyzer-type=segmentation '--analyzer-options={}' --sorted-field=id --consolidate-all=1

tree:

Total Time calls:1, time: 1522605290 us, avg call: 1.52261e+09 us
Index batch 50000 calls:667, time: 5282817493 us, avg call: 7.92027e+06 us
Commit time calls:23, time: 575678143 us, avg call: 2.50295e+07 us
Consolidation time calls:15, time: 320538815 us, avg call: 2.13693e+07 us
Consolidating all time calls:1, time: 725629046 us, avg call: 7.25629e+08 us
Cleanup time calls:15, time: 766710 us, avg call: 51114 us
Stream read total time calls:1, time: 749168554 us, avg call: 7.49169e+08 us
Stream read idle time calls:666, time: 718510518 us, avg call: 1.07884e+06 us
Stream read wait time calls:3, time: 86518 us, avg call: 28839.3 us

heap:

Total Time calls:1, time: 1616446412 us, avg call: 1.61645e+09 us
Index batch 50000 calls:667, time: 5362202977 us, avg call: 8.03928e+06 us
Commit time calls:22, time: 582694392 us, avg call: 2.64861e+07 us
Consolidation time calls:14, time: 331161461 us, avg call: 2.36544e+07 us
Consolidating all time calls:1, time: 822534070 us, avg call: 8.22534e+08 us
Cleanup time calls:14, time: 842348 us, avg call: 60167.7 us
Stream read total time calls:1, time: 762987635 us, avg call: 7.62988e+08 us
Stream read idle time calls:666, time: 727215308 us, avg call: 1.09191e+06 us
Stream read wait time calls:1, time: 29722 us, avg call: 29722 us

So we see speedup even for very cheap comparator: just memcmp 6 bytes
I expect more benefits for more expensive comparator or read only workload

Benchmark arangodb:

TODO

Alternatives

Optimistic heap (siftdown instead of unconditional siftdown + siftup in pessimistic heap)
It makes X count of comparison:

  1. X = 1 when lead iterator not changed
  2. X = log_2(size + 1) - 1 when iterator exhausted
  3. 3 <= X <= 2 * log_2(size + 1) - 2 otherwise, important note: it is discrete, so step is 2 compare to any asymptotic above

In experiments and just common sense we could understand:

  1. It's best algorithm for non-overlapped or even for really rare lead iterator changed data.
  2. It's worse for interleaved or even just random data compare to pessimistic heap.

So yes in lucky cases it could be better than tree, but in all other it is even worse than pessimistic heap.
So I want to use tree because it is improvement for almost any data.
If we try to switch to optimistic heap it will be better for one data and worse for another.

@MBkkt MBkkt requested review from gnusi and Dronplane June 25, 2023 14:52
@MBkkt MBkkt force-pushed the chore/merge-iterator branch 3 times, most recently from 5015705 to 8159ea4 Compare June 25, 2023 17:47
Base automatically changed from chore/remove-memory-allocator to master June 27, 2023 11:54
reinterpret_cast<uintptr_t>(rhs));
}

IRS_NO_INLINE bool LazyReset() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why force no-inline here?

Copy link
Contributor Author

@MBkkt MBkkt Jul 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because it's called at most two times: first and last Next.
So better to doesn't have it code in Next or in cycle with Next
Also it will be tail call, so it just jmp

Copy link
Contributor

@Dronplane Dronplane left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Dronplane Dronplane merged commit 7ead4de into master Jul 18, 2023
1 of 2 checks passed
@Dronplane Dronplane deleted the chore/merge-iterator branch July 18, 2023 15:44
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants