Adds a new spine that does compaction in a thread. #1863
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
Collaborator
gz
commented
Jun 11, 2024
gz
commented
- The idea is to have one thread per spine. Maybe later we can relax this to n background compactor thread and load-balance work across multiple spines (this is e.g., what RocksDB does)
- The thread just does the compaction on behalf of the spine and sends the results back. For now it will just follow a simple FIFO order and compact one by one. In future improvements we can add n-way merges, and round-robin or other strategies to merge multiple things at the same time.
- Having compaction in a different thread meant slightly changing the way that the backend meta-data about read-only files is kept. Before it was strictly per-thread and stored by the thread that creates them. Now they need to be shareable among compactor and threads that hold spines so this meta-data lives in a concurrent hashmap.
- There was also some issue with not dropping the lockfile on the storage dir in tests. This was resolved by moving the StorageLocation to the RuntimeHandle.
- Had to so some small changes to tests due to asserts that were too strong assumed behavior of the old spine but werent necessary.
gz
force-pushed
the
async-merge
branch
4 times, most recently
from
9f9efaa
to
7892010
Compare
gz
changed the title
Benchmark resultsNexmark
Galen
|
|
The comment from above shows that it seems to improve performance for DRAM. I'll add/run the results with storage enabled too. |
|
I'm reading the code this morning. |
blp
approved these changes
Jun 14, 2024
There was a problem hiding this comment.
I read the code and I like how much simpler it is than the fueled spine. I think that the assertion failure we're having in the fueled spine code is going to be hard to confidently fix.
This is going to create a lot of threads in cases where we have a lot of spines. I suspect we'll have to do something about that, but this is a fine place to start.
Thank you!
|
some performance results with storage: fueled spine async spine
here is a graph showing disk bw during nexmark queries. the biggest writes happen for q9 the rest is all very low writes bw but still much slower than dram.. I will try to get look a bit at perf again after merging this. |
|
@gz any idea why the peak RSS seems to be very high for async spine? Is it accumulating work faster than it is able to process? |
I changed the amount of background threads to the number of workers instead of #spines.
|
|
by popular demand, with min_storage_row set to 10_000 instead of 0 (previous tables) -async spine unfortunately the spine fueled fails with |
gz
force-pushed
the
async-merge
branch
4 times, most recently
from
356c12f
to
333e287
Compare
|
ran q9 with 50M events (min_storage_rows=100_000)..
|
- The idea is to have one background thread per worker to handle spine merges in the background. Maybe later the amount of background workers is made configurable. - The thread just does the compaction on behalf of the spine and sends the results back. For now it will just follow a simple FIFO order and compact one by one. In future improvements we can add n-way merges, and round-robin or other strategies to merge multiple things at the same time. - Having compaction in a different thread meant slightly changing the way that the backend meta-data about read-only files is kept. Before it was strictly per-thread and stored by the thread that creates them. Now they need to be shareable among compactor and threads that hold spines so this meta-data lives in a concurrent hashmap. - There was also some issue with not dropping the lockfile on the storage dir in tests. This was resolved by moving the StorageLocation to the RuntimeHandle. - Had to so some small changes to tests due to asserts that were too strong assumed behavior of the old spine but werent necessary. - I changed the concurrency for the buffer cache (again), now it is shared and protected with a Mutex, but the sharing is limited and just between the worker and it's background thread. Signed-off-by: Gerd Zellweger <mail@gerdzellweger.com>
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.