On Thu, 15 Jun 2023 00:16:15 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 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
