On Tue, 29 Jul 2025 15:11:45 GMT, Artur Barashev <[email protected]> wrote:
>> SunX509 key manager should support the same certificate checks that are
>> supported by PKIX key manager.
>>
>> Effectively there should be only 2 differences between 2 key managers:
>> - PKIX supports multiple key stores through KeyStore.Builder interface while
>> SunX509 supports only a single keystore.
>> - SunX509 caches its whole key store on initialization thus improving
>> performance. This means that subsequent modifications of the KeyStore have
>> no effect on SunX509 KM, unlike PKIX .
>>
>> **SUNX509 KeyManager performance before the change**
>> Benchmark (resume) (tlsVersion) Mode
>> Cnt Score Error Units
>> SSLHandshake.doHandshake true TLSv1.2 thrpt 15 19758.012 ±
>> 758.237 ops/s
>> SSLHandshake.doHandshake true TLS thrpt 15 1861.695 ±
>> 14.681 ops/s
>> SSLHandshake.doHandshake false TLSv1.2 thrpt 15 **1186.962**
>> ± 12.085 ops/s
>> SSLHandshake.doHandshake false TLS thrpt 15 **1056.288**
>> ± 7.197 ops/s
>>
>> **SUNX509 KeyManager performance after the change**
>> Benchmark (resume) (tlsVersion) Mode Cnt Score
>> Error Units
>> SSLHandshake.doHandshake true TLSv1.2 thrpt 15 20954.399 ±
>> 260.817 ops/s
>> SSLHandshake.doHandshake true TLS thrpt 15 1813.401 ±
>> 13.917 ops/s
>> SSLHandshake.doHandshake false TLSv1.2 thrpt 15 **1158.190**
>> ± 6.023 ops/s
>> SSLHandshake.doHandshake false TLS thrpt 15 **1012.988**
>> ± 10.943 ops/s
>
> Artur Barashev has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Address review comments
src/java.base/share/classes/sun/security/ssl/X509KeyManagerImpl.java line 88:
> 86: uidCounter = new AtomicLong();
> 87: entryCacheMap = Collections.synchronizedMap
> 88: (new SizedMap<>());
You can remove `SizedMap` on lines 94-101. Did you see any reason why a
`LinkedHashMap` was used? (I cannot, but this code has not changed in many
releases, so we should be sure its ok).
test/jdk/sun/security/ssl/X509KeyManager/CertChecking.java line 110:
> 108: private static final boolean[] NO_DG_USAGE =
> 109: new boolean[]{false, true, true, true, true, true};
> 110: private static final boolean[] NO_DG_NO_KE_USAGE =
Nit: how about NO_DS instead of NO_DG?
test/jdk/sun/security/ssl/X509KeyManager/CertChecking.java line 128:
> 126: // --- Usage and expired test cases --
> 127:
> 128: // Both should fail with no usages at all
Clarify what you mean by "Both should fail"? This test doesn't do a TLS
handshake. Maybe what you want to comment on is the order when checking is
enabled (i.e. cert with bad usage is always preferred last).
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/25016#discussion_r2240708490
PR Review Comment: https://git.openjdk.org/jdk/pull/25016#discussion_r2240672544
PR Review Comment: https://git.openjdk.org/jdk/pull/25016#discussion_r2240675270
