Updated rocksdb submodule to v4.11.2. #2

alexreg · 2016-10-25T23:27:40Z

Fixed memory leakage bug in tests/c_test.rs

Fixed memory leakage bug in `tests/c_test.rs`

Added missing `rocksdb_compactionfilter_set_ignore_snapshots` function.

jsgf · 2016-10-26T16:07:31Z

src/lib.rs

-//! Raw binding for RocksDB.
-//!
-//! This is just a thin binding to the RocksDB C API, mostly generated
-//! by rust-bindgen and lightly edited. This is intended to underpin a


Did you regenerate this with bindgen or some other tool, or manually edit it?

I regenerated it, then manually edited. Sorry, might have lost a few custom changes! Feel free to point them out.

My concern is larger than that.

Thus far, I've been incrementally updating this file to reflect changes in the RocksDB c.h API, so that changes to this files are more or less 1:1 with changes in c.h.

Your set of changes represent a total rewrite. I'm not sure:

which changes are the result of a different generator

which changes are bugfixes

which changes are aesthetic/formatting fixes

which changes are platform porting fixes

which changes are RocksDB API changes

It's good that you've started to break it down into separate commits, but it's still far from "one concern per commit".

And as a result, I'm not sure how to maintain this in future. Can I just regenerate it mechanically? If I did, what things would I need to port into the newly generated version to maintain compatibility?

You're also changing the API presented by this crate, which is something that other crates depend on, so you're going to cause downstream breakage. If you're going to do that, there needs to be a good/convincing reason for it.

I would prefer to see:

the minimal set of changes needed to update to the new RocksDB

the minimal set of changes needed to get it working on Mac OS

the minimal set of changes needed to fix the test bugs

anything else

as separate commits which we can review individually.

jsgf · 2016-10-26T16:10:48Z

build.rs

        .arg(format!("INSTALL_PATH={}", out_dir));

    if let Ok(jobs) = num_jobs {
        cmd.arg(format!("-j{}", jobs));
    }

-    cmd.arg("install-static");
+    cmd.arg("install");


By default, rocksdb was using its own allocator (tcmalloc), which conflicted with jemalloc used by Rust. The only way I could resolve it was by statically linking rocksdb into the crate so that it uses the Rust allocator.

Also I prefer static linking to avoid more dependencies for the executable. The c<->rust binding is pretty fragile, and I'm sure that the RocksDB ABI doesn't change from version to version, so independently updating rocksdb from the Rust code using it seems risky.

Ah. The problem with static linking is that there is no static libstdc++ on OS X...

Well, can you statically link rocksdb and dynamically link libstdc++? I think that's what happens on Linux.

I'm afraid you can't dynamically link to a library from a static library. Unless you do it at run-time (rather than load-time), using dlopen or something. This is definitely a problem, although I see why you wanted to avoid bringing two different allocators into play.

I'm afraid you can't dynamically link to a library from a static library.

Sure, but that's not quite what I'm talking about.

RocksDB is written in C++, so it has a dependency on libstdc++, but the library itself (the .rlib) doesn't have a direct dependency on it - it just needs to make sure when your executable is finally linked it includes a linkage to libstdc++. And because the rest of RocksDB is static, it effectively becomes part of your main executable.

I believe that rustc uses clang++ as its linker on Mac OS, so the requirement for libstdc++ will be automatically fulfilled.

Have you tried this and had it fail? If so, how did it fail?

I've been told rustc uses ld as the linker, in fact, but not 100% sure. Anyway, I got lots of "undefined symbol" errors if I tried to link it statically.

jsgf

Why? In principle they could be NULL pointers - or did you only remove the option on the functions which must be supplied?

alexreg · 2016-10-26T16:13:12Z

I believe all the functions must be supplied, but I'll double check that.

jsgf

I think it's better these exactly match the rocksdb headers. These shouldn't ever be visible to application code (ie, some nice wrapper should re-present this API in a cleaner form).

alexreg · 2016-10-26T16:15:26Z

I’ve been told that’s not possible, but will have a look into it…

On 26 Oct 2016, at 17:14, Jeremy Fitzhardinge [email protected] wrote:

@jsgf commented on this pull request.

In build.rs #2:
     .arg(format!("INSTALL_PATH={}", out_dir));
 if let Ok(jobs) = num_jobs {
     cmd.arg(format!("-j{}", jobs));
 }
cmd.arg("install-static");

cmd.arg("install");
Well, can you statically link rocksdb and dynamically link libstdc++? I think that's what happens on Linux.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub #2, or mute the thread https://github.com/notifications/unsubscribe-auth/AAEF3KEXnqd5jWDBpOfqgrOC0SgTjTZXks5q33xbgaJpZM4KgmM9.

alexreg · 2016-10-26T16:16:09Z

What bit of code were you referring to with your last comment? (I think you just submitted it as a regular comment.)

alexreg · 2016-10-27T01:26:30Z

Hmm. I force-pushed a modified commit, and that seems to have made Travis unhappy, even though all the tests should be succeeding now. Sorry about that.

jsgf · 2016-10-27T15:23:29Z

tests/c_test.rs

@@ -1,8 +1,3 @@
-/*
-   Copyright (c) 2011 The LevelDB Authors. All rights reserved.


Don't strip off copyright headers.

This was probably something accidental. Easy enough to put back. (I guess you ported the C test code directly?)

jsgf · 2016-10-27T15:26:17Z

src/lib.rs

-pub const rocksdb_block_based_table_index_type_binary_search: c_uint = 0;
-pub const rocksdb_block_based_table_index_type_hash_search: c_uint = 1;
-
-pub const rocksdb_no_compression: c_uint = 0;


Leave all these as they are, to exactly match the C headers. You can wrap them up as Rust enums in the higher-level API.

Yes, I reverted to the constants after all. I'm not sure you can say they exactly match the C headers, since the C header defines a nameless enum, but there you go! The semantics are similar.

As for the API changes, I see what you mean. What I did is regenerate the bindings using bindgen, to make things easier, then edit by hand. As far as I could tell your bindings were missing a lot of functions that are (now) in RocksDB... maybe we could just take a look at where the API clashes with your old definitions? The types should now all agree, at least.

jsgf · 2016-10-27T15:40:46Z

src/lib.rs

-//! Raw binding for RocksDB.
-//!
-//! This is just a thin binding to the RocksDB C API, mostly generated
-//! by rust-bindgen and lightly edited. This is intended to underpin a


My concern is larger than that.

Thus far, I've been incrementally updating this file to reflect changes in the RocksDB c.h API, so that changes to this files are more or less 1:1 with changes in c.h.

Your set of changes represent a total rewrite. I'm not sure:

which changes are the result of a different generator

which changes are bugfixes

which changes are aesthetic/formatting fixes

which changes are platform porting fixes

which changes are RocksDB API changes

It's good that you've started to break it down into separate commits, but it's still far from "one concern per commit".

And as a result, I'm not sure how to maintain this in future. Can I just regenerate it mechanically? If I did, what things would I need to port into the newly generated version to maintain compatibility?

You're also changing the API presented by this crate, which is something that other crates depend on, so you're going to cause downstream breakage. If you're going to do that, there needs to be a good/convincing reason for it.

I would prefer to see:

the minimal set of changes needed to update to the new RocksDB

the minimal set of changes needed to get it working on Mac OS

the minimal set of changes needed to fix the test bugs

anything else

as separate commits which we can review individually.

jsgf · 2016-10-28T18:37:55Z

I just pushed a "rocksdb-4.11.2" branch with a minimal set of changes to support the changes in c.h. I haven't tested it yet, so I'd appreciate it if you could double-check my changes.

"cargo test" also works on my Mac (Mac OS 10.11.6), which just needed the JEMALLOC change in build.rs.

jsgf · 2016-10-28T20:04:21Z

Ah, that was xcode 7.something. Xcode 8 fails.

alexreg · 2016-10-29T00:17:22Z

@jsgf Okay, so I tried this on a new machine in fact, and initially got some errors when running cargo test.

dyld: Library not loaded: /usr/local/opt/lz4/lib/liblz4.1.dylib
  Referenced from: /Users/alex/Software/rocksdb-sys/target/debug/build/rocksdb-sys-979ce3bc9a6df222/out/lib/librocksdb.4.11.dylib
  Reason: image not found

Also got similar errors for snappy, lz, jemalloc. When I installed them all, the build worked. This is on OS X 10.11 however, not 10.12. What error did you get with Xcode 8 tools?

alexreg added 4 commits October 26, 2016 00:25

Updated rocksdb submodule to v4.11.2.

a369c4f

Fixed memory leakage bug in `tests/c_test.rs`

Removed Option<_> wrappers around function types.

f6e8de1

Enum variants now have CamelCase style.

113e7ec

Added comments for various sections of functions.

bfecf0c

Added missing `rocksdb_compactionfilter_set_ignore_snapshots` function.

alexreg force-pushed the master branch from 3e9ef0e to bfecf0c Compare October 26, 2016 15:52

jsgf reviewed Oct 26, 2016

View reviewed changes

Reverted to Option<_> types for function pointers.

5edcb4a

alexreg force-pushed the master branch from e4c9fc3 to 5edcb4a Compare October 27, 2016 01:24

jsgf reviewed Oct 27, 2016

View reviewed changes

Added module documentation.

ba1a11b

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updated rocksdb submodule to v4.11.2. #2

Updated rocksdb submodule to v4.11.2. #2

alexreg commented Oct 25, 2016

jsgf Oct 26, 2016

alexreg Oct 26, 2016

jsgf Oct 27, 2016

jsgf Oct 26, 2016

alexreg Oct 26, 2016

jsgf Oct 26, 2016

alexreg Oct 27, 2016

jsgf Oct 27, 2016

alexreg Oct 28, 2016

jsgf left a comment

alexreg commented Oct 26, 2016

jsgf left a comment

alexreg commented Oct 26, 2016

alexreg commented Oct 26, 2016

alexreg commented Oct 27, 2016

jsgf Oct 27, 2016

alexreg Oct 28, 2016

jsgf Oct 27, 2016

alexreg Oct 28, 2016

alexreg Oct 28, 2016

jsgf Oct 27, 2016

jsgf commented Oct 28, 2016

jsgf commented Oct 28, 2016

alexreg commented Oct 29, 2016

		@@ -1,8 +1,3 @@
		/*
		Copyright (c) 2011 The LevelDB Authors. All rights reserved.

Updated rocksdb submodule to v4.11.2. #2

Are you sure you want to change the base?

Updated rocksdb submodule to v4.11.2. #2

Conversation

alexreg commented Oct 25, 2016

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

jsgf left a comment

Choose a reason for hiding this comment

alexreg commented Oct 26, 2016

jsgf left a comment

Choose a reason for hiding this comment

alexreg commented Oct 26, 2016

alexreg commented Oct 26, 2016

alexreg commented Oct 27, 2016

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

jsgf commented Oct 28, 2016

jsgf commented Oct 28, 2016

alexreg commented Oct 29, 2016