On Sat, 22 Jan 2022 00:00:18 GMT, liach <d...@openjdk.java.net> wrote:
>> liach 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 four additional commits since >> the last revision: >> >> - Merge branch 'master' into 8261407-reflectionfactory >> - Merge branch '8261407-reflectionfactory' >> - Just use volatile directly to ensure read order >> - 8261407: ReflectionFactory.checkInitted() is not thread-safe > > The addition of `@Stable` seems safe. Here's a comparison for the > `checkInitted` method under current patch (volatile) and the stable > annotation (stable): https://gist.github.com/b6a1090872e686f31595bcd778893e82 > Under this test setup: > https://gist.github.com/96018d7dcaa07763d1c205017a9bd99f > Can any professional review the comparison above, as I am not as acquainted > to C assembly and VM internals to confirm there is indeed no unintended side > effects? Hi @liach, Your patch seems OK, but is otherwise fixing a not well designed initialization process that has seen many updates in its lifetime. The code is not idiomatic, relies on side-effects triggered from arbitrary code paths (checkInited) which makes it hard to understand and maintain correctly. So why not making the code more "functional-oriented" and limit side-effects to a single well-understood place. For example, like this: https://github.com/plevart/jdk/commit/b62c2ce9673e40b5b051ba5962c6bfa34e172a87 ...using this approach, the initialization is guaranteed to happen when any of the fields in Config class needs to be accessed from wherever it may be without making sure that checkInitted() is called at appropriate time. This is, IMHO, more robust than current approach and less prone to bugs. And you get guaranteed visibility of final fields in Config as a bonus without relying on ordering of writes/reads with volatile boolean initted.. ------------- PR: https://git.openjdk.java.net/jdk/pull/6889
