fix(LSM): restructure Compaction to cleanup and handle concurrent IO / CPU asserts #146
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
|
@kprotty what is the next step to get this fixed? What can we try here? |
kprotty
force-pushed
the
lsm-table-visibility
branch
2 times, most recently
from
2575c05 to
b54d176
Compare
|
@jorangreef I've pivoted this PR into restructuring the Compaction and addressing various asserts that were hit along the way. |
kprotty
changed the title
kprotty
force-pushed
the
lsm-table-visibility
branch
2 times, most recently
from
aa68eb8 to
61e6cf4
Compare
eatonphil
reviewed
Sep 15, 2022
kprotty
force-pushed
the
lsm-table-visibility
branch
from
08e7c39 to
5804f07
Compare
sentientwaffle
approved these changes
Sep 19, 2022
…nfoBuffer The assertions were overly tight and assumed the tables reported from the iterator intersected with the key range hint. Instead, the tables should only overlap as asserted elsewhere. The function also implemented its own form of TableInfoBuffer() so went ahead and used the shared abstraction.
Tried to restructure the Compaction type implementation as an attempt to simplify control flow for analysis. This currently errors out with a rogue Grid.Write having its callback triggered.
I was using `inline for` to generate the callbacks for the Compaction's BlockWrites to Grid. This had unexpected behavior where it cached the callback for the first field so the other fields used the wrong one. Moving the per-field writing to its own function solved this. Also took the opportunity to clean up the control flow and assert orderings in the Tree compact code: - Now, invisible tables are removed from `level_b` and happen when a compaction completes instead of when all compactions for that half measure complete. - The `mut_table.can_commit_batch` assert was also moved after mut_table is sorted into immut_table.
Given IO is now serialized with CPU merge, we don't need the intermediary blocks for index, data, and filter. They now point directly do the Table.Builder's blocks for writing. We also know that tables reported by the iterator_b (LevelIterator) will always be visible to the compaction snapshot so they can be queued for removal/updating. Finally, moved around the check for a full data block to after builder.data_block_finish() was called as that's when it writes the block's vsr.Header to work correctly with the table helpers.
Since snapshots are no longer as unique as they once were (with compaction updating with snapshots from the past), the visibility asserts don't have to be as strict in their ranges.
These tables will be deleted so their Grid blocks can be re-used. It should also be safe to release their blocks in the iterator_b callback as iterator_b (LevelIterator) makes a copy of the block contents itself.
The level_a selected table and the overlapping level_b tables are merged into new tables inserted into level_b. The old input level_b tables were already being removed, so this removes the selected level_a one as well.
The manifest_log was using the superblock's free_set directly to acquire/release block addresses. This conflicts with it also using the Grid for read/write. Grid read/writes require that the block addresses you pass in to be from its own acquire/release since those update its cache. The ManifestLog was not doing this so we address that here.
Grid read iops can be observed to have empty `read` FIFOs during Grid.read_block_callback. There, the if branch pops from the FIFO and invokes their callbacks. On the last pop, the FIFO can be empty and the callback could iterate the iops expecting their FIFO's to not be empty.
Brings back merge_done and makes MergeIterator optional to represent .idle state.
added 9 commits
September 19, 2022 12:37
While scanning for invisible tables in a level, we may fill up the table buffer and flush/remove them from the level. When scanning in ascending order, this could invalidate the references from the iterator. Instead, scan in descending order to ensure the tables we queue and potentially flush/remove have already been visited.
There's a slight change here in that snapshot_min check is now inclusive while snapshot_max remains exclusive.
The old pattern would enforce that pending blocks at a tick are written out on the next tick. This instead gathers pending blocks across multiple ticks until they either get full or there's no more values to merge.
They're allocated with the expected maximum so overflow to flush to manifest mid-enqueue should never occur.
kprotty
force-pushed
the
lsm-table-visibility
branch
from
32d2609 to
14ff227
Compare
sentientwaffle
approved these changes
Sep 19, 2022
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.
Restructured the
Compactionstruct to clean up control flow and get rid of various asserts being tripped regarding concurrent IO and CPU work. This is the first step to addressing thetable_count_visibleassertion hit in VOPR seed 545.