Switch from jemallocator to tikv-jemallocator

[koute]

Closes #586

One thing I'm not entirely sure of is to whether I should bump the version to 0.10.2 or to 0.11.0. It feels like this should not be a breaking change, but if someone was linking to e.g. jemalloc-ctl directly before then the whole thing will stop compiling due to two jemallocs being compiled in at the same time, and I guess the ALLOC symbol is public (but only if an allocator was actually enabled) and its type did change.

[dvdplm]

My gut feeling is this is a breaking change. I really hope someone else disagrees, because of snowballing ripple effects. :/

[ordian]

That's not a breaking change.

[ordian]

and I guess the ALLOC symbol is public (but only if an allocator was actually enabled) and its type did change.

yeah, but no one is using ALLOC anyway. BTW, does it have to be public? cc @cheme

but if someone was linking to e.g. jemalloc-ctl directly

then it's their fault ;)
I would consider this a possible-breaking change according to https://doc.rust-lang.org/cargo/reference/semver.html#semver-compatibility and would lean towards 0.10.2 bump as is done in the PR.

Another thing we could do is split global allocator stuff in a separate crate, that might help with snowballing ripple effects, but that would be a breaking change. Also see #377

[drahnr]

The change itself LGTM.

I am wary of introducing this change given that rocksdb-rust uses jemalloc-sys and this using tikv-jemalloc-sys, introducing two different jemalloc library instances compiled into those two crates. I am not entirely sure how this would interact with the global ALLOC variable at this point, but should be investigated.

[ordian]

given that rocksdb-rust uses jemalloc-sys and this using tikv-jemalloc-sys

rust-rocksdb/rust-rocksdb#554 ;)
BTW, we don't activate jemalloc feature in rust-rocksdb.

[drahnr]

given that rocksdb-rust uses jemalloc-sys and this using tikv-jemalloc-sys

rust-rocksdb/rust-rocksdb#554 ;)
I'd hold this PR off until the above is merged and released.

BTW, we don't activate jemalloc feature in rust-rocksdb.

Good point, this would mandate a node wherever we depend on it in the manifest to check the relevant issue or simply avoid enabling the jemalloc feature of rocksdb.

[drahnr]

Given that the jemalloc in rocksdb is not released yet and jemalloc is not used as part of rocksdb yet, as @ordian pointed out, this is fine to merge as is.

[cheme]

yeah, but no one is using ALLOC anyway. BTW, does it have to be public?

I don't really see any reason for it to be public, except it can be very convenient. Looking at jemalloc doc it there is not much doable with it, and it looks like there is no change in api between both crate (just the rust traits and same usable_size function prototype).
Probably could break some code where user uses jemalloc::usable_size and will switch to tikv_jemalloc::usable_size so I am more in favor of 0.11 even if migration is super simple the user will need to change its cargo.toml.
In fact all user defining jmallocator somewhere will need to update so definitely 0.11
(but I really don't think any project will break if we use 0.10.2)

[koute]

Yeah, in our binaries rocksdb uses the system allocator (which, I guess, could be also another area for improvement as we could enable jemalloc there too, although not sure if it's going to be worth it if we're going to switch to paritydb anyway?).

So in the end can we have some sort of a consensus regarding the version number? Since from what I can see the opinions are divided. Personally I don't really care either way, so I'm fine with whatever you decide.

Also, maybe I should make the ALLOC not pub while I'm at it? If we really want to expose it then it'd be a better idea to have a facade for it so that its type/API is not different depending on the allocator. (But if it's not used anywhere anyway then there's no point it exporting it at all.)

(I have to push force the PR again anyway since I forgot to sign the commits. 😰)

[drahnr]

If you change visibility of ALLOC, then that mandates a 0.12 which obsoletes the discussion.

[koute]

If you change visibility of ALLOC, then that mandates a 0.12 which obsoletes the discussion.

I think you meant 0.11, but yeah - precisely. That's why I proposed it - if there's no consensus for the version number but there's a consensus for removing the pub then that sidesteps the version discussion and we can just move forward. (:

[cheme]

IMO the only point of having it public is so user can still use some api specific to allocator (not even sure there is such api in the wild but I remember finding it convenient while modifying some alloc crate).
But I am fine with removing pub access, it is never a blocker as this crate is very easy to patch, it is probably better design.

Upgrading version really depends on the impact, I did not remember too well but maybe pushing the version is a bit cumbersome due to the fact that the crate define the alloc_size trait that is being used in many place.
So staying at 0.10.2, even if not strictly correct, would not shock me. No strong opinion here.

[ordian]

I would suggest we merge it as is as 0.10.2 and make any further breaking changes in a separate PR(s).

[dvdplm]

I would suggest we merge it as is as 0.10.2 and make any further breaking changes in a separate PR(s).

Fair enough, let's do what andronik suggests.