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

[Aleksey Shipilev]

On Thu, 15 Jun 2023 00:16:15 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 five additional commits since the last revision:
> 
>  - Merge branch 'master' into JDK8309688
>  - Removed volatile and comment about the benign race
>  - Switch to using volatile instead of AtomicReference
>  - Merge branch 'master' into JDK8309688
>  - 8309688: Data race on java.io.ClassCache.strongReferent

Right. I have suggestions about the comment.

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

> 91:             // multiple threads calls get() with the same Class parameter.
> 92:             // Fixing this race could introduce noticeable performance penalty.
> 93:             // See the review thread for JDK-8309688 for details.

Feels like this comment belongs near `strongReferent` declaration. I suggest:


// This field is deliberately accessed without sychronization. ClassValue
// provides synchronization when CacheRef is published. However, when
// a thread reads this field, while another thread is clearing the field, it 
// would formally constitute a data race. But that data race is benign, and
// fixing it could introduce noticeable performance penalty, see JDK-8309688.
private T strongReferent;

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

Marked as reviewed by shade (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/14386#pullrequestreview-1480988146
PR Review Comment: https://git.openjdk.org/jdk/pull/14386#discussion_r1230633686