enabling the jemalloc feature doesn't actually enable jemalloc

[zaidoon1]

See facebook/rocksdb#12364 & zaidoon1/rust-rocksdb#23

[cvijdea-bd]

Note that rust-rocksdb enables the unprefixed_malloc_on_supported_platforms feature of tikv-jemalloc-sys, hooking the actual malloc and free, so jemalloc is used to allocate memory, event if RocksDB is not properly told about it.

Also note that tikv-jemalloc-sys includes jemalloc as a git submodule and links it as a static library, so your approach of using the includes from the system jemalloc seems wrong; it should instead be

        if let Some(jemalloc_root) = std::env::var_os("DEP_JEMALLOC_ROOT") {
            config.include(Path::new(&jemalloc_root).join("include"));
        }

[cvijdea-bd]

It also looks like RocksDB will not directly use jemalloc even if ROCKSDB_JEMALLOC is defined, the allocator needs to be set explicitly using void rocksdb_lru_cache_options_set_memory_allocator(rocksdb_lru_cache_options_t* opt, rocksdb_memory_allocator_t* allocator) (and created with rocksdb_jemalloc_nodump_allocator_create).

[zaidoon1]

It also looks like RocksDB will not directly use jemalloc even if ROCKSDB_JEMALLOC is defined, the allocator needs to be set explicitly using void rocksdb_lru_cache_options_set_memory_allocator(rocksdb_lru_cache_options_t* opt, rocksdb_memory_allocator_t* allocator) (and created with rocksdb_jemalloc_nodump_allocator_create).

so for this point, I confirmed with rocksdb devs that this is not a requirement: facebook/rocksdb#12364 (comment)

[zaidoon1]

my current change (while not ideal because it doesn't link to jemalloc used by tikv but relies on the system jemalloc) does get rocksdb to recognize jemalloc because when i set db_opts.set_dump_malloc_stats(true);, i actually see rocksdb dumping jemalloc memory stats to LOG file which is how I confirmed my change fixed something

[zaidoon1]

I tried:

   if cfg!(feature = "jemalloc") {
        config.define("WITH_JEMALLOC", "ON");
        if let Some(jemalloc_root) = env::var_os("DEP_JEMALLOC_ROOT") {
            config.include(Path::new(&jemalloc_root).join("include"));
        }
    }

and that didn't get malloc stats to work

I also tried:

       if cfg!(feature = "jemalloc") {
       config.define("ROCKSDB_JEMALLOC", Some("1"));
       config.define("JEMALLOC_NO_DEMANGLE", Some("1"));
       if let Some(jemalloc_root) = env::var_os("DEP_JEMALLOC_ROOT") {
           config.include(Path::new(&jemalloc_root).join("include"));
       }
   }

and that failed the build as rocksdb couldn't find jemalloc. Did I misunderstand your suggestion?

[cvijdea-bd]

because it doesn't link to jemalloc used by tikv but relies on the system jemalloc

I think it doesn't actually link to the system jemalloc, but only uses the headers from it (the static jemalloc will be prioritized by the linker).

and that failed the build as rocksdb couldn't find jemalloc. Did I misunderstand your suggestion?

No, that seems good, it builds for me locally.

ROCKSDB_JEMALLOC is correct, it's required to make RocksDB know about jemalloc (so it's required for set_dump_malloc_stats to even try to do anything useful) - WITH_JEMALLOC is only valid as a cmake argument for rocksdb (and rust-rocksdb is not using cmake).

env::var_os("DEP_JEMALLOC_ROOT") should be giving you the location of tikv-jemalloc-sys's build directory, because librocksdb-sys directly depends on it, because it has a links = "jemalloc" entry in its Cargo.toml, and because it exports the cargo:root variable in its build script. (see https://doc.rust-lang.org/cargo/reference/build-script-examples.html#using-another-sys-crate)

[zaidoon1]

No, that seems good, it builds for me locally.

ok to be clear, I tried that on my mac and that failed, let me try on linux which is what i run in prod

[zaidoon1]

ok confirmed working on linux!

[zaidoon1]

it looks like what we want to do is something similar to what tikv rust-rocksdb does;https://github.com/tikv/rust-rocksdb/blob/05fc3f80ed50bac9932ca238e9dfbaadb7390965/librocksdb_sys/build.rs#L26C7-L26C26 to make sure the build doesn't fail on mac when trying to enable jemalloc. going to test this out.

[zaidoon1]

and confirmed working, I'll make the change on my end. Thanks for the help on this!

[Chaoses-Ib]

Note that rust-rocksdb enables the unprefixed_malloc_on_supported_platforms feature of tikv-jemalloc-sys, hooking the actual malloc and free, so jemalloc is used to allocate memory, event if RocksDB is not properly told about it.

Rust uses malloc on Linux by default. Does that mean crates using rust-rocksdb will be forced to use jemalloc on Linux? (But not on Windows since Rust uses HeapAlloc instead?)

[cvijdea-bd]

Note that rust-rocksdb enables the unprefixed_malloc_on_supported_platforms feature of tikv-jemalloc-sys, hooking the actual malloc and free, so jemalloc is used to allocate memory, event if RocksDB is not properly told about it.

Rust uses malloc on Linux by default. Does that mean crates using rust-rocksdb will be forced to use jemalloc on Linux? (But not on Windows since Rust uses HeapAlloc instead?)

If using rust-rocksdb with the jemalloc feature enabled, I think yes.