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

aesni: Add target_feature annotations to allow intrinsic inlining #165

Merged
merged 2 commits into from
Oct 15, 2020

Conversation

jack-signal
Copy link
Contributor

@jack-signal jack-signal commented Oct 13, 2020

When built with nocheck feature, the AES-NI intrinsics are not inlined causing a severe performance drop. (rust-lang/rust#53069)

When compiled with --features=nocheck, improvement is notable (all numbers MB/s as reported by cargo bench on an i7-10510U, rustc 1.47)

Benchmark Current master (+aes) Current master (nocheck) This PR (nocheck)
aes128_encrypt 615 285 516
aes128_encrypt8 2782 551 2327
aes192_encrypt 533 242 444
aes192_encrypt8 2327 462 2000
aes256_encrypt 457 210 400
aes256_encrypt8 2000 393 1753

When compiled with RUSTFLAGS="-C target-feature=+aes,+ssse3" there was no performance change (based on cargo bench output)

Decryption could be updated similarly. I didn't need it, but could update that as well if desired.

Otherwise when built with nocheck feature, the AES-NI intrinsics are
not inlined causing a severe performance drop.
@jack-signal jack-signal force-pushed the jack/aesni-target-features branch from 8b2f0ac to a3c8e5b Compare October 13, 2020 20:29
@tarcieri tarcieri requested a review from newpavlov October 14, 2020 13:18
Copy link
Member

@tarcieri tarcieri left a comment

Choose a reason for hiding this comment

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

This looks good to me

Decryption could be updated similarly. I didn't need it, but could update that as well if desired.

That'd be appreciated!

Will wait to hear from @newpavlov for a bit before merging.

@jack-signal
Copy link
Contributor Author

Added decryption in 82970df, improvement looks to be similar.

@tarcieri
Copy link
Member

@jack-signal thanks!

clippy error looks unrelated. I can try to fix that up in a separate PR.

@newpavlov
Copy link
Member

newpavlov commented Oct 15, 2020

Strictly speaking doing so is incorrect and even having the nocheck feature in the first place. The methods would have to be marked as unsafe since we do not check target feature availability neither at compile, nor run time. The feature was added mostly to make doc generation easier.

Do you plan to use it in practice with a runtime detection on top or do you want to simply omit enabling target-features using compiler flags?

@jack-signal
Copy link
Contributor Author

Do you plan to use it in practice with a runtime detection on top

Yes in my code I have an enum

pub enum Aes256 {
    Soft(Box<aes_soft::Aes256>),
    #[cfg(any(target_arch = "x86", target_arch = "x86_64"))]
    AesNi(Box<aesni::Aes256>),
    #[cfg(target_arch = "aarch64")]
    Aarch64(Box<aarch64::Aes256Aarch64>),
}

and then Aes256::new chooses which to use based on runtime checks (is_x86_feature_detected, or getauxval for Android/Linux on Aarch64)

The methods would have to be marked as unsafe since we do not check target feature availability neither at compile, nor run time.

But that's true for nocheck with or without this change, right? I thought the whole point of this feature was to allow for higher levels to choose which implementation to use.

@newpavlov
Copy link
Member

But that's true for nocheck with or without this change, right?

Yes. Unfortunately it is not possible to mark safe trait methods as unsafe in implementation.

I guess we can merge it as a temporary solution until #135 lands.

Copy link
Member

@newpavlov newpavlov left a comment

Choose a reason for hiding this comment

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

Also add similar attributes for the CTR code?

aes/aesni/src/aes128.rs Show resolved Hide resolved
@newpavlov newpavlov merged commit 22339bc into RustCrypto:master Oct 15, 2020
tarcieri added a commit that referenced this pull request Oct 16, 2020
Information about #165 was accidentally omitted
tarcieri added a commit that referenced this pull request Oct 16, 2020
Information about #165 was accidentally omitted
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants