Added ACCP for ECDH for client generate #2276
Conversation
| private static final Logger LOGGER = LoggerFactory.getLogger(CryptoProviderService.class); | ||
|
|
||
| // ECDH provider selection: tries ACCP first, falls back to default (SunEC) | ||
| private static final String ECDH_PROVIDER_NAME = initEcdhProvider(); |
There was a problem hiding this comment.
Just call initEcdhProvider in createKeyAgreement
There was a problem hiding this comment.
This is to ensure the provider is registered once globally. Calling initECDHProvider in createKeyAgreement would try to register it for every thread that tries to access it. While Security.addProvider is idempotent, I didn't want to repeat that operation unnecessarily, especially since if ACCP is not available for some reason, it would throw an exception (which is caught) that would unwind the stack and introduce some overhead.
There was a problem hiding this comment.
Calling initECDHProvider once vs 10 times is a bit of a microoptimisation which sacrifices readability, for example, I had to read twice to understand why this was a static variable, if it was used anywhere else, etc
There was a problem hiding this comment.
Agreed. However, I removed ThreadLocal caching due to the issue Andrei pointed out and now it is important we don't throw an exception every time if ACCP doesn't load for some reason.
|
I'm guessing we will use ACCP for other encryption functions in future MRs? or does this automatically optimise the other encryption functions as well? |
It explicitly only registers ACCP for ECDH. I ran a few benchmarks and found that ACCP (over gcompat which we have to use due to alpine) is slower than native SunJCE for AES. If we want to use ACCP for other crypto we should consider moving to a glibc based distribution where I have found ACCP AES is up to 3 times faster. |
| return KeyAgreement.getInstance("ECDH"); | ||
| } | ||
|
|
||
| public static KeyAgreement getKeyAgreement() { |
There was a problem hiding this comment.
This has a subtle semantic change comparing to KeyAgreement.getInstance():
- the original method returned a new object; if called multiple times - every call would result in a new object and returned results can be used independently of each other
- in your case the caller would get the same object (assuming same thread) - if you try to use both in parallel (e.g. to compute with two different private keys) for whatever reason, it wouldn't work
In the current codebase it does not matter, but over time requirements might change this could bite. How much benefit do you get from re-using a threadlocal vs creating a new instance every time? How can the new semantics be made clearer to the caller?
There was a problem hiding this comment.
That is true. I checked JFR profiling and getInstance (in this case) doesn't cost us much. I removed the ThreadLocal caching and made createKeyAgreement public, in case requirements change. Thanks for pointing this out. I did not think of this case.
There was a problem hiding this comment.
I've approved the PR, but please check again how much CPU is spent initialising the key agreement object after this is deployed
…ecdh-crypto-to-accp
Synthetic Benchmarks:
Tests done: