On Fri, 8 Jan 2021 13:00:16 GMT, Peter Levart <plev...@openjdk.org> wrote:
>>> So what do you think of this variant: >> >> I like it. I originally kept the fields volatile so that we don't observe >> stale values on `getForward`/`getReverse`, but you're right in that even if >> we do, the correct value will be observed when doing a volatile read in >> compute, at the additional expense of evaluating class loader relationships, >> but again, that's the slow path. >> >> IIUC, your changes mostly all flow from the decision to declare the fields >> as non-volatile; if they were still declared as volatile then it'd be >> impossible to observe null in them, I think (correct me if I'm wrong; it >> seems like you thought through this quite thoroughly) as then I don't see >> how could a volatile read happen before the initial volatile writes as the >> writes are part of the ClassValues constructor invocation and the reference >> to the ClassValues object is unavailable externally before the constructor >> completes. In any case, your approach definitely avoids any of these >> concerns so I'm inclined to go with it. > >> IIUC, your changes mostly all flow from the decision to declare the fields >> as non-volatile; if they were still declared as volatile then it'd be >> impossible to observe null in them, I think (correct me if I'm wrong; it >> seems like you thought through this quite thoroughly) as then I don't see >> how could a volatile read happen before the initial volatile writes as the >> writes are part of the ClassValues constructor invocation and the reference >> to the ClassValues object is unavailable externally before the constructor >> completes. In any case, your approach definitely avoids any of these >> concerns so I'm inclined to go with it. > > It depends entirely on the guarantees of ClassValue and not on whether the > fields are volatile or not. If ClassValue publishes the BiClassValues object > via data race then even if the fields in BiClassValues are volatile and > initialized in constructor, the publishing write in ClassValue could get > reordered before the volatile writes of the fields, so you could observe the > fields uninitialized. > I can't find in the spec of ClassValue any guarantees of ordering, but I > guess the implementation does guarantee safe publication. So if you want to > rely on ClassValue guaranteeing safe publication, you can pre-initialized the > fields in constructor and code can assume they are never null even if they > are not volatile. I checked the code of ClassValue and it can be assumed that it publishes associated values safely. The proof is that it keeps values that it publishes assigned to the final field `java.lang.ClassValue.Entry#value`. ------------- PR: https://git.openjdk.java.net/jdk/pull/1918
