On Tue, 13 Jun 2023 19:24:08 GMT, Man Cao <m...@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
