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

Possibility of new fns const? #461

Closed
rakshith-ravi opened this issue Sep 11, 2023 · 17 comments
Closed

Possibility of new fns const? #461

rakshith-ravi opened this issue Sep 11, 2023 · 17 comments

Comments

@rakshith-ravi
Copy link

Is it possible to make the Argon2::new and new_with_secret functions const? That way, we can have a constant / global static reference to the hasher and use that throughout our code.

Is this something that's possible? I don't see anything that the new function is doing that can't be made const. It pretty much seems to be just assigning fields of the struct, and that's it.

Would be happy to put a PR if required

@rakshith-ravi
Copy link
Author

Follow up:

Looks like avx2_cpuid::init() is not const. I'm gonna investigate further and see if it can be made const though

@rakshith-ravi
Copy link
Author

Okay, I have found a way to do this:

I've basically removed all references to cpu_feat_avx2, and replaced the self.cpu_feat_avx2.get() call with avx2_cpuid::get(). There is a difference in code path this way - Instead of the avx2_cpuid::InitToken getting initialized when Argon2::new() is called, it now gets initialized when the actual hashing is done. I'm not sure if this exposes us to any sort of timing attacks, but I don't think it should, since subsequent calls to hash are not affected, will all have a constant AtomicU8 lookup time added to it uniformly.

cargo build, cargo test, cargo build --release and cargo test --release all works.

Please let me know if you're open to a PR with these changes. Looking forward to hearing from you

@tarcieri
Copy link
Member

It will cause a performance regression because it will call CPUID every time instead of caching the result.

Try running the benchmarks on an AVX2-capable system.

@tarcieri
Copy link
Member

Also: is there some reason you need Argon2 itself to be const that the const constructors of Params don't suffice for?

@rakshith-ravi
Copy link
Author

Try running the benchmarks on an AVX2-capable system.

How do I do that?

Also: is there some reason you need Argon2 itself to be const that the const constructors of Params don't suffice for?

I feel very daft now. What's Params again? 😅

@rakshith-ravi
Copy link
Author

Ohhh you mean argon2::Params. Yeah that works too, but I was just wondering if we can make Argon2 itself const, to see if we can get any perf optimization out of it. Worth a shot I guess?

@tarcieri
Copy link
Member

I don't think there's any way to make it const without sacrificing performance, since it robs us of our ability to precache information about what CPU features are available

@tarcieri
Copy link
Member

Let me look at this when I'm not on an M2 Mac and see if I can figure out what the performance hit would be.

@rakshith-ravi
Copy link
Author

The avx2_cpuid::get() call only seems to be doing a simple check with an AtomicU8. It's a one-time init and every subsequent call to get would only fetch the same initialized value from a static reference. Would that impact performance that much?

Now that I think about it, I suppose it could impact cache hit ratios? Idk, I wouldn't want to make too many assumptions without benchmarking.

Speaking of which, my PC supports AVX2. Would be happy to run benchmarks and get back to you. Just not sure how to though.

@tarcieri
Copy link
Member

Yeah, it's probably fine, though I do worry about painting ourselves into a corner around other potential future optimizations.

@rakshith-ravi
Copy link
Author

Hmm, yes. That is a fair point. const functions would significantly limit how much we can do within the function, until rust adds support for it, and I'm not sure that is something we should depend on.

Well, I'm cool with it either way. If making new const and having a singleton either static or a constant improves performance, I'd be happy to put the PR. If not, we can leave it as it is as well

@newpavlov
Copy link
Member

Ideally, we need something like const_eval_select. For now, I think a better solution would be too keep code as-is and rely on const params.

@rakshith-ravi
Copy link
Author

Is this something that can help us?

rust-lang/rust#116114

@tarcieri
Copy link
Member

While that looks very helpful for other things, it doesn't help here

@rakshith-ravi
Copy link
Author

Sorry if I don't understand this correctly, but can't we use target_feature = "avx2" to call different functions based on the target feature?

@tarcieri
Copy link
Member

No, #[target_feature = "avx2"] informs the compiler that it can use that target feature, and also inline between functions annotated with the same target_feature(s).

We already use it here: https://github.com/RustCrypto/password-hashes/blob/master/argon2/src/lib.rs#L459

The nice thing about that upstream change is right now any function annotated with target_feature must be unsafe, which leads to some unnecessarily unsafe code in some cases. This will allow those functions to be safe (including the one I linked to), which is helpful for other reasons but not for this particular problem.

@tarcieri
Copy link
Member

Closing as "won't fix"

@tarcieri tarcieri closed this as not planned Won't fix, can't repro, duplicate, stale Jul 12, 2024
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

No branches or pull requests

3 participants