gh-1852 sort memtable KV pairs on read #1853
Merged
Conversation
This file contains 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
The memtable for Map is a binary tree so it's always sorted. However, since this is type 'Map' each "row key" holds a map. This map was unsorted in the past. In #1832 we introduced a change that made sure this change would always be sorted ON DISK, i.e. in the segments. It was very natural to also keep it sorted in the memtable, as we did not have to do any sorting when flushing. However, the performance tests on imports that make heavy use of the inverted index had a large performance degradation after #1832. In a test I did locally the import time went up by over 30%. This fix goes back to keeping the KV pairs unsorted and making each change an append only operation. This means it now needs to be sorted in just two places (as opposed to on every single insertion): 1. On a read query. Those should be rare on memtable, since memtables are mostly meant for writing. The added overhead here (minimal) is not a problem since it was also there before #1832 2. When flushing. Flushing is an async operation and the small overhead of sorting each row's Map KVs is neglible. This new implementation has the same import speed as prior to #1832 while keeping all the runtime benefits of having the KV pairs sorted on disk. closes #1852
antas-marcin
approved these changes
Mar 10, 2022
parkerduckworth
approved these changes
Mar 10, 2022
Codecov Report
@@ Coverage Diff @@
## master #1853 +/- ##
==========================================
+ Coverage 66.06% 66.09% +0.02%
==========================================
Files 411 411
Lines 30915 30914 -1
==========================================
+ Hits 20425 20432 +7
+ Misses 8691 8682 -9
- Partials 1799 1800 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
The memtable for Map is a binary tree so it's always sorted. However,
since this is type 'Map' each "row key" holds a map. This map was
unsorted in the past. In #1832 we introduced a change that made sure
this change would always be sorted ON DISK, i.e. in the segments. It was
very natural to also keep it sorted in the memtable, as we did not have
to do any sorting when flushing. However, the performance tests on
imports that make heavy use of the inverted index had a large
performance degradation after #1832. In a test I did locally the import
time went up by over 30%.
This fix goes back to keeping the KV pairs unsorted and making each
change an append only operation. This means it now needs to be sorted in
just two places (as opposed to on every single insertion):
are mostly meant for writing. The added overhead here (minimal) is
not a problem since it was also there before LSMKV: Store Map in always sorted manner #1832
of sorting each row's Map KVs is neglible.
This new implementation has the same import speed as prior to #1832
while keeping all the runtime benefits of having the KV pairs sorted on
disk.
closes #1852