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

Enable aesni #10756

Merged
merged 27 commits into from
Jun 18, 2019
Merged

Enable aesni #10756

merged 27 commits into from
Jun 18, 2019

Conversation

dvdplm
Copy link
Collaborator

@dvdplm dvdplm commented Jun 17, 2019

Build parity-ethereum with SSE2, SSSE3 and AES enabled for x86_64 targets so that the aesni crate is picked up at compile time.

Builds on #10714

dvdplm added 16 commits June 1, 2019 19:44
Truncate payload properly
…ypto-from-devp2p

* dp/chore/devp2p-cosmetics:
  Reorg imports
  Upgrade ethereum types (#10670)
* master:
  [devp2p] Fix warnings and re-org imports (#10710)
* 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)
This enables the `aesni` crate for 50x faster AES crypto.
@dvdplm dvdplm self-assigned this Jun 17, 2019
@dvdplm dvdplm added the A0-pleasereview 🤓 Pull request needs code review. label Jun 17, 2019
@dvdplm dvdplm requested review from ordian and cheme and removed request for ordian June 17, 2019 15:59
ordian
ordian previously approved these changes Jun 17, 2019
@ordian
Copy link
Collaborator

ordian commented Jun 17, 2019

wait, if we specify both RUSTFLAGS and rustflags in .cargo/config, doesn't the latter get overwritten by the former?

@ordian ordian added this to the 2.6 milestone Jun 17, 2019
@dvdplm
Copy link
Collaborator Author

dvdplm commented Jun 17, 2019

This does not work, something's wrong here.

When I build with RUSTFLAGS I can verify the build with objdump -d path/to/artifact | rg aesimc and I see e.g. 1002108d0: 66 0f 38 db ca aesimc %xmm2, %xmm1 but with rustflags set to the exact same string in ./cargo/config I get this error instead:

   Compiling aes-ctr v0.3.0
   Compiling aes v0.3.2
error[E0463]: can't find crate for `aesni`
  --> /Users/dvd/.cargo/registry/src/github.com-1ecc6299db9ec823/aes-0.3.2/src/lib.rs:63:1
   |
63 | extern crate aesni;
   | ^^^^^^^^^^^^^^^^^^^ can't find crate
error[E0463]: can't find crate for `aesni`

  --> /Users/dvd/.cargo/registry/src/github.com-1ecc6299db9ec823/aes-ctr-0.3.0/src/lib.rs:62:1
   |
62 | extern crate aesni;
   | ^^^^^^^^^^^^^^^^^^^ can't find crate

error: aborting due to previous error

Something's definitely weird. I can reproduce it with plain rustc as well:

$ rustc --crate-name aes /Users/dvd/.cargo/registry/src/github.com-1ecc6299db9ec823/aes-0.3.2/src/lib.rs --color always --crate-type lib --emit=dep-info,metadata,link -C opt-level=3 -C metadata=13d9028e470a76bd -C extra-filename=-13d9028e470a76bd --out-dir /Users/dvd/dev/parity/parity-ethereum/target/release/deps -L dependency=/Users/dvd/dev/parity/parity-ethereum/target/release/deps --extern block_cipher_trait=/Users/dvd/dev/parity/parity-ethereum/target/release/deps/libblock_cipher_trait-cde3daa108f21a6f.rlib --cap-lints allow -C target-feature=+sse2,+aes,+ssse3
error[E0463]: can't find crate for `aesni`
  --> /Users/dvd/.cargo/registry/src/github.com-1ecc6299db9ec823/aes-0.3.2/src/lib.rs:63:1
   |
63 | extern crate aesni;
   | ^^^^^^^^^^^^^^^^^^^ can't find crate

@dvdplm dvdplm added the A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. label Jun 17, 2019
@dvdplm
Copy link
Collaborator Author

dvdplm commented Jun 17, 2019

if we specify both RUSTFLAGS and rustflags in .cargo/config, doesn't the latter get overwritten by the former?

Yes, RUSTFLAGS should take the priority for the values that overlap.

@dvdplm
Copy link
Collaborator Author

dvdplm commented Jun 17, 2019

Oh, this must be rust-lang/cargo#6858

@ordian ordian dismissed their stale review June 17, 2019 19:07

PR is in progress

@dvdplm
Copy link
Collaborator Author

dvdplm commented Jun 18, 2019

I think it overrides all flags, not just the ones that overlap.

You are correct, using RUSTFLAGS in any form clobbers the rustflags setting from .cargo/config. This is a pretty serious foot gun: it's quite possible that some future PR introduce RUSTFLAGS usage somewhere and without knowing it we're not using aesimc anymore.

@dvdplm dvdplm removed the A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. label Jun 18, 2019
@dvdplm dvdplm requested a review from ordian June 18, 2019 06:26
@ordian
Copy link
Collaborator

ordian commented Jun 18, 2019

#10714 is merged, could you rebase on/merge with master and resolve conflicts please?

@ordian
Copy link
Collaborator

ordian commented Jun 18, 2019

You are correct, using RUSTFLAGS in any form clobbers the rustflags setting from .cargo/config.

We still set them in build-linux.sh and in test-linux.sh.

@dvdplm
Copy link
Collaborator Author

dvdplm commented Jun 18, 2019

We still set them in build-linux.sh and in test-linux.sh.

Yeah I know. So what is the best way to go here?

  1. we can set the flags in both .cargo/config and in the release/test scripts – easy to get wrong
  2. we can scrap this PR and not use aesimc until something changes in cargo or rustc that enables us to avoid the clobbering
  3. only use RUSTFLAGS and not enable aesimc for devs, but only for release/test builds (i.e. set it in build-*.sh and test-*.sh and not use .cargo/config at all)

Do we have any other options?

@cheme
Copy link
Contributor

cheme commented Jun 18, 2019

The third option seems to be the more realistic to me (but I can be a bit pessimistic :)

@dvdplm
Copy link
Collaborator Author

dvdplm commented Jun 18, 2019

The third option seems to be the more realistic to me (but I can be a bit pessimistic :)

Hmm. One big downside is that devs will not run the same code as users, which can be a terrible timesink when fixing bugs.

@ordian
Copy link
Collaborator

ordian commented Jun 18, 2019

I would vote for the third option as well.

One big downside is that devs will not run the same code as users, which can be a terrible timesink when fixing bugs.

If we make clear for everyone which flags we use for release builds, devs can set the flags themselves, or use build-linux.sh directly.

@dvdplm
Copy link
Collaborator Author

dvdplm commented Jun 18, 2019

Hmm. I think I'd prefer 1) then: we make a best effort to keep the two in sync and if .cargo/config goes out of sync, then it's not the end of the world because releases are built with RUSTFLAGS.

scripts/gitlab/build-linux.sh Outdated Show resolved Hide resolved
scripts/gitlab/build-windows.sh Show resolved Hide resolved
@dvdplm
Copy link
Collaborator Author

dvdplm commented Jun 18, 2019

@TriplEight is there an easy way to trigger a release build from here so I can double check the resulting binaries to ensure that AES in hardware is activated?

Copy link
Contributor

@cheme cheme left a comment

Choose a reason for hiding this comment

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

LGTM, just a small nit (see comment).

.cargo/config Outdated Show resolved Hide resolved
@dvdplm dvdplm merged commit be5db14 into master Jun 18, 2019
@dvdplm dvdplm deleted the dp/chore/turn-on-aesni branch June 18, 2019 18:24
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)
dvdplm added a commit that referenced this pull request Jun 19, 2019
…-even

* master:
  [devp2p] Update to 2018 edition (#10716)
  Add a way to signal shutdown to snapshotting threads (#10744)
  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)
dvdplm added a commit that referenced this pull request Jun 19, 2019
…p/chore/aura-log-validator-set-in-epoch-manager

* dp/chore/aura-warn-when-validators-is-1-or-even:
  [devp2p] Update to 2018 edition (#10716)
  Add a way to signal shutdown to snapshotting threads (#10744)
  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)
  Update ethcore/src/engines/validator_set/simple_list.rs
  DevP2p: Get node IP address and udp port from Socket, if not included in PING packet (#10705)
  ethcore: enable ECIP-1054 for classic (#10731)
dvdplm added a commit that referenced this pull request Jun 19, 2019
…dp/fix/prevent-building-block-on-top-of-same-parent

* dp/chore/aura-log-validator-set-in-epoch-manager:
  [devp2p] Update to 2018 edition (#10716)
  Add a way to signal shutdown to snapshotting threads (#10744)
  Enable aesni (#10756)
  remove support of old SS db formats (#10757)
  [devp2p] Don't use `rust-crypto` (#10714)
  Update ethcore/src/engines/validator_set/simple_list.rs
de1acr0ix pushed a commit to btccom/parity-ethereum that referenced this pull request Sep 20, 2019
* Run cargo fix

* Optimize imports

* compiles

* cleanup

* Use Secret to store mac-key
Truncate payload properly

* cleanup

* Reorg imports

* brwchk hand waving

* Review feedback

* whitespace

* error chain is dead

* Build parity-ethereum with SSE2, SSSE3 and AES enabled

This enables the `aesni` crate for 50x faster AES crypto.

* Correct rustflag setting

* List all target triples because [target.'cfg(…)'] is broken

* whitespace

* Enable hardware aes for CI builds

* Add note about synchronizing changes

* Remove "Linker" printout

* Build artefacts to check hardware aesni

* Skip signing windows binaries

* Build windows like before

* address grumble
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-pleasereview 🤓 Pull request needs code review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants