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

*/Cargo.toml: Quaratine jemalloc with features #5299

Merged
merged 15 commits into from Sep 5, 2019

Conversation

@Hoverbear
Copy link
Member

Hoverbear commented Aug 19, 2019

Signed-off-by: Ana Hobden operator@hoverbear.org

What have you changed? (mandatory)

While I don't have any particular problem with jemalloc, I'd prefer it only be in the build graph when we want it to!

Currently, when you build TiKV with --no-default-features --features "sse portable mimalloc" (or tcmalloc) our friend jemalloc also comes into the build tree.

You can verify this with:

cargo build --no-default-features --features "portable mimalloc sse" --build-plan -Z unstable-options | grep jemalloc

You'll see jemalloc is part of the build plan regardless of feature.

This is problematic for targets like musl since jemalloc is not compatible... But having two allocators is generally a bad idea anyways.

This patch makes jemalloc only part of the build graph if it is enabled.

You can test by running:

cargo build --no-default-features --features "portable mimalloc sse" --build-plan -Z unstable-options | grep jemalloc

In this tree, and comparing it to the one on master. In this tree, the result is empty. In master, there is a result.

What are the type of changes? (mandatory)

Bugfix, we already implemented this feature in other PRs, this one just fixes a couple missed flags.

How has this PR been tested? (mandatory)

cargo build --no-default-features --features "portable mimalloc sse" --build-plan -Z unstable-options | grep jemalloc

Does this PR affect documentation (docs) or release note? (mandatory)

No, it was found by following the documentation. :)

Does this PR affect tidb-ansible update? (mandatory)

Nope

Refer to a related PR or issue link (optional)

Benchmark result if necessary (optional)

Add a few positive/negative examples (optional)

Signed-off-by: Ana Hobden <operator@hoverbear.org>
@Hoverbear Hoverbear added the T: Bug label Aug 19, 2019
@Hoverbear Hoverbear requested review from brson and sticnarf Aug 19, 2019
@Hoverbear Hoverbear self-assigned this Aug 19, 2019
@brson
brson approved these changes Aug 19, 2019
Copy link
Contributor

brson left a comment

LGTM though in the future i'd prefer the unrelated changes to tikv_util be put elsewhere.

Copy link
Contributor

brson left a comment

Actually, the changes to tikv_util don't pass CI. Please put those in a different PR.

Copy link
Contributor

nrc left a comment

I think it is better for engine not to have default features and to always have the allocator passed in explicitly. IMO, default features for non-top level crates is a bit of a footgun.

Copy link
Contributor

nrc left a comment

I think it is better for engine not to have default features and to always have the allocator passed in explicitly. IMO, default features for non-top level crates is a bit of a footgun.

@brson

This comment has been minimized.

Copy link
Contributor

brson commented Aug 20, 2019

Actually I don't see that either engine nor rust-rocksdb has default features, so I'm unsure what affect this PR is having.

@Hoverbear

This comment has been minimized.

Copy link
Member Author

Hoverbear commented Aug 20, 2019

Whoops, those tikv_util changes are stray...

@breeswish

This comment has been minimized.

Copy link
Member

breeswish commented Aug 20, 2019

Hmmm. So why not simply remove these default features of the malloc?

@siddontang

This comment has been minimized.

Copy link
Contributor

siddontang commented Aug 20, 2019

As @breeswish said above, can we remove the default malloc feature? so we must pass allocator we want to use explicitly.

/cc @BusyJay

@Hoverbear

This comment has been minimized.

Copy link
Member Author

Hoverbear commented Aug 20, 2019

@siddontang I think it is a big usability feature to support cargo build (run, fmt, test, bench....) without any extra options so I think it's important to keep some default in TiKV.

Perhaps it's better we adopt a strategy of not having default features on subcrates? So only TiKV has default features, and the subcrates must have features manually opted into?

@siddontang

This comment has been minimized.

Copy link
Contributor

siddontang commented Aug 21, 2019

So only TiKV has default features, and the subcrates must have features manually opted into

I think it is workable :-)

/cc @BusyJay

@Hoverbear

This comment has been minimized.

Copy link
Member Author

Hoverbear commented Aug 21, 2019

OK, I'll work on this

Hoverbear added 2 commits Aug 21, 2019
Signed-off-by: Ana Hobden <operator@hoverbear.org>
@Hoverbear Hoverbear force-pushed the Hoverbear:jemalloc-quarantine branch from 999137e to 857203a Aug 21, 2019
Signed-off-by: Ana Hobden <operator@hoverbear.org>
@Hoverbear

This comment has been minimized.

Copy link
Member Author

Hoverbear commented Aug 21, 2019

Should be good to go, only crates with default features are cmd and tikv.

@Hoverbear Hoverbear requested review from brson, nrc and breeswish and removed request for sticnarf Aug 21, 2019
Copy link
Contributor

nrc left a comment

lgtm otherwise

@@ -49,7 +49,7 @@ hex = "0.3"
vlog = "0.1.4"
kvproto = { git = "https://github.com/pingcap/kvproto.git" }
tikv_util = { path = "../components/tikv_util" }
engine = { path = "../components/engine" }
engine = { path = "../components/engine", default-features = false }

This comment has been minimized.

Copy link
@nrc

nrc Aug 21, 2019

Contributor

These changes to the dep on engine are not needed

This comment has been minimized.

Copy link
@brson

brson Aug 21, 2019

Contributor

Likewise on tikv's engine dep - engine dosen't have any default features.

@@ -35,6 +35,7 @@ features = ["nightly"]
[dependencies.engine_rocksdb]
git = "https://github.com/pingcap/rust-rocksdb.git"
package = "rocksdb"
default-features = false

This comment has been minimized.

Copy link
@brson

brson Aug 21, 2019

Contributor

rut-rocksdb doesn't have any default features, at least on master.

Copy link
Contributor

brson left a comment

Per comments, I don't see default features on either the engine or rust-rocksdb crates, so default-features=false seems unnecessary.

@siddontang

This comment has been minimized.

Copy link
Contributor

siddontang commented Aug 22, 2019

Per comments, I don't see default features on either the engine or rust-rocksdb crates, so default-features=false seems unnecessary.

@brson maybe we can avoid mistakes if the crates introduce default feature later?

Hoverbear added 2 commits Aug 22, 2019
Signed-off-by: Ana Hobden <operator@hoverbear.org>
@Hoverbear Hoverbear requested review from brson and nrc Aug 22, 2019
@siddontang

This comment has been minimized.

Copy link
Contributor

siddontang commented Aug 23, 2019

so is there a way to check that the crate has a default feature but we forget to forbid it?

cmd/Cargo.toml Outdated Show resolved Hide resolved
Copy link
Contributor

nrc left a comment

lgtm

Hoverbear added 2 commits Aug 26, 2019
Signed-off-by: Ana Hobden <operator@hoverbear.org>
…jemalloc-quarantine
@Hoverbear Hoverbear requested review from nrc and siddontang Aug 26, 2019
Cargo.toml Outdated Show resolved Hide resolved
Copy link
Contributor

siddontang left a comment

Rest LGTM

PTAL @brson @BusyJay

Copy link
Contributor

brson left a comment

It still looks to me like engine does not have any default features. Can you explain?

Cargo.toml Outdated Show resolved Hide resolved
@brson

This comment has been minimized.

Copy link
Contributor

brson commented Sep 3, 2019

Hoverbear added 2 commits Sep 4, 2019
Signed-off-by: Ana Hobden <operator@hoverbear.org>
…jemalloc-quarantine
@Hoverbear Hoverbear requested a review from brson Sep 4, 2019
@nrc
nrc approved these changes Sep 5, 2019
Copy link
Contributor

nrc left a comment

lgtm

@brson
brson approved these changes Sep 5, 2019
@brson brson merged commit ab603f1 into tikv:master Sep 5, 2019
4 checks passed
4 checks passed
DCO All commits are signed off!
Details
idc-jenkins-ci-tikv/integration-common-test Jenkins job succeeded.
Details
idc-jenkins-ci-tikv/integration-cop-push-down-test Jenkins job succeeded.
Details
idc-jenkins-ci/test Jenkins job succeeded.
Details
sticnarf pushed a commit to sticnarf/tikv that referenced this pull request Oct 27, 2019
* */Cargo.toml: Quaratine jemalloc with features

Signed-off-by: Ana Hobden <operator@hoverbear.org>

* Trim mistaken inclusion

Signed-off-by: Ana Hobden <operator@hoverbear.org>

* Clean default features

Signed-off-by: Ana Hobden <operator@hoverbear.org>

* Remove unnecessary no-default-features

Signed-off-by: Ana Hobden <operator@hoverbear.org>

* Clean unnecessary engine feature

Signed-off-by: Ana Hobden <operator@hoverbear.org>

* Actually remove engine default feature.

Signed-off-by: Ana Hobden <operator@hoverbear.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.