Make `cargo build --release` the default for native engine bootstrapping. #4090

Merged
merged 4 commits into from Nov 22, 2016

Conversation

Projects
None yet
4 participants
@kwlzn
Member

kwlzn commented Nov 21, 2016

Problem

Currently, the native engine binary is created with cargo build sans the --release flag, which implicitly generates a non-optimized (read: slow) binary.

Solution

Set the default $MODE to "release" and plumb $MODE_FLAG to result in cargo build --release for the default case.

Result

Benchmarking before and after indicate a >2x performance boost (19m->9m).

@jsirois

Just making sure this is what you really want; ie both local dev and bintray releases are now free of debug symbols. If you want to keep debug symbols for local dev, the change should be lifted into the bintray prep for osx in a similar way that the linux prep is done today (but passing another param in addition to the target triple to indicate do a --release build).

readonly NATIVE_ENGINE_VERSION_RESOURCE="${REPO_ROOT}/src/python/pants/engine/subsystem/native_engine_version"
+# N.B. Set $MODE to "debug" to generate a binary with debugging symbols.

This comment has been minimized.

@jsirois

jsirois Nov 21, 2016

Member

Not quite true since cargo build --debug is not a thing afaict.

@jsirois

jsirois Nov 21, 2016

Member

Not quite true since cargo build --debug is not a thing afaict.

This comment has been minimized.

@kwlzn

kwlzn Nov 21, 2016

Member

ah, poor assumption on my part due to not having first class access to cargo since internalization - fixed!

@kwlzn

kwlzn Nov 21, 2016

Member

ah, poor assumption on my part due to not having first class access to cargo since internalization - fixed!

@kwlzn

This comment has been minimized.

Show comment
Hide comment
@kwlzn

kwlzn Nov 21, 2016

Member

@jsirois the thinking here is that given the speed delta w and w/o --release, it probably makes sense to tradeoff debug functionality for performance in the default dev case (w/ the ability to toggle that on as/if needed) since performance is a key focus. open to other ideas tho.

Member

kwlzn commented Nov 21, 2016

@jsirois the thinking here is that given the speed delta w and w/o --release, it probably makes sense to tradeoff debug functionality for performance in the default dev case (w/ the ability to toggle that on as/if needed) since performance is a key focus. open to other ideas tho.

@jsirois

A sidetrack comment below - its off the main line of your current goal, so as you see fit.

build-support/bin/native/bootstrap.sh
@@ -19,7 +19,10 @@ readonly NATIVE_ENGINE_VERSION_RESOURCE="${REPO_ROOT}/src/python/pants/engine/su
# N.B. Set $MODE to "debug" to generate a binary with debugging symbols.
readonly MODE="release"

This comment has been minimized.

@jsirois

jsirois Nov 21, 2016

Member

These 2 things would be good:

  • readonly MODE="${MODE:-release}" so this can be done via MODE=debug ./pants ....
  • mix the MODE into the hashing so MODE=debug ./pants ... actually re-builds when there has been a prior --release build.
@jsirois

jsirois Nov 21, 2016

Member

These 2 things would be good:

  • readonly MODE="${MODE:-release}" so this can be done via MODE=debug ./pants ....
  • mix the MODE into the hashing so MODE=debug ./pants ... actually re-builds when there has been a prior --release build.

This comment has been minimized.

@kwlzn

kwlzn Nov 21, 2016

Member

sgtm! added both.

@kwlzn

kwlzn Nov 21, 2016

Member

sgtm! added both.

@jsirois

This comment has been minimized.

Show comment
Hide comment
@jsirois

jsirois Nov 21, 2016

Member

@kwlzn re default dev case - ok. That surprises me - since perf in the pantsbuild/pants codebase will be not a concern afaict (small number of targets) - but mainly wanted to check you were making this decision consciously.

Member

jsirois commented Nov 21, 2016

@kwlzn re default dev case - ok. That surprises me - since perf in the pantsbuild/pants codebase will be not a concern afaict (small number of targets) - but mainly wanted to check you were making this decision consciously.

build-support/bin/native/bootstrap.sh
git ls-files -c -o --exclude-standard ${NATIVE_ROOT} | \
- git hash-object -t blob --stdin-paths | fingerprint_data
+ git hash-object -t blob --stdin-paths | \
+ awk -v MODE="$MODE" '{print $1 "\n" MODE}' | fingerprint_data

This comment has been minimized.

@jsirois

jsirois Nov 21, 2016

Member

This needs more care, see here.

The MODE should probably not affect the version resource file that gets checked in, just the binary picked for use in dev mode. This may be too much to deal with for this RB - not coming up with simple ideas here.

@jsirois

jsirois Nov 21, 2016

Member

This needs more care, see here.

The MODE should probably not affect the version resource file that gets checked in, just the binary picked for use in dev mode. This may be too much to deal with for this RB - not coming up with simple ideas here.

This comment has been minimized.

@kwlzn

kwlzn Nov 22, 2016

Member

ah, I see what you mean. will punt on this for now - reverted.

@kwlzn

kwlzn Nov 22, 2016

Member

ah, I see what you mean. will punt on this for now - reverted.

@baroquebobcat

Looks good to me. I'm a little low on context though.

@kwlzn

This comment has been minimized.

Show comment
Hide comment
@kwlzn

kwlzn Nov 21, 2016

Member

@jsirois at least in our case, the default dev mode is to run pants from pants' repo sources directly in our monorepo (and I had assumed others did this as well). so as far as integration testing/iteration is concerned, pants' defaults here intrinsically propagate to our larger scale testing where the perf gap here is much more noticeable.

Member

kwlzn commented Nov 21, 2016

@jsirois at least in our case, the default dev mode is to run pants from pants' repo sources directly in our monorepo (and I had assumed others did this as well). so as far as integration testing/iteration is concerned, pants' defaults here intrinsically propagate to our larger scale testing where the perf gap here is much more noticeable.

@jsirois

This comment has been minimized.

Show comment
Hide comment
@jsirois

jsirois Nov 21, 2016

Member

@kwlzn OK - that's a hidden parameter though - huge repo not present here in pantsbuild, but I'm ok with moving forward. Ideally I'd think the defaults here would make sense for pantsbuild/pants development + release and be easy to adapt for hosting in an external repo like Twitters. Fundamentally hacking on a thing and needing release mode seems odd / not typical, but then again Twitter hackers are the only native engine hackers right now.

Member

jsirois commented Nov 21, 2016

@kwlzn OK - that's a hidden parameter though - huge repo not present here in pantsbuild, but I'm ok with moving forward. Ideally I'd think the defaults here would make sense for pantsbuild/pants development + release and be easy to adapt for hosting in an external repo like Twitters. Fundamentally hacking on a thing and needing release mode seems odd / not typical, but then again Twitter hackers are the only native engine hackers right now.

@kwlzn kwlzn merged commit c10ec03 into pantsbuild:master Nov 22, 2016

1 of 2 checks passed

coverage/coveralls Coverage pending from Coveralls.io
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment