RFR: 8345954: Revisit Class Initializers and Locking in X509TrustManagerImpl [v2]

[Artur Barashev]

On Thu, 22 Jan 2026 23:51:42 GMT, Koushik Muthukrishnan Thirupattur <duke at openjdk.org> wrote:

>> Refactor sun.security.provider.X509Factory cache access to avoid coarse-grained locking and reduce contention during certificate/CRL interning and parsing.
>> 
>> As per request in [the PR](https://github.com/openjdk/jdk/pull/22616#issuecomment-2524971845), re-visit "the initialisation and locking in this area, e.g. addToCache is a static synchronized method so very coarse locking."
>
> Koushik Muthukrishnan Thirupattur has updated the pull request incrementally with one additional commit since the last revision:
> 
>   8345954: Addressing review comments

src/java.base/share/classes/sun/security/provider/X509Factory.java line 118:

> 116:         X509CertImpl newCert = new X509CertImpl(encoding);
> 117:         byte[] enc = newCert.getEncodedInternal();
> 118:         return addIfNotPresent(certCache, enc, newCert);

Same: Why we call `addIfNotPresent` if we already checked above that it's not present?

src/java.base/share/classes/sun/security/provider/X509Factory.java line 234:

> 232:      */
> 233:     private static <V> V getFromCache(Cache<Object, V> cache, byte[] encoding) {
> 234:         synchronized (cache) {

This `synchronized` is redundant. This cache's `get` method is already synchronized.

src/java.base/share/classes/sun/security/provider/X509Factory.java line 242:

> 240:      * Add the X509CertImpl or X509CRLImpl to the cache.
> 241:      */
> 242:     private static <V> V addIfNotPresent(Cache<Object, V> cache, byte[] encoding, V value) {

Why do we need this method? How is it different from simply calling `put` if both current cache value and possibly-present-in-the-cache value are identical?

src/java.base/share/classes/sun/security/provider/X509Factory.java line 413:

> 411:                 // Build outside lock
> 412:                 X509CRLImpl crl = new X509CRLImpl(encoding);
> 413:                 byte[] enc = crl.getEncodedInternal();

How `enc` is different from  `encoding`? Isn't they the same?

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

PR Review Comment: https://git.openjdk.org/jdk/pull/29181#discussion_r2743262746
PR Review Comment: https://git.openjdk.org/jdk/pull/29181#discussion_r2743236443
PR Review Comment: https://git.openjdk.org/jdk/pull/29181#discussion_r2743243270
PR Review Comment: https://git.openjdk.org/jdk/pull/29181#discussion_r2743253752