agate: refactor for memtable impl #37

skyzh · 2020-11-03T06:39:17Z

This PR adds prototypes of memtable / wal implementation. At the same time, MemTable is refactored to use one Wal for each memtable.

Signed-off-by: Alex Chi <[email protected]>

skyzh · 2020-11-04T07:54:18Z

@BusyJay @Connor1996 This PR only contains some prototypes and is relatively easy to review, and yet is critical for implementing new functionalities. I suggest taking a look at this patch first to help me get this project move on. Thanks!

Currently, we have many PRs waiting for reviewing. Some of them are small patches and add small features, and some of them are about implementing new structures to make this project move on and complete. I have added priority labels for those PRs that is on critical path and may block me from moving ahead. For other PRs, I think it's okay to review them later.

Signed-off-by: Alex Chi <[email protected]>

… into refactor-for-memtable

src/db.rs

Connor1996 · 2020-11-09T04:00:11Z

src/memtable.rs

+}
+
+impl MemTable {
+    pub fn new(skl: Skiplist<Comparator>, wal: Option<Wal>, opt: AgateOptions) -> Self {


Not sure how it is used. But I think skl and wal should be created in MemTable instead of passing from outside.

WAL name is decided and generated by database instance, so it is passed from outside. And for skiplist, I think we could move it inside memtable.

Connor1996 · 2020-11-09T04:01:58Z

src/memtable.rs

-            immutable: VecDeque::with_capacity(max_count - 1),
-        }
+impl MemTables {
+    pub(crate) fn new(mutable: MemTable, immutable: VecDeque<MemTable>) -> Self {


I prefer the previous with_capacity instead of new with mutable and immutable from outside

The "open memtable" logic is implemented in db.rs, the database engine. Therefore, it's necessary to provide an interface to construct Memtables from a single Memtable.

Signed-off-by: Alex Chi <[email protected]>

BusyJay · 2020-11-12T06:44:56Z

src/util.rs

 pub use skiplist::{FixedLengthSuffixComparator, KeyComparator};
 use std::{cmp, ptr};

-pub static COMPARATOR: FixedLengthSuffixComparator = FixedLengthSuffixComparator::new(8);
+pub static COMPARATOR: FixedLengthSuffixComparator = make_comparator();


What's the difference between Comparator::new(8)?

No difference, just to make the interface unified. What about reverting this change?

BusyJay · 2020-11-12T06:48:51Z

src/util.rs

@@ -56,3 +62,15 @@ where
    }
    i
 }
+
+/// Fill a file with 0.
+pub fn fill_file(file: &mut impl std::io::Write, mut size: u64) -> Result<()> {


Why not just file.set_len(size)?

This function has been removed for now. We will directly call set_len on File, instead of creating this new function to handle that.

BusyJay · 2020-11-12T06:52:43Z

src/db.rs


 pub struct Core {
-    wal: Wal,
-    memtable: MemTable,
+    mt: RwLock<MemTables>,


How about move the lock into MemTables? And Mutex is probably a better choice.

This RwLock guards list of MemTables (e.g. immutable list). Using RwLock ensures that we could parallel fetch data from memtables, while blocking reads only when we're flushing mutable table to immutable one.

~~If we're using Mutex, we may have to hide all memtables behind an Arc to reduce lock contention when parallel read data. Is it right?~~ Current develop branch already uses Arc<Memtable>, we do not need to worry about this point.

And for moving that lock into MemTables, I think that is OK. Meanwhile, I would also like to know, is it our design principle to hide locks in structs, and make all functions callable with immutable reference in structs like MemTable, Agate, LevelController, etc.?

If we're using Mutex, we may have to hide all memtables behind an Arc to reduce lock contention when parallel read data. Is it right?

Why? I think the time it consumes is just creating MemTableView, which should be very quick. Mutex usually has a better performance than RwLock, you can bench and see if it's an improvement.

All functions of Agate should should take immutable reference and Agate should be Sync + Send.

Yes, thanks for pointing that out. I'll use Mutex for MemTables.

Signed-off-by: Alex Chi <[email protected]>

… into refactor-for-memtable

skyzh · 2020-11-13T10:09:30Z

@Connor1996 PTAL, thanks!

src/error.rs

BusyJay · 2020-11-13T11:51:20Z

src/entry.rs

+    pub(crate) version: u64,
+}
+
+pub struct EntryRef<'a> {


Why not just use &Entry?

Entry uses Bytes internally. But sometimes we could only get &[u8] (if not copy).

FYI, https://github.com/tikv/agatedb/blob/develop/src/value.rs#L185

The key and value is a Vec<u8> (or you may think of it as BytesMut), which will only be valid for one iteration. This way, we let our user to decide whether to copy it into a Bytes.

src/db/opt.rs

BusyJay · 2020-11-13T11:56:20Z

src/db/opt.rs

+
+    pub create_if_not_exists: bool,
+    pub num_memtables: usize,
+    pub mem_table_size: u64,


How about using the same name as badger?

I've added all supported options in develop branch. Now they have the same naming convention as badger.

Signed-off-by: Alex Chi <[email protected]>

Connor1996

LGTM

src/db/opt.rs

BusyJay · 2020-11-17T07:12:20Z

src/db/opt.rs

+    pub base_table_size: u64,
+    pub base_level_size: u64,
+    pub level_size_multiplier: usize,
+    pub table_size_multiplier: usize,


What is table_size_multiplier?

This option is defined at https://github.com/dgraph-io/badger/blob/master/options.go#L58 , which seems to be used when calculating target file size of a LSM level.

Turns out I'm referencing an outdated version. 🤣

I have forked badger@45bca18 at https://github.com/skyzh/badger . Seems that badger is under constant change, with new things added and some logic changed. We could always use my fork as reference implementation.

src/db.rs

BusyJay · 2020-11-17T07:14:48Z

src/memtable.rs

+pub struct MemTable {
+    pub(crate) skl: Skiplist<Comparator>,
+    opt: AgateOptions,
+    core: RwLock<MemTableCore>,


Why use RwLock?

This RwLock protects Wal. It seems that when database is running, we only write to Wal, and we don't need to read from it when reading from memtable. Perhaps Mutex is a better choice, I'll change that.

BusyJay · 2020-11-17T07:19:46Z

src/wal.rs

@@ -59,7 +69,7 @@ impl Header {
    }

    /// Decode header from bytes
-    pub fn decode(&mut self, bytes: &mut Bytes) -> Result<usize> {
+    pub fn decode(&mut self, bytes: &[u8]) -> Result<usize> {


This should break implementation of iterator, which depends on decode mutating Bytes.

We could always convert &Bytes to &[u8] without overhead. And when we're decoding header from EntryRef, we had to use &[u8]. So I think this change is reasonable.

I'm not objecting the changes. I'm suggesting the change introduces a bug at

agatedb/src/table/iterator.rs

Lines 123 to 139 in afb59d2

let mut entry_data = self.data.slice(start_offset as usize..end_offset as usize);

let mut header = Header::default();

header.decode(&mut entry_data);

// TODO: merge this truncate with the following key truncate

if header.overlap > self.perv_overlap {

self.key.truncate(self.perv_overlap as usize);

self.key.extend_from_slice(

&self.base_key[self.perv_overlap as usize..header.overlap as usize],

);

}

self.perv_overlap = header.overlap;

let diff_key = &entry_data[..header.diff as usize];

self.key.truncate(header.overlap as usize);

self.key.extend_from_slice(diff_key);

self.val = entry_data.slice(header.diff as usize..);

that needs to be fixed.

That's not the same header. I think this naming might be confusing. This is the header for WAL, and the one in iterator is the SST header. I think we could later unify these interfaces :)

What about using TableHeader for SST, and WalHeader for write-ahead log / vlog? Meanwhile, we could change all decode to use &[u8] as parameter to reduce the need of using slice on Bytes (which introduces overhead of increasing the reference counter)

LGTM, but better another PR.

Alright. I've reverted this change.

Signed-off-by: Alex Chi <[email protected]>

skyzh added 5 commits November 3, 2020 14:33

agate: prepare for memtable and wal impl

b57171e

Signed-off-by: Alex Chi <[email protected]>

fix compile error

5183a95

Signed-off-by: Alex Chi <[email protected]>

format

a084447

Signed-off-by: Alex Chi <[email protected]>

fix warnings

d290a6e

Signed-off-by: Alex Chi <[email protected]>

remove impls

f3e0b22

Signed-off-by: Alex Chi <[email protected]>

skyzh requested review from BusyJay and Connor1996 November 3, 2020 06:44

skyzh added the status/review Ready for review label Nov 3, 2020

skyzh added 2 commits November 4, 2020 15:45

fix warning

9683c84

Signed-off-by: Alex Chi <[email protected]>

Merge branch 'master' into refactor-for-memtable

636f611

skyzh added the priority/p1 label Nov 4, 2020

format

8b72bb1

Signed-off-by: Alex Chi <[email protected]>

skyzh mentioned this pull request Nov 4, 2020

Implement Storage Engine #6

Open

25 tasks

Merge branch 'refactor-for-memtable' of https://github.com/tikv/agatedb…

2502250

… into refactor-for-memtable

Connor1996 reviewed Nov 9, 2020

View reviewed changes

src/db.rs Outdated Show resolved Hide resolved

src/db.rs Outdated Show resolved Hide resolved

src/db.rs Outdated Show resolved Hide resolved

Connor1996 reviewed Nov 9, 2020

View reviewed changes

address comments

b621563

Signed-off-by: Alex Chi <[email protected]>

skyzh requested a review from Connor1996 November 10, 2020 02:15

skyzh force-pushed the refactor-for-memtable branch from f13a71d to b621563 Compare November 10, 2020 02:15

Merge branch 'master' into refactor-for-memtable

aa86661

BusyJay reviewed Nov 12, 2020

View reviewed changes

skyzh added 3 commits November 13, 2020 18:07

remove agateoptions methods

940c52d

Signed-off-by: Alex Chi <[email protected]>

use mutex

0607327

Signed-off-by: Alex Chi <[email protected]>

Merge branch 'refactor-for-memtable' of https://github.com/tikv/agatedb…

e327656

… into refactor-for-memtable

skyzh requested a review from BusyJay November 13, 2020 10:09

Merge branch 'master' into refactor-for-memtable

61b45e7

BusyJay reviewed Nov 13, 2020

View reviewed changes

src/error.rs Outdated Show resolved Hide resolved

BusyJay reviewed Nov 13, 2020

View reviewed changes

src/db/opt.rs Outdated Show resolved Hide resolved

BusyJay reviewed Nov 13, 2020

View reviewed changes

skyzh added 3 commits November 17, 2020 13:19

add all options

089adc6

Signed-off-by: Alex Chi <[email protected]>

remove poison error

2bed2a5

Signed-off-by: Alex Chi <[email protected]>

fix ce

fd68fb6

Signed-off-by: Alex Chi <[email protected]>

skyzh requested a review from BusyJay November 17, 2020 05:22

Connor1996 approved these changes Nov 17, 2020

View reviewed changes

BusyJay reviewed Nov 17, 2020

View reviewed changes

resolve comments

5e7395a

Signed-off-by: Alex Chi <[email protected]>

skyzh requested a review from BusyJay November 17, 2020 07:30

revert decode

9ee5b23

Signed-off-by: Alex Chi <[email protected]>

BusyJay approved these changes Nov 17, 2020

View reviewed changes

skyzh merged commit 87f5412 into master Nov 18, 2020

skyzh deleted the refactor-for-memtable branch November 24, 2020 04:58

	let mut entry_data = self.data.slice(start_offset as usize..end_offset as usize);
	let mut header = Header::default();
	header.decode(&mut entry_data);

	// TODO: merge this truncate with the following key truncate
	if header.overlap > self.perv_overlap {
	self.key.truncate(self.perv_overlap as usize);
	self.key.extend_from_slice(
	&self.base_key[self.perv_overlap as usize..header.overlap as usize],
	);
	}
	self.perv_overlap = header.overlap;

	let diff_key = &entry_data[..header.diff as usize];
	self.key.truncate(header.overlap as usize);
	self.key.extend_from_slice(diff_key);
	self.val = entry_data.slice(header.diff as usize..);

agate: refactor for memtable impl #37

agate: refactor for memtable impl #37

Conversation

skyzh commented Nov 3, 2020 • edited Loading

skyzh commented Nov 4, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skyzh Nov 13, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skyzh commented Nov 13, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Connor1996 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skyzh commented Nov 3, 2020 •

edited

Loading

skyzh Nov 13, 2020 •

edited

Loading