Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

Use RUSTFLAGS to set the optimization level #10719

Merged
merged 10 commits into from
Jun 7, 2019

Conversation

dvdplm
Copy link
Collaborator

@dvdplm dvdplm commented Jun 5, 2019

Cargo has a quirk in how configuration settings are propagated when cargo test runs: local code respect the settings in [profile.test] but all dependencies use the [profile.dev] settings. Here we force opt-level=3 for all dependencies.

Cargo has a [quirk]() in how configuration settings are propagated when `cargo test` runs: local code respect the settings in `[profile.test]` but all dependencies use the `[profile.dev]` settings. Here we force `opt-level=3` for all dependencies.
@dvdplm dvdplm requested a review from TriplEight June 5, 2019 08:17
@dvdplm dvdplm self-assigned this Jun 5, 2019
@dvdplm dvdplm added A0-pleasereview 🤓 Pull request needs code review. A2-insubstantial 👶 Pull request requires no code review (e.g., a sub-repository hash update). labels Jun 5, 2019
@dvdplm dvdplm added A1-onice 🌨 Pull request is reviewed well, but should not yet be merged. and removed A0-pleasereview 🤓 Pull request needs code review. labels Jun 5, 2019
Copy link
Collaborator

@TriplEight TriplEight left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will deal with it.

Cargo.toml Outdated
[profile.test]
lto = false
opt-level = 3 # makes tests slower to compile, but faster to run

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might work: RUSTFLAGS="-C opt-level=3 -C overflow-checks=off -C incremental=false -C debuginfo=2" time cargo test -v

These are the settings for release mode I think:

The release profile, used for cargo build --release (and the dependencies

for cargo test --release, including the local library or binary).

[profile.release]
opt-level = 3
debug = false
rpath = false
lto = false
debug-assertions = false
codegen-units = 16
panic = 'unwind'
incremental = false
overflow-checks = false

@TriplEight
Copy link
Collaborator

dvdplm
So cargo test --release will use the settings from [profile.release] which with androniks change includes lto = true, which in turn makes compilation take a really long time (but binary size is smaller). So for tests, where we do care about fast run speeds, we want the code optimizations from release mode but not lto = true.

The first idea was to stop passing --release to cargo test and instead just add opt-level = 3 to [profile.test]. That didn't work because cargo applies opt-level =3 only to the local code but not to dependencies. For dependencies it uses [profile.dev] (which is kind of crazy and probably a bug). To get around that we tried passing in the optimization level with RUST_FLAGS but as Kiril points out, we probably need more flags than just -C opt-level = 3.

@dvdplm
Copy link
Collaborator Author

dvdplm commented Jun 5, 2019

@TriplEight pushed #d654d255f to see what happens.

@TriplEight
Copy link
Collaborator

I think it's still "not enough" optimized.
Since you are passing opt-level=3 to everything including deps, it must be faster, but it's still not.
There have to be something missing.

Copy link
Collaborator

@TriplEight TriplEight left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved. Looks good, but I feel there is some space for improve.

* master:
  SecretStore: non-blocking wait of session completion (#10303)
  removed secret_store folder (#10722)
  SecretStore: expose restore_key_public in HTTP API (#10241)
  Revert "enable lto for release builds (#10717)" (#10721)
@dvdplm dvdplm added A0-pleasereview 🤓 Pull request needs code review. and removed A1-onice 🌨 Pull request is reviewed well, but should not yet be merged. A2-insubstantial 👶 Pull request requires no code review (e.g., a sub-repository hash update). labels Jun 6, 2019
@dvdplm dvdplm requested a review from ordian June 6, 2019 15:54
scripts/gitlab/test-linux.sh Outdated Show resolved Hide resolved
.gitlab-ci.yml Outdated Show resolved Hide resolved
@ordian ordian added this to the 2.6 milestone Jun 7, 2019
@ordian ordian added the A8-looksgood 🦄 Pull request is reviewed well. label Jun 7, 2019
@ordian ordian added B0-patch-stable 🕷 Pull request should also be back-ported to the stable branch. B1-patch-beta 🕷🕷 M0-build 🏗 Building and build system. and removed A0-pleasereview 🤓 Pull request needs code review. labels Jun 7, 2019
@dvdplm dvdplm merged commit 7827cc0 into master Jun 7, 2019
@dvdplm dvdplm deleted the dp/fix/force-tests-to-run-opt-level3 branch June 7, 2019 09:25
This was referenced Jun 11, 2019
soc1c pushed a commit that referenced this pull request Jun 11, 2019
* Use RUSTFLAGS to set the optimization level

Cargo has a [quirk]() in how configuration settings are propagated when `cargo test` runs: local code respect the settings in `[profile.test]` but all dependencies use the `[profile.dev]` settings. Here we force `opt-level=3` for all dependencies.

* Remove unused profile settings

* Maybe like this?

* Turn off incremental compilation

* Remove colors; try again with overflow-checks on

* Use quiet CI machine

* Turn overflow checking back on

* Be explicit about what options we use

* Remove "quiet machine" override
soc1c pushed a commit that referenced this pull request Jun 11, 2019
* Use RUSTFLAGS to set the optimization level

Cargo has a [quirk]() in how configuration settings are propagated when `cargo test` runs: local code respect the settings in `[profile.test]` but all dependencies use the `[profile.dev]` settings. Here we force `opt-level=3` for all dependencies.

* Remove unused profile settings

* Maybe like this?

* Turn off incremental compilation

* Remove colors; try again with overflow-checks on

* Use quiet CI machine

* Turn overflow checking back on

* Be explicit about what options we use

* Remove "quiet machine" override
soc1c added a commit that referenced this pull request Jun 11, 2019
* version: bump stable to 2.4.7

* [CI] allow cargo audit to fail (#10676)

* [CI] allow cargo audit to fail

* [.gitlab-ci.yml] add a comment about cargo audit

* [Cargo.lock] cargo update -p protobuf

* new image (#10673)

* Update publishing (#10644)

* docker images are now built on k8s: test run

* copy check_sync.sh in build-linux job

* copy scripts/docker/hub/* in build-linux job

* removed cache var

* cleanup, no more nightly dockers

* cleanup in dockerfile

* some new tags

* removed sccsche debug log, cleanup

* no_gits, new artifacts dir, changed scripts. Test run.

* define version once

* one source for TRACK

* stop kovan onchain updates

* moved changes for two images to a new branch

* rename Dockerfile

* no need in libudev-dev

* enable lto for release builds (#10717)

* Use RUSTFLAGS to set the optimization level (#10719)

* Use RUSTFLAGS to set the optimization level

Cargo has a [quirk]() in how configuration settings are propagated when `cargo test` runs: local code respect the settings in `[profile.test]` but all dependencies use the `[profile.dev]` settings. Here we force `opt-level=3` for all dependencies.

* Remove unused profile settings

* Maybe like this?

* Turn off incremental compilation

* Remove colors; try again with overflow-checks on

* Use quiet CI machine

* Turn overflow checking back on

* Be explicit about what options we use

* Remove "quiet machine" override

* ethcore: enable ECIP-1054 for classic (#10731)

* config: enable atlantis on ethereum classic

* config: enable atlantis on morden classic

* config: enable atlantis on morden classic

* config: enable atlantis on kotti classic

* ethcore: move kotti fork block to 0xAEF49

* ethcore: move morden fork block to 0x4829BA

* ethcore: move classic fork block to 0x81B320

* remove trailing comma

* remove trailing comma

* fix chainspec

* ethcore: move classic fork block to 0x7fffffffffffffff
soc1c added a commit that referenced this pull request Jun 11, 2019
* version: bump beta to 2.5.2

* [CI] allow cargo audit to fail (#10676)

* [CI] allow cargo audit to fail

* [.gitlab-ci.yml] add a comment about cargo audit

* [Cargo.lock] cargo update -p protobuf

* Reset blockchain properly (#10669)

* delete BlockDetails from COL_EXTRA

* better proofs

* added tests

* PR suggestions

* new image (#10673)

* Update publishing (#10644)

* docker images are now built on k8s: test run

* copy check_sync.sh in build-linux job

* copy scripts/docker/hub/* in build-linux job

* removed cache var

* cleanup, no more nightly dockers

* cleanup in dockerfile

* some new tags

* removed sccsche debug log, cleanup

* no_gits, new artifacts dir, changed scripts. Test run.

* define version once

* one source for TRACK

* stop kovan onchain updates

* moved changes for two images to a new branch

* rename Dockerfile

* no need in libudev-dev

* enable lto for release builds (#10717)

* Use RUSTFLAGS to set the optimization level (#10719)

* Use RUSTFLAGS to set the optimization level

Cargo has a [quirk]() in how configuration settings are propagated when `cargo test` runs: local code respect the settings in `[profile.test]` but all dependencies use the `[profile.dev]` settings. Here we force `opt-level=3` for all dependencies.

* Remove unused profile settings

* Maybe like this?

* Turn off incremental compilation

* Remove colors; try again with overflow-checks on

* Use quiet CI machine

* Turn overflow checking back on

* Be explicit about what options we use

* Remove "quiet machine" override

* ethcore: enable ECIP-1054 for classic (#10731)

* config: enable atlantis on ethereum classic

* config: enable atlantis on morden classic

* config: enable atlantis on morden classic

* config: enable atlantis on kotti classic

* ethcore: move kotti fork block to 0xAEF49

* ethcore: move morden fork block to 0x4829BA

* ethcore: move classic fork block to 0x81B320

* remove trailing comma

* remove trailing comma

* fix chainspec

* ethcore: move classic fork block to 0x7fffffffffffffff
dvdplm added a commit that referenced this pull request Jun 17, 2019
* master:
  updater: fix static id hashes initialization (#10755)
  Use fewer threads for snapshotting (#10752)
  Die error_chain, die (#10747)
  Fix deprectation warnings on nightly (#10746)
  fix docker tags for publishing (#10741)
  DevP2p: Get node IP address and udp port from Socket, if not included in PING packet (#10705)
  ethcore: enable ECIP-1054 for classic (#10731)
  Stop breaking out of loop if a non-canonical hash is found (#10729)
  Refactor Clique stepping (#10691)
  Use RUSTFLAGS to set the optimization level (#10719)
  SecretStore: non-blocking wait of session completion (#10303)
  removed secret_store folder (#10722)
  SecretStore: expose restore_key_public in HTTP API (#10241)
  Revert "enable lto for release builds (#10717)" (#10721)
  enable lto for release builds (#10717)
  Merge `Notifier` and `TransactionsPoolNotifier` (#10591)
dvdplm added a commit that referenced this pull request Jun 18, 2019
* master:
  Enable aesni (#10756)
  remove support of old SS db formats (#10757)
  [devp2p] Don't use `rust-crypto` (#10714)
  updater: fix static id hashes initialization (#10755)
  Use fewer threads for snapshotting (#10752)
  Die error_chain, die (#10747)
  Fix deprectation warnings on nightly (#10746)
  fix docker tags for publishing (#10741)
  DevP2p: Get node IP address and udp port from Socket, if not included in PING packet (#10705)
  ethcore: enable ECIP-1054 for classic (#10731)
  Stop breaking out of loop if a non-canonical hash is found (#10729)
  Refactor Clique stepping (#10691)
  Use RUSTFLAGS to set the optimization level (#10719)
  SecretStore: non-blocking wait of session completion (#10303)
  removed secret_store folder (#10722)
  SecretStore: expose restore_key_public in HTTP API (#10241)
  Revert "enable lto for release builds (#10717)" (#10721)
  enable lto for release builds (#10717)
  Merge `Notifier` and `TransactionsPoolNotifier` (#10591)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A8-looksgood 🦄 Pull request is reviewed well. B0-patch-stable 🕷 Pull request should also be back-ported to the stable branch. M0-build 🏗 Building and build system.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants