RFR: 8322767: TLS full handshake is slow with PKCS12KeyStore and X509KeyManagerImpl [v2]

[Hai-May Chao]

On Tue, 19 Mar 2024 06:20:53 GMT, John Jiang <jjiang at openjdk.org> wrote:

>> Hai-May Chao has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Updated with John's comments
>
> src/java.base/share/classes/sun/security/ssl/X509KeyManagerImpl.java line 82:
> 
>> 80:     private final Map<String,Reference<PrivateKeyEntry>> entryCacheMap;
>> 81: 
>> 82:     private boolean ksP12;
> 
> Could `ksP12` also be `final`?

Made it final.

> src/java.base/share/classes/sun/security/ssl/X509KeyManagerImpl.java line 106:
> 
>> 104:         this.builders = builders;
>> 105:         uidCounter = new AtomicLong();
>> 106:         KeyStore keyStore = null;
> 
> It may be better to define `keyStore` in the below for-loop.

Done.

> src/java.base/share/classes/sun/security/ssl/X509KeyManagerImpl.java line 109:
> 
>> 107:         boolean foundPKCS12 = false;
>> 108: 
>> 109:         for (int i = 0, n = builders.size(); i < n; i++) {
> 
> The index `i` just be used for retrieving the elements from the list, then it can apply enhanced for-loop.

Done.

> src/java.base/share/classes/sun/security/ssl/X509KeyManagerImpl.java line 335:
> 
>> 333: 
>> 334:             // Convert KeyStore certificates to X509Certificates
>> 335:             X509Certificate[] newCerts = Arrays.copyOf(keyStoreCerts,
> 
> Does it have to convert `Certificate[]` to `X509Certificate[]`?
> The doc for `Arrays::equals` states
>> the two arrays are equal if they contain the same elements in the same order.
> 
> It looks this method doesn't care the types of the arrays themselves.

Removed conversion.

> src/java.base/share/classes/sun/security/ssl/X509KeyManagerImpl.java line 339:
> 
>> 337:             return Arrays.equals(cachedCerts, newCerts);
>> 338:         } catch (Exception e) {
>> 339:             return false;
> 
> Should this exception be logged?

Done.

> src/java.base/share/classes/sun/security/ssl/X509KeyManagerImpl.java line 356:
> 
>> 354:         }
>> 355: 
>> 356:         PrivateKeyEntry privateKeyEntry = (PrivateKeyEntry) newEntry;
> 
> Would you like to apply pattern matching for `instanceof`?
> 
> if (!(newEntry instanceof PrivateKeyEntry privateKeyEntry)) {
>     return false;
> }
> 
> PrivateKey privateKey = privateKeyEntry.getPrivateKey();

Done.

> src/java.base/share/classes/sun/security/ssl/X509KeyManagerImpl.java line 358:
> 
>> 356:         PrivateKeyEntry privateKeyEntry = (PrivateKeyEntry) newEntry;
>> 357:         PrivateKey privateKey = privateKeyEntry.getPrivateKey();
>> 358:         Certificate[] certs = privateKeyEntry.getCertificateChain();
> 
> The method `getCertificateChain(String)` contains the below codes (line 211~213),
> 
> PrivateKeyEntry entry = getEntry(alias);
> return entry == null ? null :
>         (X509Certificate[])entry.getCertificateChain();
> 
> This method assumes `PrivateKeyEntry::getCertificateChain` should always return `X509Certificate[]`.
> 
> Then, could line 358 also cast the certificate chain to `X509Certificate[]` directly?
> If so, the following codes can be simplified.
> It may not need to check the certification type, and could not to convert the `Certificate[]` to `X509Certificate[]`.

Changed to cast the certificate chain.

> src/java.base/share/classes/sun/security/ssl/X509KeyManagerImpl.java line 410:
> 
>> 408:             return mapEntryUpdated;
>> 409:         } catch (Exception e) {
>> 410:             return false;
> 
> Should this exception be logged?

Done.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/17956#discussion_r1535125307
PR Review Comment: https://git.openjdk.org/jdk/pull/17956#discussion_r1535125365
PR Review Comment: https://git.openjdk.org/jdk/pull/17956#discussion_r1535125397
PR Review Comment: https://git.openjdk.org/jdk/pull/17956#discussion_r1535125509
PR Review Comment: https://git.openjdk.org/jdk/pull/17956#discussion_r1535125456
PR Review Comment: https://git.openjdk.org/jdk/pull/17956#discussion_r1535125429
PR Review Comment: https://git.openjdk.org/jdk/pull/17956#discussion_r1535125548
PR Review Comment: https://git.openjdk.org/jdk/pull/17956#discussion_r1535125483