On Thu, 22 Jan 2026 23:51:42 GMT, Koushik Muthukrishnan Thirupattur <[email protected]> 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
