On Wed, 10 May 2023 10:03:46 GMT, Sergey Chernyshev <d...@openjdk.org> wrote:
>> Hi all, >> >> I would like to propose a patch for an issue discussed in [1][2] that fixes >> an OOME in the current code base. The issue appears when a SunJCE provider >> object reference is stored in a non-static variable, which eventually leads >> to a Java heap OOME. >> >> The solution proposed earlier [1] raised a concern that the identity of >> provider objects may be somehow broken when using `WeakHashMap<Class<? >> extends Provider>, Object>` instead of `IdentityHashMap<Provider, Object>`. >> The solution hasn't been integrated that time. Later in 2019 a performance >> improvement was introduced with [3][4] that changed `IdentityHashMap` to >> `ConcurrentHashMap` of `IdentityWrapper<Provider> `objects that maintained >> the object identity while improved performance. >> >> The solution being proposed keeps up with performance improvement in [3], >> further narrowing the synchronization down to the hash table node, avoids >> lambdas that may cause startup time regressions and maintains providers >> identity using `WeakIdentityWrapper` that extends `WeakReference<Object>` >> while avoiding depletion of Java heap by using weak pointers as the mapping >> key. Once a provider object becomes weakly reachable it is queued along with >> its reference object to the `ReferenceQueue` (a new static member in >> `JceSecurity`). The `equals()` method of the `WeakIdentityWrapper` will >> never match a new wrapper object to anything inside the hash table after the >> corresponding element's `WeakReference.get()` returned null. This leads to >> accumulating "empty" elements in the hash table. The new static function >> `expungeStaleWrappers()` drops the "empty" elements queued by GC each time >> the `getVerificationResult()` method is called. >> >> `ConcurrentHashMap`'s `computeIfAbsent()` does (partially) detect recursive >> updates for keys that are being added to empty bins. For such keys an >> `IllegalStateException` is thrown prior to recursive execution of the >> `mappingFunction`. For nodes that are added to existing TreeBins or linked >> lists, in which case no recursion detection is done prior to calling >> `mappingFunction`, the recursion detection relies on old method with >> `IdentityMap` of Providers. >> >> I include a test that runs under memory constrained conditions (128M) that >> hits the heap limit in current VMs where it is impossible to reclaim >> providers' memory (unless they've been cached under weak references). The >> updated jdk versions pass the test. >> >> Testing: jtreg + JCK on a downport of the patch to JDK 17 with no >> regressio... > > Sergey Chernyshev has updated the pull request incrementally with one > additional commit since the last revision: > > addressed review comment Marked as reviewed by valeriep (Reviewer). ------------- PR Review: https://git.openjdk.org/jdk/pull/13658#pullrequestreview-1425046189
