-
Notifications
You must be signed in to change notification settings - Fork 92
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
add multibatch write into memtable #131
Conversation
Signed-off-by: Little-Wallace <bupt2013211450@gmail.com>
Signed-off-by: Little-Wallace <bupt2013211450@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the super long delay in reviewing.
I did a quick comparison between PipelinedWriteImpl
and MultiThreadWriteImpl
and find some difference. Also some minor comment about memory ordering for atomic variables. I'll do another pass tomorrow.
db/db_test_util.cc
Outdated
@@ -550,6 +550,13 @@ Options DBTestBase::GetOptions( | |||
options.enable_pipelined_write = true; | |||
break; | |||
} | |||
case kMultiThreadWrite: { | |||
if (options.allow_concurrent_memtable_write) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this be enable_multi_thread_write = true
regardless of allow_concurrent_memtable_write
value?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can not understand. allow_concurrent_memtable_write
is necessary so that we can insert into skiplist concurrently.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But DBImpl::SanitizeOptions
is setting allow_concurrent_memtable_write=true
when enable_multi_thread_write=true
is set.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok. I will remove it.
@@ -114,6 +117,7 @@ class WriteThread { | |||
// Information kept for every waiting writer. | |||
struct Writer { | |||
WriteBatch* batch; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it easy to remove batch
in favor of batches
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not easy....
while (memtable_write_group.running.load(std::memory_order_acquire) > 0) { | ||
std::function<void()> work; | ||
if (write_thread_.write_queue_.PopFront(work)) { | ||
work(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here the leader works on the writer queue. how does followers help with the work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Followers would do this work in AwaitState
.
@@ -137,6 +141,43 @@ uint8_t WriteThread::AwaitState(Writer* w, uint8_t goal_mask, | |||
// 1/sampling_base. | |||
const int sampling_base = 256; | |||
|
|||
if (enable_multi_thread_write_) { | |||
#ifdef NDEBUG |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's the reason debug build and release build logic are different?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because unit test need to block and wait SYNC_POINT to wake up. But in product environment, we need it not block thread so that it could do more jobs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be great if release build and debug build can use the same logic. Otherwise rocksdb unit tests (maybe also tikv unit tests and schrodinger tests?) is not testing the real logic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM overall.
Signed-off-by: Little-Wallace <bupt2013211450@gmail.com>
2670855
to
a6637d7
Compare
Signed-off-by: Little-Wallace <bupt2013211450@gmail.com>
Signed-off-by: Little-Wallace <bupt2013211450@gmail.com>
/run-all-tests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Please fix CI.
/run-all-tests |
Signed-off-by: Little-Wallace <bupt2013211450@gmail.com> Support write multiple WriteBatch into RocksDB together. These WriteBatch will be assigned sequence number in order and pushed into queue. If a thread is waiting for some state, it could steal some job from work queue. Signed-off-by: tabokie <xy.tao@outlook.com>
* add multibatch write into memtable (#131) Signed-off-by: Little-Wallace <bupt2013211450@gmail.com> Support write multiple WriteBatch into RocksDB together. These WriteBatch will be assigned sequence number in order and pushed into queue. If a thread is waiting for some state, it could steal some job from work queue. Signed-off-by: tabokie <xy.tao@outlook.com> * Improve Multi Batch Write (#154) * perform like normal pipelined write Signed-off-by: Little-Wallace <bupt2013211450@gmail.com> Signed-off-by: tabokie <xy.tao@outlook.com> * pass enable_multi_thread_write to BuildDBOptions (#170) Signed-off-by: Little-Wallace <bupt2013211450@gmail.com> Signed-off-by: tabokie <xy.tao@outlook.com> * Fix life time of `memtable_write_group` (#171) * fix life time of memtable_write_group Signed-off-by: Little-Wallace <bupt2013211450@gmail.com> Signed-off-by: tabokie <xy.tao@outlook.com> * Commit pipeline when write a WriteBatch for linearizability (#267) * support commit pipeline Signed-off-by: Little-Wallace <bupt2013211450@gmail.com> Signed-off-by: tabokie <xy.tao@outlook.com> * format Signed-off-by: tabokie <xy.tao@outlook.com> * remove useless code Signed-off-by: tabokie <xy.tao@outlook.com> Co-authored-by: Wallace <bupt2013211450@gmail.com>
* add multibatch write into memtable (#131) Signed-off-by: Little-Wallace <bupt2013211450@gmail.com> Support write multiple WriteBatch into RocksDB together. These WriteBatch will be assigned sequence number in order and pushed into queue. If a thread is waiting for some state, it could steal some job from work queue. Signed-off-by: tabokie <xy.tao@outlook.com> * Improve Multi Batch Write (#154) * perform like normal pipelined write Signed-off-by: Little-Wallace <bupt2013211450@gmail.com> Signed-off-by: tabokie <xy.tao@outlook.com> * pass enable_multi_thread_write to BuildDBOptions (#170) Signed-off-by: Little-Wallace <bupt2013211450@gmail.com> Signed-off-by: tabokie <xy.tao@outlook.com> * Fix life time of `memtable_write_group` (#171) * fix life time of memtable_write_group Signed-off-by: Little-Wallace <bupt2013211450@gmail.com> Signed-off-by: tabokie <xy.tao@outlook.com> * Commit pipeline when write a WriteBatch for linearizability (#267) * support commit pipeline Signed-off-by: Little-Wallace <bupt2013211450@gmail.com> Signed-off-by: tabokie <xy.tao@outlook.com> * format Signed-off-by: tabokie <xy.tao@outlook.com> * remove useless code Signed-off-by: tabokie <xy.tao@outlook.com> Co-authored-by: Wallace <bupt2013211450@gmail.com>
Signed-off-by: Little-Wallace bupt2013211450@gmail.com
Support write multiple WriteBatch into RocksDB together. These WriteBatch will be assigned sequence number in order and pushed into queue. If a thread is waiting for some state, it could steal some job from work queue.