On Tue, 29 Jul 2025 15:11:45 GMT, Artur Barashev <abaras...@openjdk.org> 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