user properties: clean up API #90

BusyJay · 2017-07-06T06:44:03Z

reduce allocation
collect lazily
strict access mode

disksing · 2017-07-06T07:05:21Z

librocksdb_sys/crocksdb/c.cc

-  crocksdb_table_properties_collection_t* props, char** errptr) {
+crocksdb_table_properties_collection_t *
+crocksdb_get_properties_of_all_tables(crocksdb_t *db, char **errptr) {
+  std::unique_ptr<crocksdb_table_properties_collection_t> props(


You can use make_unique().

But make_unique is introduced in c++14?

huachaohuang · 2017-07-06T09:46:10Z

librocksdb_sys/crocksdb/c.cc

@@ -1768,7 +1768,7 @@ void crocksdb_options_set_compression(crocksdb_options_t* opt, int t) {
  opt->rep.compression = static_cast<CompressionType>(t);
 }

-int crocksdb_options_get_compression(crocksdb_options_t* opt) {
+int crocksdb_options_get_compression(crocksdb_options_t *opt) {


Do we have any style agreement on this?

Clang-format with Google style

I don't think so. We should use librocksdb_sys/crocksdb/format-diff.sh to format c/c++ codes, which is adapted from rocksdb.

use the same style with RocksDB.

but, I guess this may be a bug with the format tool 😓

🤣 I know. We've borrowed the format-diff script, but leaves out .clang-format config.

I myself use clang-format with google style as command line arguments, which is enough for this project.

@andelf can you add the RocksDB clang format config?

Copying the .clang-format from top dir of rocksdb solves

huachaohuang · 2017-07-06T09:57:51Z

src/table_properties.rs


-pub fn new_table_properties_collection() -> TablePropertiesCollection {
-    TablePropertiesCollection::new()
-}

 pub struct TablePropertiesCollection {
    pub inner: *mut DBTablePropertiesCollection,


Don't need pub anymore :)

huachaohuang · 2017-07-06T10:04:54Z

src/table_properties.rs

-impl Drop for TableProperties {
-    fn drop(&mut self) {
+impl TableProperties {
+    fn from_ptr<'a>(ptr: *const DBTableProperties) -> &'a TableProperties {


Can we return a TableProperties<'a> and keep the inner pointer here?
It's a bit of magic for me to figure out how this work.

I don't think it's suitable that TableProperties contains a lifetime. Maybe we can rename it to TablePropertiesRef. But I believe &'a TableProperties is more clear and rusty than TablePropertiesRef. Actually I don't invent the magic by myself, both CStr and str are implemented via similar logic.

Seems this has already been discussed here, I think I'm OK with that.

andelf · 2017-07-06T09:46:19Z

librocksdb_sys/crocksdb/c.cc

+const crocksdb_user_collected_properties_t *
+crocksdb_table_properties_get_user_properties(
+    const crocksdb_table_properties_t *props) {
+  return (const crocksdb_user_collected_properties_t *)&props->rep


Better use c++ style cast

andelf · 2017-07-06T10:19:57Z

src/table_properties.rs

-            iter.next();
-        }
-        res
+    pub fn user_collected_properties(&self) -> UserCollectedPropertiesIter {


Maybe wrapping UserCollectedProperties struct is better.
Since we need fast index access for propties, not iterating over style access.

I don't get it. UserCollectedProperties is a map, how do you access it via index?

impl Index<&str> for it, wraps std::map operator[]

Oh, I see. I thought you want to get (k, v) pair at specific position.

huachaohuang · 2017-07-06T13:54:43Z

librocksdb_sys/crocksdb/c.cc

+const char* crocksdb_user_collected_properties_get(
+    const crocksdb_user_collected_properties_t* props, const char* key,
+    size_t klen, size_t* vlen) {
+  auto val = props->rep.find(std::string(key, klen));


Maybe we can iterate the map to avoid constructing the string?

It depends. I prefer using index access here.

But it invokes copying the key here?

Yes. But it compare less.

Seems C++ 14 can find without constructing the string, can we use it?

I'm not sure. I prefer stick to C++ 11 for now since rocksdb itself still just require C++ 11. It may be surprising if the wrapper requires a newer version of compiler.

huachaohuang · 2017-07-06T13:58:30Z

src/rocksdb.rs

@@ -1072,16 +1070,14 @@ impl DB {
        let limit_keys: Vec<*const u8> = ranges.iter().map(|x| x.end_key.as_ptr()).collect();
        let limit_keys_lens: Vec<_> = ranges.iter().map(|x| x.end_key.len()).collect();
        unsafe {
-            let props = new_table_properties_collection();
-            ffi_try!(crocksdb_get_properties_of_tables_in_range(self.inner,
+            let props = ffi_try!(crocksdb_get_properties_of_tables_in_range(self.inner,


Seems not formatted here?

Yes. I'm waiting Travis to tell me the exact code he likes.

huachaohuang · 2017-07-06T14:03:45Z

src/table_properties.rs

+    }
+}
+
+pub struct UserCollectedPropertiesIter<'a> {
    props: PhantomData<&'a TableProperties>,


This should change to UserCollectedProperties?

huachaohuang · 2017-07-06T15:43:00Z

LGTM

huachaohuang · 2017-07-07T04:29:50Z

Btw, a len() method can be helpful.

BusyJay · 2017-07-07T04:41:55Z

@zhangjinpeng1987 @disksing @andelf PTAL

huachaohuang · 2017-07-07T04:48:37Z

A len() for TablePropertiesCollection, thanks.

disksing · 2017-07-07T06:13:14Z

LGTM

BusyJay · 2017-07-07T06:21:07Z

@huachaohuang PTAL

huachaohuang

LGTM

- reduce allocation - collect lazily - strict access mode

zhangjinpeng87

LGTM

)" (#102) (#374)

BusyJay requested review from huachaohuang, disksing and zhangjinpeng87 July 6, 2017 06:44

disksing reviewed Jul 6, 2017

View reviewed changes

BusyJay force-pushed the busyjay/cleanup-properties branch from e4dfd82 to 7cb2901 Compare July 6, 2017 08:37

huachaohuang reviewed Jul 6, 2017

View reviewed changes

andelf reviewed Jul 6, 2017

View reviewed changes

huachaohuang reviewed Jul 6, 2017

View reviewed changes

BusyJay force-pushed the busyjay/cleanup-properties branch from b72feff to 7912996 Compare July 7, 2017 06:00

huachaohuang previously approved these changes Jul 7, 2017

View reviewed changes

user properties: clean up API (#90)

fcef2d6

- reduce allocation - collect lazily - strict access mode

BusyJay dismissed huachaohuang’s stale review via fcef2d6 July 7, 2017 09:34

BusyJay force-pushed the busyjay/cleanup-properties branch from e5d79d6 to fcef2d6 Compare July 7, 2017 09:34

zhangjinpeng87 approved these changes Jul 7, 2017

View reviewed changes

BusyJay merged commit a50c084 into master Jul 7, 2017

BusyJay deleted the busyjay/cleanup-properties branch July 7, 2017 15:28

yiwu-arbug mentioned this pull request Apr 30, 2019

Update to latest Titan #292

Closed

sre-bot added a commit that referenced this pull request Nov 4, 2019

titan: Revert "Relax warnings as errors so we can build on Clang 11 (#90

66e0f31

)" (#102) (#374)

user properties: clean up API #90

user properties: clean up API #90

Conversation

BusyJay commented Jul 6, 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

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

BusyJay Jul 6, 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

huachaohuang commented Jul 6, 2017

huachaohuang commented Jul 7, 2017

BusyJay commented Jul 7, 2017

huachaohuang commented Jul 7, 2017

disksing commented Jul 7, 2017

BusyJay commented Jul 7, 2017

huachaohuang left a comment

Choose a reason for hiding this comment

zhangjinpeng87 left a comment

Choose a reason for hiding this comment

BusyJay Jul 6, 2017 •

edited

Loading