-
Notifications
You must be signed in to change notification settings - Fork 77
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
skiplist: not use CAS for single thread writing #158
skiplist: not use CAS for single thread writing #158
Conversation
Signed-off-by: zhangjinpeng1987 <zhangjinpeng@pingcap.com>
Codecov Report
@@ Coverage Diff @@
## master #158 +/- ##
==========================================
+ Coverage 90.11% 90.36% +0.24%
==========================================
Files 36 36
Lines 6951 6982 +31
==========================================
+ Hits 6264 6309 +45
+ Misses 687 673 -14 |
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.
In what cases will we concurrently write to memtable?
Lines 248 to 254 in 366404d
/// `write_to_lsm` will only be called in write thread (or write coroutine). | |
/// | |
/// By using a fine-grained lock approach, writing to LSM tree acquires: | |
/// 1. read lock of memtable list (only block flush) | |
/// 2. write lock of mutable memtable WAL (won't block mut-table read). | |
/// 3. level controller lock (TBD) | |
pub fn write_to_lsm(&self, request: Request) -> Result<()> { |
Currently, all writes go into only one thread.
@skyzh The context is that, skiplist's previous implementation works under both single-thread write and multi-threads write. But there is a more efficient way to implement for single thread write scenario which just use atomic.store, don't use CAS. That is what I have done in this PR.
This is true, but skiplist is a fundamental component, maybe in the future we need the concurrent write ability. |
Signed-off-by: zhangjinpeng1987 <zhangjinpeng@pingcap.com>
Signed-off-by: zhangjinpeng1987 <zhangjinpeng@pingcap.com>
@skyzh Any other concerns or comments? |
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
} | ||
} | ||
} else { | ||
// There is no need to use CAS for single thread writing. |
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.
we plan to support the multiple thread writing, it's necessary now? We may need to delete in future.
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.
If treat AgateDB as a fundamental kv engine on cloud, maybe it need both single thread write and multi-threads write.
Signed-off-by: zhangjinpeng1987 zhangjinpeng@pingcap.com