RFR: 8309688: Data race on java.io.ClassCache$CacheRef.strongReferent [v2]

[Aleksey Shipilev]

On Tue, 13 Jun 2023 19:24:08 GMT, Man Cao <manc at openjdk.org> wrote:

>> Hi all,
>> 
>> Could anyone review this small fix for a data race in java.io.ClassCache$CacheRef? This fix makes the code safer by making the code data-race-free.
>
> Man Cao has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains two additional commits since the last revision:
> 
>  - Merge branch 'master' into JDK8309688
>  - 8309688: Data race on java.io.ClassCache.strongReferent

I think this data race is benign, because `ClassValue` gives us the required memory semantics. Introducing additional performance hogs seems weird in this case, see below.

Since TSAN complains about the unsynchronized conflicting accesses in `getStrong()` and `clearStrong()`, maybe the better solution would be marking `strong` as `volatile`? It would still be awkward, but we would "only" pay a price of volatile load on a hot path.

src/java.base/share/classes/java/io/ClassCache.java line 43:

> 41:     private static class CacheRef<T> extends SoftReference<T> {
> 42:         private final Class<?> type;
> 43:         private final AtomicReference<T> strongReferent;

This introduces a dereference and additional `AR` instance per `CacheRef`. Maybe it should be `AtomicReferenceFieldUpdater` or a `VarHandle`, assuming there are no bootstrapping issues with this code.

src/java.base/share/classes/java/io/ClassCache.java line 87:

> 85:             // This guarantees progress for at least one thread on every CacheRef.
> 86:             // Clear the strong referent before returning to make the cache soft.
> 87:             T strongVal = ref.getAndClearStrong();

This is a cache, so IIRC, the prevailing case is when CacheRef already exists in the map, and its `strong` is already `null`. So this change introduces an atomic op, which is mostly useless on this path. So we pay a latency price on most `get()`-s without much benefit.

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

Changes requested by shade (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/14386#pullrequestreview-1477926122
PR Review Comment: https://git.openjdk.org/jdk/pull/14386#discussion_r1228605313
PR Review Comment: https://git.openjdk.org/jdk/pull/14386#discussion_r1228607613