Skip to content
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

Implement static linking #80

Merged
merged 5 commits into from Nov 1, 2016
Merged

Implement static linking #80

merged 5 commits into from Nov 1, 2016

Conversation

kaedroho
Copy link
Contributor

@kaedroho kaedroho commented Oct 28, 2016

Part of #69

Motivation

Currently, RocksDB is linked dynamically. There's a few drawbacks to this approach:

  1. End users of applications built using this wrapper must install exactly the same version of RocksDB that the application was built with. RocksDB is not included in many package managers at the moment so this either needs to be built from source or provided by the application author.
  2. There's a problem with tcmalloc (used by RocksDB) clashing with jemalloc (used by Rust) that causes random segfaults
  3. A developer using rust-rocksdb can choose which version of RocksDB to link with. This makes maintainability of rust-rocksdb more difficult as it must support mulltiple versions of RocksDB. This also makes it difficult to implement features that only exist a new version of RocksDB if it still has to support older versions.

These issues can be solved by statically linking RocksDB instead.

  1. RocksDB is compiled in to the applications binary. End users don't need to install anything.
  2. We can now control how RocksDB is built to make sure it doesn't pull in tcmalloc
  3. Only one version of RocksDB can be used by the wrapper. This version is controlled the wrappers authors.

Implementation

A new sub crate has been added to this repo rocksdb-sys. This includes the FFI bindings and a build script for RocksDB/Snappy. It is based on the work in the Ethcore fork of this repo (https://github.com/ethcore/rust-rocksdb) which itself is based on the rocksdb-sys crate (https://github.com/jsgf/rocksdb-sys)

The build script is written in Rust and uses gcc-rs to talk to the the C++ compiler. It works on Windows (tested with VC++ 2015) and Linux.

RocksDB and Snappy's source code are pulled in with git submodules. This means that developers of rust-rocksdb will need to run git submodule init and git submodule update before developing. This does not affect users of the crate as the submodules are bundled within the crate at the point of packaging it. Users who directly link to the git repo in their cargo dependencies will take a little longer on the first build, submodules are handled automatically by cargo and are cached as well.

Updating RocksDB

RocksDB is currently pinned to the latest commit of the 4.13 maintenance branch.

Instead of using the makefile, we pass a list of .cc files in RocksDB to gcc-rs. The makefile is not used so that we can perform the build for both Linux and Windows platforms from the same build script.

The build script loads a list of .cc files from a text file rocksdb_lib_sources.txt. This is generated using a Makefile (that calls RocksDB's makefile to get the sources list) and committed into the git repo.

The process for updating RocksDB is as follows:

  • cd into rocksdb-sys/rocksdb.
  • git checkout the commit hash of the new version
  • cd into rocksdb-sys
  • run make gen_lib_sources
  • Change the commit sha and date in rocksdb-sys/build_version.cc (it would be nice to automate this in build.rs)
  • Test and commit those changes into git

There's a chance that they may change the build process/makefile causing the above steps to not work. I think it would be best to tackle these issues as they come; I don't expect they will be very common.

Alternative solutions

Pulling in rocksdb source code with git submodules

I chose to use git-submodules, despite concerns about the impact on build time when having to download the history of RocksDB.

  • RocksDB will be bundled in the crate that is uploaded to crates.io so this won't afect most users
  • Users who do reference the git repo from their Cargo.toml will have to download the RocksDB repo via the submodule, but this only happens on the first build as it is cached thereafter
  • It takes less than a minute for me to download (10Mbit connection)
  • I'm against making decisions based on performance concerns before actually trying it out

Alternative solution 1: Bundle the RocksDB source code in the repo

This is what the ethcore fork does

This would be my second choice if we find git-submodules to be a pain. I wanted to avoid this for the following reasons:

  • Avoid bloat of this repo over time
  • It's very tempting to make changes to the bundled version of RocksDB, but this will make maintainability a pain

Alternative solution 2: Download RocksDB during compile time

This is what the ngaut fork does.

I think this solution is horrible. You can find my thoughts about it in this comment: #69 (comment)

Compiling RocksDB using a build.rs file

I decided to go with this as we can assume that all the platforms this project will be built on will have the ability to compile and run Rust code. The only dependency is a C++ compiler.

Alternative solution 1: Call the makefile from build.rs

I think this would be fine for Linux/Mac support but we will need something separate for Windows which uses CMake/MSBuild.

Alternative solution 2: Use a bash script instead of build.rs

This is an extension of "solution 1". It is what the ngaut fork does. I didn't like this for the following reasons:

  • We would need to implement a separate build script for Windows
  • We can already do a lot of the things that bash can do in Rust. But in a cross-platform way


fn main() {
build_rocksdb();
build_snappy();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should the order of these two be reversed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both of these generate a static library (.a file) that are passed to the linker which links them together when the final binary is built. They're not aware of each other at this point so the order they are built in doesn't matter.

@@ -0,0 +1 @@
db/auto_roll_logger.cc db/builder.cc db/c.cc db/column_family.cc db/compacted_db_impl.cc db/compaction.cc db/compaction_iterator.cc db/compaction_job.cc db/compaction_picker.cc db/convenience.cc db/range_del_aggregator.cc db/db_filesnapshot.cc db/dbformat.cc db/db_impl.cc db/db_impl_debug.cc db/db_impl_readonly.cc db/db_impl_experimental.cc db/db_info_dumper.cc db/db_iter.cc db/external_sst_file_ingestion_job.cc db/experimental.cc db/event_helpers.cc db/file_indexer.cc db/filename.cc db/flush_job.cc db/flush_scheduler.cc db/forward_iterator.cc db/internal_stats.cc db/log_reader.cc db/log_writer.cc db/managed_iterator.cc db/memtable_allocator.cc db/memtable.cc db/memtable_list.cc db/merge_helper.cc db/merge_operator.cc db/repair.cc db/snapshot_impl.cc db/table_cache.cc db/table_properties_collector.cc db/transaction_log_impl.cc db/version_builder.cc db/version_edit.cc db/version_set.cc db/wal_manager.cc db/write_batch.cc db/write_batch_base.cc db/write_controller.cc db/write_thread.cc db/xfunc_test_points.cc memtable/hash_cuckoo_rep.cc memtable/hash_linklist_rep.cc memtable/hash_skiplist_rep.cc memtable/skiplistrep.cc memtable/vectorrep.cc port/stack_trace.cc port/port_posix.cc table/adaptive_table_factory.cc table/block_based_filter_block.cc table/block_based_table_builder.cc table/block_based_table_factory.cc table/block_based_table_reader.cc table/block_builder.cc table/block.cc table/block_prefix_index.cc table/bloom_block.cc table/cuckoo_table_builder.cc table/cuckoo_table_factory.cc table/cuckoo_table_reader.cc table/flush_block_policy.cc table/format.cc table/full_filter_block.cc table/get_context.cc table/iterator.cc table/merger.cc table/meta_blocks.cc table/sst_file_writer.cc table/plain_table_builder.cc table/plain_table_factory.cc table/plain_table_index.cc table/plain_table_key_coding.cc table/plain_table_reader.cc table/persistent_cache_helper.cc table/table_properties.cc table/two_level_iterator.cc tools/dump/db_dump_tool.cc util/arena.cc util/bloom.cc util/build_version.cc util/cf_options.cc util/clock_cache.cc util/coding.cc util/comparator.cc util/compaction_job_stats_impl.cc util/concurrent_arena.cc util/crc32c.cc util/db_options.cc util/delete_scheduler.cc util/dynamic_bloom.cc util/env.cc util/env_chroot.cc util/env_hdfs.cc util/env_posix.cc util/event_logger.cc util/file_util.cc util/file_reader_writer.cc util/filter_policy.cc util/hash.cc util/histogram.cc util/histogram_windowing.cc util/instrumented_mutex.cc util/iostats_context.cc util/io_posix.cc util/log_buffer.cc util/logging.cc util/lru_cache.cc util/memenv.cc util/murmurhash.cc util/options.cc util/options_helper.cc util/options_parser.cc util/options_sanity_check.cc util/perf_context.cc util/perf_level.cc util/random.cc util/rate_limiter.cc util/sharded_cache.cc util/slice.cc util/sst_file_manager_impl.cc util/statistics.cc util/status.cc util/status_message.cc util/string_util.cc util/sync_point.cc util/thread_local.cc util/thread_status_impl.cc util/thread_status_updater.cc util/thread_status_updater_debug.cc util/thread_status_util.cc util/thread_status_util_debug.cc util/threadpool_imp.cc util/transaction_test_util.cc util/xfunc.cc util/xxhash.cc utilities/backupable/backupable_db.cc utilities/blob_db/blob_db.cc utilities/convenience/info_log_finder.cc utilities/checkpoint/checkpoint.cc utilities/compaction_filters/remove_emptyvalue_compactionfilter.cc utilities/document/document_db.cc utilities/document/json_document_builder.cc utilities/document/json_document.cc utilities/env_mirror.cc utilities/env_registry.cc utilities/flashcache/flashcache.cc utilities/geodb/geodb_impl.cc utilities/leveldb_options/leveldb_options.cc utilities/memory/memory_util.cc utilities/merge_operators/put.cc utilities/merge_operators/max.cc utilities/merge_operators/string_append/stringappend2.cc utilities/merge_operators/string_append/stringappend.cc utilities/merge_operators/uint64add.cc utilities/option_change_migration/option_change_migration.cc utilities/options/options_util.cc utilities/persistent_cache/persistent_cache_tier.cc utilities/persistent_cache/volatile_tier_impl.cc utilities/persistent_cache/block_cache_tier_file.cc utilities/persistent_cache/block_cache_tier_metadata.cc utilities/persistent_cache/block_cache_tier.cc utilities/redis/redis_lists.cc utilities/simulator_cache/sim_cache.cc utilities/spatialdb/spatial_db.cc utilities/table_properties_collectors/compact_on_deletion_collector.cc utilities/transactions/optimistic_transaction_impl.cc utilities/transactions/optimistic_transaction_db_impl.cc utilities/transactions/transaction_base.cc utilities/transactions/transaction_db_impl.cc utilities/transactions/transaction_db_mutex_impl.cc utilities/transactions/transaction_lock_mgr.cc utilities/transactions/transaction_impl.cc utilities/transactions/transaction_util.cc utilities/ttl/db_ttl_impl.cc utilities/date_tiered/date_tiered_db_impl.cc utilities/write_batch_with_index/write_batch_with_index.cc utilities/write_batch_with_index/write_batch_with_index_internal.cc
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this necessary to include, since it gets generated in the Makefile?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, the idea of this is that we don't have to run any Makefiles on an end-user's machine which may not have make installed (eg, Windows). The makefile is ran only when the RocksDB submodule is updated.

@spacejam
Copy link
Member

@ngaut would an approach like this work well for use cases you've encountered? Can you see any possible complications?

@kaedroho are there any tweaks like the ones mentioned here that can mitigate submodule checkout delay?

@ngaut
Copy link
Contributor

ngaut commented Oct 29, 2016

@spacejam I don't actually like the idea. It's not clean. But it's acceptable for those users who is using Windows. Thank you for your excellent work! @kaedroho

@kaedroho
Copy link
Contributor Author

@kaedroho are there any tweaks like the ones mentioned here that can mitigate submodule checkout delay?

Looks like that may be of help for developers, I'll try it out later today. It doesn't look like Cargo does this when installing directly from git though.

I don't actually like the idea. It's not clean.

I'm not aiming to make a perfectly "clean" solution first time. I often find solutions change a lot over time anyway so it's not worth overthinking it now. We should gradually improve it over time when we have more experience maintaining it.

My main goal with this solution is to make it as easy as possible for end users to install this library, and even if that makes it less "clean" to look at, it's a good tradeoff in my opinion *. This is the reason why I went with the build.rs approach over a bash script and bundling the dependencies instead of downloading them at compile-time.

* Most people care more about how easy a library is to install than how the build system is implemented. Any problems users encounter at install-time can be a blocker to adoption.

@kaedroho
Copy link
Contributor Author

It appears that the name rocksdb-sys has already been taken on crates.io. This will be a problem as that sub-crate needs to be published separately.

We'll either need to rename it, negotiate with the owner of that crate name to get the keys or combine the sub-crate into the main one.

@spacejam
Copy link
Member

In my opinion, library user experience > library developer experience, but it would be detrimental to long-term contributor motivation if the library is too gross for them to bother with. This was originally one of the first Rust projects I created, and it definitely has its gross elements, which may be a big reason why there is such a wide diaspora among forks :) I'd like to improve the contributor-friendliness of this library as time goes on.

@ngaut I'd like to be particularly encouraging of your improvements over time, and if you see specific problems with this that make it harder to combine our efforts, I'd like to know about them so I can think about ways to improve the situation.

@kaedroho I'm in agreement that we should move forward with this quickly and maintain momentum. I'm in favor of getting this in as soon as we get the submodule name sorted out. re: rocksdb-sys I don't care so much about ownership over that name on crates.io. Feel free to pick another name that you feel is appropriate.

@steveklabnik a looooong time ago on IRC I interpreted a remark of yours as recommending that I not include an expensive build.rs / compilation phase in this library. I'd like to strike a nice balance here between end-user friendliness (pull in the crate and good to go), operational considerations (big user finds bug in rocksdb version X, learns it's fixed in X+1, wants to use that version as easily as possible from here), and library developer friendliness (being as idiomatic as possible, supporting multiple platforms easily, avoiding big delays from checkouts etc...). Do you know of any current idioms for handling FFI libraries that would improve on what we've got in this PR so far?

@alexreg
Copy link
Contributor

alexreg commented Oct 29, 2016

Are we all good to merge here? I'd like to get on #69 (comment) as soon as possible, if @spacejam is okay with that.

@alexreg
Copy link
Contributor

alexreg commented Oct 30, 2016

I'm not sure I like this approach of redoing the build script in Rust, and maintaining it separately. Doesn't RocksDB support a Windows build anyway?

@alexreg
Copy link
Contributor

alexreg commented Oct 30, 2016

I merged this myself to do some experimenting. One note: there's a real mixture of tabs and spaces for indentation in some files!

@kaedroho
Copy link
Contributor Author

kaedroho commented Oct 31, 2016

@alexreg Cheers for looking at this!

I'm not sure I like this approach of redoing the build script in Rust, and maintaining it separately. Doesn't RocksDB support a Windows build anyway?

Yep it does. But the Windows builds are done through CMake/MSBuild, which is completely separate to the Makefile used on Linux/Mac.

A few potential downsides to using it over the current implementation are:

  • We have to support two separate build systems (instead of calling gcc-rs, which works as a wrapper for Visual C++ too).
  • I'm not sure if CMake is included with Visual Studio so this may add an extra dependency for users (ideally, users should just need either Visual Studio or MinGW installed and it'll just work)
  • The CMake configuration doesn't appear to support the GNU toolchain

One note: there's a real mixture of tabs and spaces for indentation in some files!

Sorry about this. Some of the code was copied from the ethcore fork and it looks like they like tabs. I can fix these tonight

@steveklabnik
Copy link
Contributor

@steveklabnik a looooong time ago on IRC I interpreted a remark of yours as recommending that I not include an expensive build.rs / compilation phase in this library

I don't remember that conversation 😅 not saying that you're wrong or something, I just don't remember the context, so I can't say if this follows that or not.

@alexreg
Copy link
Contributor

alexreg commented Oct 31, 2016

Fair points, @kaedroho. I suppose it's not ideal, but probably the least bad solution!

@kaedroho
Copy link
Contributor Author

kaedroho commented Nov 1, 2016

Fixed tabs and rebased

@spacejam
Copy link
Member

spacejam commented Nov 1, 2016

Thanks! We can figure out the naming of the submodule later. Merging this now to keep things moving.

@spacejam spacejam merged commit a372ea6 into rust-rocksdb:master Nov 1, 2016
@kaedroho kaedroho deleted the static-linking branch November 1, 2016 09:44
BusyJay pushed a commit to BusyJay/rust-rocksdb that referenced this pull request Jul 7, 2017
zhangjinpeng87 pushed a commit to zhangjinpeng87/rust-rocksdb that referenced this pull request Apr 19, 2019
Update RocksDB to include the following changes:
018aa1972 Titan: fix error message formatting (rust-rocksdb#84)
ee2f17ca8 Fix crash with memtable prefix bloom and key out of prefix extractor domain (#5190)
16ace7c47 Titan: Add snapshot related test (rust-rocksdb#80)
ee8b59301 Titan: Fix iterator fail to pass SuperVersion to underlying iterator (rust-rocksdb#79)

Signed-off-by: Yi Wu yiwu@pingcap.com
BusyJay pushed a commit to BusyJay/rust-rocksdb that referenced this pull request Jul 23, 2020
Update RocksDB to include the following changes:
018aa1972 Titan: fix error message formatting (rust-rocksdb#84)
ee2f17ca8 Fix crash with memtable prefix bloom and key out of prefix extractor domain (#5190)
16ace7c47 Titan: Add snapshot related test (rust-rocksdb#80)
ee8b59301 Titan: Fix iterator fail to pass SuperVersion to underlying iterator (rust-rocksdb#79)

Signed-off-by: Yi Wu yiwu@pingcap.com
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants