export mulit_get interface #59

BusyJon · 2017-05-23T12:16:11Z

PTAL @zhangjinpeng1987

siddontang · 2017-05-23T12:53:17Z

Please add test.

andelf

doubt about error handing, return types, and Vec<&[u8]> as parameter type.

andelf · 2017-05-23T18:44:27Z

librocksdb_sys/src/lib.rs

+                                 num_keys: size_t,
+                                 keys_list: *const *const u8,
+                                 keys_list_sizes: *const size_t,
+                                 values_list: *mut *mut u8,


should it be *mut *const u8 ?

DBVector type use *mut u8.

I think it should be *mut *mut u8. Both the values_list and its content needs to be dropped explicitly.

I understand. value_list is created by crocksdb, malloc().
so we need to own it in DBVector and drop() it by free() via ffi.

andelf · 2017-05-23T18:48:40Z

src/rocksdb.rs

+                            cf: &CFHandle,
+                            keys_vec: Vec<&[u8]>,
+                            readopts: &ReadOptions)
+                            -> Result<Option<Vec<DBVector>>, String> {


should be Result<Vec<Option<DBVector>>, _>
or more precisely
Vec<Result<Option<DBVector>, String>>

since we use Option<> to denote Status::NotFound

andelf · 2017-05-23T18:49:15Z

src/rocksdb.rs

@@ -513,6 +513,81 @@ impl DB {
        self.get_cf_opt(cf, key, &ReadOptions::new())
    }

+    pub fn multi_get_cf_opt(&self,
+                            cf: &CFHandle,
+                            keys_vec: Vec<&[u8]>,


should this be &[&[u8]] ?

well, i'm not sure, maybe &[Vec].

&[Vec] is kind of strange. we've already use &[u8] to denote Slice.
I think &[&[u8]] is ok.
@siddontang

andelf · 2017-05-23T18:53:57Z

src/rocksdb.rs

+            let keys_list_sizes = keys_list_sizes.as_ptr();
+            let mut value_list = Vec::<*mut u8>::with_capacity(num_keys);
+            let mut value_list_sizes = Vec::<size_t>::with_capacity(num_keys);
+            ffi_try!(crocksdb_multi_get_cf(self.inner,


wrong.
since errs is c array of error string, not a pointer to a c string

andelf · 2017-05-23T18:55:02Z

src/rocksdb.rs

+    pub fn multi_get_opt(&self,
+                         keys_vec: Vec<&[u8]>,
+                         readopts: &ReadOptions)
+                         -> Result<Option<Vec<DBVector>>, String> {


if you export DB::DefaultColumnFamily()
you can impl all the other fn by multi_get_cf_opt

we can use cf_handle("default") to get the default CF handle.

BusyJay · 2017-05-24T04:45:14Z

librocksdb_sys/src/lib.rs

+                              keys_list: *const *const u8,
+                              keys_list_sizes: *const size_t,
+                              values_list: *mut *mut u8,
+                              values_list_sizes: *const size_t,


Should be *mut.

BusyJay · 2017-05-24T04:46:55Z

librocksdb_sys/src/lib.rs

+                                 num_keys: size_t,
+                                 keys_list: *const *const u8,
+                                 keys_list_sizes: *const size_t,
+                                 values_list: *mut *mut u8,


I think it should be *mut *mut u8. Both the values_list and its content needs to be dropped explicitly.

BusyJay · 2017-05-24T04:49:09Z

src/rocksdb.rs

+            }
+            let keys_list = keys_list.as_ptr();
+            let keys_list_sizes = keys_list_sizes.as_ptr();
+            let mut value_list = Vec::<*mut u8>::with_capacity(num_keys);


Type annotation is redundant.

BusyJon · 2017-05-24T06:29:12Z

all comments considered, PTAL again. @zhangjinpeng1987 @siddontang @andelf @BusyJay

siddontang · 2017-05-24T06:32:09Z

src/rocksdb.rs

@@ -513,6 +513,71 @@ impl DB {
        self.get_cf_opt(cf, key, &ReadOptions::new())
    }

+    pub fn multi_get_cf_opt(&self,
+                            cf: &CFHandle,
+                            keys_vec: &[Vec<u8>],


s/keys/keys_vec/g

&[&[u8]] may be better?

/cc @BusyJay

siddontang · 2017-05-24T06:39:53Z

src/rocksdb.rs

@@ -513,6 +513,71 @@ impl DB {
        self.get_cf_opt(cf, key, &ReadOptions::new())
    }

+    pub fn multi_get_cf_opt(&self,


I think for easy use, we can declare the return type as Result<Vec<Option<DBVector>>, String>, If we meet any error in multi_get, we will return error directly.

/cc @BusyJay @andelf @zhangjinpeng1987

In what case get will return error?

to �name a few, db corruption, unsupported compression, io error, db busy?

BusyJay · 2017-05-24T07:13:42Z

src/rocksdb.rs

@@ -513,6 +513,71 @@ impl DB {
        self.get_cf_opt(cf, key, &ReadOptions::new())
    }

+    pub fn multi_get_cf_opt(&self,
+                            cf: &CFHandle,
+                            keys: &[Vec<u8>],


Should be &[&[u8]], so we don't need to move the key when query.

andelf · 2017-05-24T07:13:43Z

src/rocksdb.rs

+            let mut value_vec = Vec::with_capacity(num_keys);
+            for i in 0..num_keys {
+                if !err_list[i].is_null() {
+                    return Err(crocksdb_ffi::error_message(err_list[i]));


memory leaks?

strdup without free

There are still memory leaks. But the content in value_list and err_list should be dropped.

BusyJay

If we want to changed the return type from Vec to Result, the implementation in crocksdb.c can be changed directly.

BusyJay · 2017-05-24T10:49:42Z

src/rocksdb.rs

+            let mut value_vec = Vec::with_capacity(num_keys);
+            for i in 0..num_keys {
+                if !err_list[i].is_null() {
+                    return Err(crocksdb_ffi::error_message(err_list[i]));


There are still memory leaks. But the content in value_list and err_list should be dropped.

siddontang · 2017-05-25T03:42:10Z

librocksdb_sys/crocksdb/c.cc

@@ -854,6 +854,7 @@ void crocksdb_multi_get_cf(
      values_list_sizes[i] = 0;
      if (!statuses[i].IsNotFound()) {
        errs[i] = strdup(statuses[i].ToString().c_str());
+        break;


please update multi_get too.

siddontang · 2017-05-25T03:52:53Z

Any test?

BusyJay · 2017-05-25T03:48:21Z

src/rocksdb.rs

@@ -513,6 +513,71 @@ impl DB {
        self.get_cf_opt(cf, key, &ReadOptions::new())
    }

+    pub fn multi_get_cf_opt(&self,
+                            cf: &CFHandle,
+                            keys: &[&[u8]],


Looks like using generic type here is more suitable. like f<F: AsRef<[u8]>>(keys: &[F])

BusyJay · 2017-05-25T03:49:22Z

librocksdb_sys/crocksdb/c.cc

@@ -854,6 +854,7 @@ void crocksdb_multi_get_cf(
      values_list_sizes[i] = 0;
      if (!statuses[i].IsNotFound()) {
        errs[i] = strdup(statuses[i].ToString().c_str());
+        break;


errs should not be accessed as an array any more. Because only the first error is cared now. And copied values should be dropped.

siddontang · 2017-05-25T08:51:33Z

src/rocksdb.rs

+                                                value_sizes.as_mut_ptr(),
+                                                err);
+            if !err.is_null() {
+                return Err(crocksdb_ffi::error_message(err));


may still meet memory leak if value list has values.

BusyJay · 2017-05-25T08:55:39Z

librocksdb_sys/crocksdb/c.cc

-        errs[i] = strdup(statuses[i].ToString().c_str());
-      } else {
-        errs[i] = nullptr;
+        err = strdup(statuses[i].ToString().c_str());


This is wrong. err is a pointer which will be passed by value. Use char** err and SaveError instead. And should drop all the content in values_list too.

siddontang · 2017-05-25T10:02:49Z

any benchmark to show multi get is better than seek + next if we want to get data in a range.

BusyJon · 2017-05-25T17:26:28Z

benched on my own laptop

fetch one kv

test multi_get::bench_get_serially_in_memtable ... bench:         679 ns/iter (+/- 311)
test multi_get::bench_iterator_get_in_memtable ... bench:         961 ns/iter (+/- 372)
test multi_get::bench_multi_get_in_memtable    ... bench:       1,559 ns/iter (+/- 976)

fetch two kv

test multi_get::bench_get_serially_in_memtable ... bench:       1,451 ns/iter (+/- 644)
test multi_get::bench_iterator_get_in_memtable ... bench:       1,080 ns/iter (+/- 397)
test multi_get::bench_multi_get_in_memtable    ... bench:       2,321 ns/iter (+/- 1,370)

it seems using get to fetch one key is quick than seek, but when fetching more than one key, the cheap next make the whole process getting more faster.

siddontang · 2017-05-25T23:49:44Z

How about get from SST file?
For the result, I guess we can't use it in raft CF. @zhangjinpeng1987

zhangjinpeng87 · 2017-05-26T01:41:00Z

If there is only one memtable, seek is as fast as get. Please add bench for the case there are some sst files, since in the most cases there are some sst files.

BusyJon · 2017-05-26T07:13:59Z

benchmark result

with about 10 sst file and some keys in memtable, fetch 8 kvs which is in memtable
use get is faster than seek+next when fetching no more than 8 kv.

test multi_get::bench_get_serially ... bench:       2,270 ns/iter (+/- 39)
test multi_get::bench_iterator     ... bench:       3,425 ns/iter (+/- 240)
test multi_get::bench_multi_get    ... bench:       2,525 ns/iter (+/- 40)

by the way, the differ between different count fetching in the same condition indicates seek is expensive.

fetch 1 key     2,815 ns/iter (+/- 77)
fetch 2 keys    2,923 ns/iter (+/- 443) 
fetch 4 keys    3,161 ns/iter (+/- 78)
fetch 8 keys    3,349 ns/iter (+/- 35)

andelf · 2017-06-01T05:34:35Z

according to #62, this pr should be closed. 😿 @lishihai9017

export multi_get interface

4283ff4

andelf suggested changes May 23, 2017

View reviewed changes

BusyJay reviewed May 24, 2017

View reviewed changes

udpate

6b858c4

siddontang reviewed May 24, 2017

View reviewed changes

lishihai9017 added 2 commits May 24, 2017 14:32

update

9200b7c

update

943f5d8

siddontang reviewed May 24, 2017

View reviewed changes

update

109b5a6

BusyJay reviewed May 24, 2017

View reviewed changes

andelf reviewed May 24, 2017

View reviewed changes

update

d89ea59

andelf approved these changes May 24, 2017

View reviewed changes

BusyJay reviewed May 24, 2017

View reviewed changes

lishihai9017 added 2 commits May 25, 2017 10:24

update

9bf0b56

update

94ea06b

siddontang reviewed May 25, 2017

View reviewed changes

BusyJay reviewed May 25, 2017

View reviewed changes

lishihai9017 added 3 commits May 25, 2017 16:05

fix bugs & add test

0776882

update crocksdb multi_get* interface

da125df

update

442dbb5

siddontang reviewed May 25, 2017

View reviewed changes

BusyJay reviewed May 25, 2017

View reviewed changes

fix memory leaks

2e4002e

add bench for different ways to fetch multi-kv

7b8d192

BusyJon mentioned this pull request May 25, 2017

optimize raft log entry fetch tikv/tikv#1864

Merged

update bench case

3b857e7

remove unused warning

6d492a0

andelf mentioned this pull request May 31, 2017

export get pinnable slice #62

Merged

siddontang closed this Jun 1, 2017

siddontang mentioned this pull request Nov 28, 2017

Expose multi_get APIs #162

Closed

zhangjinpeng87 deleted the lishihai/add-mulit-get branch December 4, 2017 01:39

export mulit_get interface #59

export mulit_get interface #59

Conversation

BusyJon commented May 23, 2017

siddontang commented May 23, 2017

andelf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BusyJon May 24, 2017 • edited Loading

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

BusyJon commented May 24, 2017

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

BusyJay left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

siddontang commented May 25, 2017

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

siddontang commented May 25, 2017

BusyJon commented May 25, 2017

fetch one kv

fetch two kv

siddontang commented May 25, 2017

zhangjinpeng87 commented May 26, 2017

BusyJon commented May 26, 2017 • edited Loading

benchmark result

andelf commented Jun 1, 2017

BusyJon May 24, 2017 •

edited

Loading

BusyJon commented May 26, 2017 •

edited

Loading