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

Split release/dist_release makefile rules in two, reorganize and document makefiles #4996

Merged
merged 16 commits into from Jul 4, 2019

Conversation

@brson
Copy link
Contributor

commented Jun 29, 2019

What have you changed? (mandatory)

The main thing this patch does is to modify the default cargo release profile to use thinLTO instead of full LTO, and to turn debuginfo off. This reduces full build time by 25%, and partial build time by 30%.

It makes the various release and dist_release rules independent of each other. Now the dist_release rules rely on the scripts/run-cargo.sh script to put a special .cargo/config in place that contains the "real" release profile. This script requires the use of the unstable -Zcargo-profile flag.

One thing to note about this arrangement is that if any of the dist_release commands are interrupted they will leave .cargo/config in place. Subsequent cargo commands print a warning about cargo profiles being unstable, and effectively ignores the config file. I expect this to not be encountered by most contributors as they are not encouraged to run the dist_release targets by hand.

In the process it deletes all of the experimental x-foo makefile rules, leaving only one in place for dist.

As part of this work I restructured and documented the entire makefile. All pre-existing rules remain, they just might be in a different order. The build_release rule, which does the dist release without the extra testing and file moving, is renamed to build_dist_release for consistency, and build_release left in with a note to use build_dist_release.

I have confirmed @BusyJay that even with debuginfo off perf reports symbol names in profiles. Whether this is sufficient for practical profiling I don't know. We'll find out.

Despite weakening the LTO, I expect the runtime performance to still be excellent, and reasonable to use for benchmarking, as long as the comparison is done between two thinLTO builds. That means one should avoid comparing release builds to the actual released binaries; instead always make two release builds to compare, and only as a final sanity check, do dist_release builds.

Locally:

full before
    Finished release [optimized + debuginfo] target(s) in 29m 08s
partial before
    Finished release [optimized + debuginfo] target(s) in 18m 18s
full after
    Finished release [optimized] target(s) in 21m 51s
partial after
    Finished release [optimized] target(s) in 12m 41s

Fixes #4674

What are the type of the changes? (mandatory)

  • Improvement (change which is an improvement to an existing feature)

How has this PR been tested? (mandatory)

Scripted timing tests and months of experiments.

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

No.

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

No.

Refer to a related PR or issue link (optional)

#4674

Benchmark result if necessary (optional)

See above.

Add a few positive/negative examples (optional)

brson added some commits Jun 28, 2019

make: make dist_ rules call dist_rules
Signed-off-by: Brian Anderson <andersrb@gmail.com>
make: rename build_release to build_dist_release
Re-introduce build_release.

Signed-off-by: Brian Anderson <andersrb@gmail.com>
make: rename 'prod' configs to 'dist'
Signed-off-by: Brian Anderson <andersrb@gmail.com>
make: have the dist_ rules use custom .cargo/config
Signed-off-by: Brian Anderson <andersrb@gmail.com>
make: deoptimize the release profile
Signed-off-by: Brian Anderson <andersrb@gmail.com>
make: depotimize the release rules
Signed-off-by: Brian Anderson <andersrb@gmail.com>
make: remove all etc/cargo.config and x- rules but 'dist'
Signed-off-by: Brian Anderson <andersrb@gmail.com>
make: temporarily make the 'build_release' rule an error
It should have been called build_dist_release

Signed-off-by: Brian Anderson <andersrb@gmail.com>
make: move clippy rules near other static analysis rules
Signed-off-by: Brian Anderson <andersrb@gmail.com>
make: reorganize and document makefile
Signed-off-by: Brian Anderson <andersrb@gmail.com>

@brson brson requested review from AndreMouche, BusyJay and zhouqiang-cl Jun 29, 2019

@siddontang

This comment has been minimized.

Copy link
Contributor

commented Jun 29, 2019

Great @brson

Have we already fixed the problem that removing debuginfo from RocksDB?

Show resolved Hide resolved Makefile Outdated
Show resolved Hide resolved Makefile Outdated

lonng and others added some commits Jun 29, 2019

make: Fix order of targets
Signed-off-by: Brian Anderson <andersrb@gmail.com>
make: Remove dist_prof_release and dist_fail_release
Signed-off-by: Brian Anderson <andersrb@gmail.com>
make: Remove build_release target
Signed-off-by: Brian Anderson <andersrb@gmail.com>
@brson

This comment has been minimized.

Copy link
Contributor Author

commented Jul 1, 2019

I pushed commits to address feedback @siddontang .

Have we already fixed the problem that removing debuginfo from RocksDB?

No. There are two problems related to debuginfo in RocksDB. They aren't fixed, but they are also not related specifically to the release profile:

  • #4711 - In dev mode, RocksDB always has debuginfo on. This is relatively easy to fix by hacking the rocksdb build script, and harder to fix properly in cmake-rs. I intend to fix this soon.

  • #4894 - There isn't a single way to turn debuginfo back on for both Rust and C/C++. Right now this is addressed by educating developers to use RUSTFLAGS + CXXCFLAGS in the README. I don't know a good technical fix for this.

@Connor1996 Connor1996 added the C: Build label Jul 2, 2019

@mkdir -p ${BIN_PATH}
@cp -f ${CARGO_TARGET_DIR}/release/tikv-ctl ${CARGO_TARGET_DIR}/release/tikv-server ${CARGO_TARGET_DIR}/release/tikv-importer ${BIN_PATH}/
bash scripts/check-sse4_2.sh

# Build with release flag as if it were for distribution, but without
# additional sanity checks and file movement.
build_dist_release:

This comment has been minimized.

Copy link
@siddontang

siddontang Jul 2, 2019

Contributor

@iamxy

please make sure again that now we use dist_release to build TiKV in CI and publish this binary.

This comment has been minimized.

Copy link
@iamxy

iamxy Jul 4, 2019

Collaborator

Yep, it has been confirmed.

@siddontang

This comment has been minimized.

Copy link
Contributor

commented Jul 2, 2019

I am with this change

PTAL @nrc @breeswish

Show resolved Hide resolved Makefile Outdated
@Hoverbear

This comment has been minimized.

Copy link
Member

commented Jul 2, 2019

This looks great, @brson , thanks for organizing this confusing file.

make: remove unused trace_test
Signed-off-by: Brian Anderson <andersrb@gmail.com>
@nrc

nrc approved these changes Jul 3, 2019

Copy link
Contributor

left a comment

lgtm (using a script to copy the profiles is pretty awful, but I can't think of a better way to do it)

@iamxy

iamxy approved these changes Jul 4, 2019

@siddontang siddontang merged commit eb9f27a into tikv:master Jul 4, 2019

2 checks passed

DCO All commits are signed off!
Details
idc-jenkins-ci/test Jenkins job succeeded.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.