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

*: Support both Prost and rust-protobuf libraries #5379

Open
wants to merge 2 commits into
base: master
from

Conversation

@nrc
Copy link
Contributor

nrc commented Sep 2, 2019

To use Prost, set the PROST env var, e.g.,: PROST=1 make dev. If using Cargo,
use --no-default-features --features prost-codec.

The most notable change is threading the prost-codec/protobuf-codec through the
Cargo.tomls of all crates. In addition, in order to make this work I had to move
integraton tests and benchmarks into their own crate (tests). This is because
Cargo features do not interact perfectly with dev-dependencies.

We're using a Git dep for Prost in order to get some optimisations which are on
master, but not in the latest release. We can change to a crates.io dep when there
is another release.

We must allow the identity_conversion lint because there are some conversions
which are meaningful with rust-protobuf, but no-ops with Prost.

The changes to src/coprocessor/endpoint.rs are because Prost does not permit
setting a custom recursion limit. We only did this for tests previously. We
now use the default recursion limit all the time for both codecs; the test must
be adjusted so that we hit the higher limit.

PTAL @breeswish @BusyJay @overvenus

What have you changed?

Add a feature flag for Prost which builds TiKV and its deps using Prost rather than rust-protobuf as the protobuf codec.

What is the type of the changes?

  • Engineering (engineering change which doesn't change any feature or fix any issue)

How is the PR tested?

make dev

Does this PR affect documentation (docs) or should it be mentioned in the release notes?

Should have dev docs (to come) and mentioned in release docs.

Does this PR affect tidb-ansible?

No

Refer to a related PR or issue link (optional)

#2452

@nrc nrc requested review from BusyJay, breeswish and overvenus Sep 2, 2019
@nrc nrc force-pushed the nrc:prost-3 branch from ae5a11c to e46905f Sep 2, 2019
@@ -1,5 +1,8 @@
// Copyright 2019 TiKV Project Authors. Licensed under Apache-2.0.

#[cfg(feature = "prost-codec")]
use prost;
use protobuf::ProtobufError;

This comment has been minimized.

Copy link
@siddontang

siddontang Sep 2, 2019

Contributor

em, do we need to use protobuf feature here?

seem if we pass prost-codec feature, we still need to import protobuf?

This comment has been minimized.

Copy link
@nrc

nrc Sep 2, 2019

Author Contributor

I use the protobuf::Message trait (and supporting error types) to support both Prost and rust-proto, so we still need the protobuf imports in the Prost case.

This comment has been minimized.

Copy link
@siddontang

siddontang Sep 3, 2019

Contributor

got it

failpoints = ["fail/failpoints"]
prost-codec = [

This comment has been minimized.

Copy link
@siddontang

siddontang Sep 3, 2019

Contributor

em, I see nearly all the workspaces use these, seem too verbose, but I also find no way to simply them.

This comment has been minimized.

Copy link
@BusyJay

BusyJay Oct 16, 2019

Contributor

Since features are unions, enabling one should be sufficient for all dependencies.

Cargo.toml Outdated
@@ -96,8 +89,8 @@ zipf = "5.0.1"
bitflags = "1.0.1"
fail = "0.3"
uuid = { version = "0.6", features = [ "serde", "v4" ] }
grpcio = { version = "0.5.0-alpha.3", features = [ "openssl-vendored" ] }
raft = "0.6.0-alpha"
grpcio = { version = "0.5.0-alpha.3", features = [ "openssl-vendored" ], default-features = false }

This comment has been minimized.

Copy link
@siddontang

siddontang Sep 3, 2019

Contributor

why disable default features here and following deps?

This comment has been minimized.

Copy link
@nrc

nrc Sep 3, 2019

Author Contributor

Because when we use grpc-rs etc., we must use either prost-codec or protobuf-codec (which is the default), but not both.

This comment has been minimized.

Copy link
@siddontang

siddontang Sep 4, 2019

Contributor

got it

@siddontang

This comment has been minimized.

Copy link
Contributor

siddontang commented Sep 4, 2019

@zhouqiang-cl

How to bench this PR with building TiKV "PROST=1"?

@siddontang

This comment has been minimized.

Copy link
Contributor

siddontang commented Sep 4, 2019

@mahjonp

This comment has been minimized.

Copy link
Contributor

mahjonp commented Sep 6, 2019

Hi @nrc, can we use PROST=1 make dist_release to release with using Prost? And if yes, it's easy to bench this PR.

@mahjonp

This comment has been minimized.

Copy link
Contributor

mahjonp commented Sep 6, 2019

/release PROST=1 make dist_release

@mahjonp

This comment has been minimized.

Copy link
Contributor

mahjonp commented Sep 6, 2019

/bench

@mahjonp

This comment has been minimized.

Copy link
Contributor

mahjonp commented Sep 6, 2019

@nrc Oops, benchmark failed to start a TiDB cluster using this commit, there were some problem.

@siddontang

This comment has been minimized.

Copy link
Contributor

siddontang commented Sep 8, 2019

friendly ping @nrc

@zhouqiang-cl

This comment has been minimized.

Copy link
Member

zhouqiang-cl commented Sep 8, 2019

/release PROST=1 make dist_release

@nrc nrc added the C: gRPC label Sep 9, 2019
@nrc nrc force-pushed the nrc:prost-3 branch 3 times, most recently from 12239b2 to 7b25341 Sep 9, 2019
@nrc

This comment has been minimized.

Copy link
Contributor Author

nrc commented Sep 10, 2019

/bench

@siddontang

This comment has been minimized.

Copy link
Contributor

siddontang commented Sep 10, 2019

CI failed with

image

PTAL @breeswish

@nrc nrc force-pushed the nrc:prost-3 branch from 7b25341 to b294b71 Sep 10, 2019
@nrc

This comment has been minimized.

Copy link
Contributor Author

nrc commented Sep 10, 2019

/bench

@mahjonp

This comment has been minimized.

Copy link
Contributor

mahjonp commented Sep 10, 2019

/release PROST=1 make dist_release

@mahjonp

This comment has been minimized.

Copy link
Contributor

mahjonp commented Sep 10, 2019

/test

@tikv tikv deleted a comment from zhouqiang-cl Sep 10, 2019
@sre-bot

This comment has been minimized.

Copy link
Collaborator

sre-bot commented Sep 10, 2019

@@                               Benchmark Diff                               @@
================================================================================
tidb: 0f2434660c509ec0b493d8c7ee0bbbf8e8549184
--- tikv: ff82aa9eba331585aec1c6cdf9e1584512bccb34
+++ tikv: b294b71f9a4afd4ab1c47c2c452048d511954224
pd: c7c572a7dc9710aca57cb0bddf4d9bca6c4a111b
================================================================================
test-1: < oltp_insert >
    * QPS : 20840.65 ± 0.5553% (std=70.69) delta: -1.91%
    * AvgMs : 12.28 ± 0.5376% (std=0.04) delta: 1.93%
    * PercentileMs99 : 42.92 ± 1.0903% (std=0.38) delta: 0.73%
            
test-2: < oltp_update_non_index >
    * QPS : 29243.96 ± 0.1096% (std=20.95) delta: -0.69%
    * AvgMs : 8.75 ± 0.1143% (std=0.01) delta: 0.71%
    * PercentileMs99 : 30.70 ± 2.1758% (std=0.41) delta: -0.35%
            
test-3: < oltp_read_write >
    * QPS : 36624.11 ± 0.2892% (std=63.44) delta: -0.78%
    * AvgMs : 140.37 ± 0.2821% (std=0.25) delta: 0.79%
    * PercentileMs99 : 262.64 ± 0.0000% (std=0.00) delta: 1.82%
            
test-4: < oltp_point_select >
    * QPS : 73650.82 ± 0.2890% (std=134.00) delta: -0.62%
    * AvgMs : 3.47 ± 1.0387% (std=0.02) delta: 0.35%
    * PercentileMs99 : 7.48 ± 1.0425% (std=0.06) delta: 0.00%
            
test-5: < oltp_update_index >
    * QPS : 16519.31 ± 0.1599% (std=17.75) delta: -2.55%
    * AvgMs : 15.50 ± 0.1613% (std=0.02) delta: 2.62%
    * PercentileMs99 : 49.21 ± 0.0000% (std=0.00) delta: 1.80%
            

https://perf.pingcap.com

@nrc nrc force-pushed the nrc:prost-3 branch 2 times, most recently from 8df9ffb to 1ba056a Oct 30, 2019
@siddontang

This comment has been minimized.

Copy link
Contributor

siddontang commented Nov 3, 2019

Friendly ping @BusyJay @breeswish

@nrc nrc force-pushed the nrc:prost-3 branch from 1ba056a to 5838f88 Nov 3, 2019
Copy link
Member

breeswish left a comment

Putting benches in tests annoys me..

@@ -12,52 +12,42 @@ edition = "2018"
publish = false

[features]
default = ["jemalloc"]

This comment has been minimized.

Copy link
@breeswish

breeswish Nov 4, 2019

Member

Does this mean that using cargo bench ... will no longer contain jemalloc. Instead, you have to use cargo bench --features jemalloc?

components/engine/src/errors.rs Show resolved Hide resolved
@@ -29,9 +29,6 @@ pub struct Endpoint<E: Engine> {
read_pool_normal: FuturePool,
read_pool_low: FuturePool,

/// The recursion limit when parsing Coprocessor Protobuf requests.
recursion_limit: u32,

This comment has been minimized.

Copy link
@breeswish

breeswish Nov 4, 2019

Member

@nrc We can change it via modifying the configuration item "end_point_recursion_limit".

@nrc

This comment has been minimized.

Copy link
Contributor Author

nrc commented Nov 4, 2019

Does this mean that using cargo bench ... will no longer contain jemalloc. Instead, you have to use cargo bench --features jemalloc?

No because all the benches are inside the tests crate, so you must run cargo bench -p tests, but jemalloc will still be on by default

@nrc

This comment has been minimized.

Copy link
Contributor Author

nrc commented Nov 4, 2019

Putting benches in tests annoys me..

I think we can put them outside in the directory as long as they are inside the crate logically, I find that is worse, but I don't mind either way.

@@ -4,6 +4,9 @@ version = "0.0.1"
publish = false
edition = "2018"

[features]
prost-codec = ["tikv_util/prost-codec"]

This comment has been minimized.

Copy link
@BusyJay

BusyJay Nov 4, 2019

Contributor

Why protobuf-codec not necessary anymore?

This comment has been minimized.

Copy link
@nrc

nrc Nov 5, 2019

Author Contributor

we only need prost-codec for tikv_util because it optionally depends on prost itself and rust-proto is always included.

But I think fuzz/targets is another root crate and it doesn't depend on tikv? So we need to do something else here?

This comment has been minimized.

Copy link
@nrc

nrc Nov 5, 2019

Author Contributor

This seems to work actually, cargo run --package fuzz -- list-targets compiles and has the expected results.

@@ -39,8 +39,8 @@ safemem = { version = "0.3", default-features = false }
flate2 = { version = "1.0", features = ["zlib"], default-features = false }
slog = { version = "2.3", features = ["max_level_trace", "release_max_level_debug"] }
slog-global = { version = "0.1", git = "https://github.com/breeswish/slog-global.git", rev = "0e23a5baff302a9d7bccd85f8f31e43339c2f2c1" }
tipb = { git = "https://github.com/pingcap/tipb.git" }
kvproto = { git = "https://github.com/pingcap/kvproto.git" }
tipb = { git = "https://github.com/pingcap/tipb.git", default-features = false }

This comment has been minimized.

Copy link
@BusyJay

BusyJay Nov 4, 2019

Contributor

I'm curious why it can compile as no features means no struct at all.

This comment has been minimized.

Copy link
@nrc

nrc Nov 5, 2019

Author Contributor

It is because of your suggestion :-) The features are unioned together and in the top level Cargo.toml I set the Prost or proto features.

This comment has been minimized.

Copy link
@BusyJay

BusyJay Nov 5, 2019

Contributor

Does it mean tidb_query can't be compiled alone?

This comment has been minimized.

Copy link
@nrc

nrc Nov 5, 2019

Author Contributor

No, cargo build -p tidb_query --features prost-codec at the top-level will work. I believe this is because Cargo computes the whole dep graph including features and then only builds what is needed (I am not sure if this is a bug or a feature :-) )

This comment has been minimized.

Copy link
@BusyJay

BusyJay Nov 11, 2019

Contributor

Get it. Seems features are required to get it built now.

Cargo.toml Outdated Show resolved Hide resolved
export DYLD_LIBRARY_PATH="${DYLD_LIBRARY_PATH}:${LOCAL_DIR}/lib" && \
export LOG_LEVEL=DEBUG && \
export RUST_BACKTRACE=1 && \
cargo test --no-default-features --features "${ENABLE_FEATURES}" --all ${EXTRA_CARGO_ARGS} -- --nocapture $(TEST_THREADS) && \
cargo test --no-default-features --features "${ENABLE_FEATURES}" --all --bench misc ${EXTRA_CARGO_ARGS} -- --nocapture $(TEST_THREADS) && \
cargo test --no-default-features --features "${ENABLE_FEATURES}" -p tests --bench misc ${EXTRA_CARGO_ARGS} -- --nocapture $(TEST_THREADS) && \

This comment has been minimized.

Copy link
@BusyJay

BusyJay Nov 5, 2019

Contributor

So benchmarks inside components will be ignored?

This comment has been minimized.

Copy link
@nrc

nrc Nov 5, 2019

Author Contributor

No, this command only tests, it does not bench, and previously it only built and tested the misc bench, which is what it still does.

@BusyJay

This comment has been minimized.

Copy link
Contributor

BusyJay commented Nov 5, 2019

One goal to separate codes into components is to speed up the development by focus on small pieces and compile less code. So it's important to make each component be able to build and test.

@nrc nrc force-pushed the nrc:prost-3 branch 4 times, most recently from 7154376 to 616a489 Nov 5, 2019
@nrc

This comment has been minimized.

Copy link
Contributor Author

nrc commented Nov 7, 2019

@breeswish I've changed the PR to keep the recursion limit when using rust-proto, but not with Prost. PTAL.

@nrc nrc force-pushed the nrc:prost-3 branch from 616a489 to 65bf171 Nov 11, 2019
@nrc

This comment has been minimized.

Copy link
Contributor Author

nrc commented Nov 11, 2019

@breeswish @BusyJay rebased again. PTAL

@nrc nrc force-pushed the nrc:prost-3 branch from 65bf171 to e1e9c14 Nov 11, 2019
tests/benches/misc/coprocessor/codec/chunk/chunk.rs Outdated Show resolved Hide resolved
components/tikv_util/Cargo.toml Outdated Show resolved Hide resolved
components/test_coprocessor/Cargo.toml Outdated Show resolved Hide resolved
protobuf = "2.8"
kvproto = { git = "https://github.com/pingcap/kvproto.git", default-features = false }
raft = { version = "0.6.0-alpha" , default-features = false }
prost = { git = "https://github.com/danburkert/prost", rev = "1944c27c3029d01ff216e7b126d846b6cf8c7d77", optional = true }

This comment has been minimized.

Copy link
@BusyJay

BusyJay Nov 12, 2019

Contributor

Should use release version.

tcmalloc = ["tikv/tcmalloc"]
jemalloc = ["tikv/jemalloc"]
mimalloc = ["tikv/mimalloc"]
portable = ["tikv/portable"]
sse = ["tikv/sse"]
mem-profiling = ["tikv/mem-profiling"]
profiling = ["tikv/profiling"]

This comment has been minimized.

Copy link
@BusyJay

BusyJay Nov 12, 2019

Contributor

Why remove this feature?

This comment has been minimized.

Copy link
@nrc

nrc Nov 12, 2019

Author Contributor

It is moved to tests/Cargo.toml because profiling is only used by the profiler crate which is only used via the tests entry point.

This comment has been minimized.

Copy link
@BusyJay

BusyJay Nov 13, 2019

Contributor

Interesting. I thought it was going to support profiling tikv-server at runtime. But I'm OK to readd this when it's implemented.

@nrc nrc force-pushed the nrc:prost-3 branch 2 times, most recently from 9e4fdc8 to d6126d3 Nov 12, 2019
@nrc

This comment has been minimized.

Copy link
Contributor Author

nrc commented Nov 12, 2019

@BusyJay all comments should be addressed in the last push.

Also rebased.

@breeswish are you happy with the recursion limit solution?

To use Prost, set the `PROST` env var, e.g.,: `PROST=1 make dev`. If using Cargo,
use `--no-default-features --features prost-codec`.

The most notable change is threading the prost-codec/protobuf-codec through the
Cargo.tomls of all crates. In addition, in order to make this work I had to move
integraton tests and benchmarks into their own crate (`tests`). This is because
Cargo features do not interact perfectly with dev-dependencies.

We're using a Git dep for Prost in order to get some optimisations which are on
master, but not in the latest release. We can change to a crates.io dep when there
is another release.

We must allow the `identity_conversion` lint because there are some conversions
which are meaningful with rust-protobuf, but no-ops with Prost.

The changes to src/coprocessor/endpoint.rs are because Prost does not permit
setting a custom recursion limit. We only did this for tests previously. We
now use the default recursion limit all the time for both codecs; the test must
be adjusted so that we hit the higher limit.

Signed-off-by: Nick Cameron <nrc@ncameron.org>
@nrc nrc force-pushed the nrc:prost-3 branch from d6126d3 to f4b9503 Nov 12, 2019
tcmalloc = ["tikv/tcmalloc"]
jemalloc = ["tikv/jemalloc"]
mimalloc = ["tikv/mimalloc"]
portable = ["tikv/portable"]
sse = ["tikv/sse"]
mem-profiling = ["tikv/mem-profiling"]
profiling = ["tikv/profiling"]

This comment has been minimized.

Copy link
@BusyJay

BusyJay Nov 13, 2019

Contributor

Interesting. I thought it was going to support profiling tikv-server at runtime. But I'm OK to readd this when it's implemented.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.