Re: RFR: 8261407: ReflectionFactory.checkInitted() is not thread-safe [v9]

2022-02-22 Thread liach
On Sat, 19 Feb 2022 16:06:35 GMT, liach wrote: >> Upon review of [8261407](https://bugs.openjdk.java.net/browse/JDK-8261407), >> by design, duplicate initialization of ReflectionFactory should be safe as >> it performs side-effect-free property read actions, and the suggesting of >> making

Re: RFR: 8261407: ReflectionFactory.checkInitted() is not thread-safe [v9]

2022-02-21 Thread Mandy Chung
On Mon, 21 Feb 2022 16:35:09 GMT, liach 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 13 additional commits since >> the last

Re: RFR: 8261407: ReflectionFactory.checkInitted() is not thread-safe [v9]

2022-02-21 Thread Mandy Chung
On Sat, 19 Feb 2022 16:06:35 GMT, liach wrote: >> Upon review of [8261407](https://bugs.openjdk.java.net/browse/JDK-8261407), >> by design, duplicate initialization of ReflectionFactory should be safe as >> it performs side-effect-free property read actions, and the suggesting of >> making

Re: RFR: 8261407: ReflectionFactory.checkInitted() is not thread-safe [v9]

2022-02-21 Thread Peter Levart
On Mon, 21 Feb 2022 16:35:09 GMT, liach 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 13 additional commits since >> the last

Re: RFR: 8261407: ReflectionFactory.checkInitted() is not thread-safe [v9]

2022-02-21 Thread liach
On Sat, 19 Feb 2022 16:06:35 GMT, liach wrote: >> Upon review of [8261407](https://bugs.openjdk.java.net/browse/JDK-8261407), >> by design, duplicate initialization of ReflectionFactory should be safe as >> it performs side-effect-free property read actions, and the suggesting of >> making

Re: RFR: 8261407: ReflectionFactory.checkInitted() is not thread-safe [v9]

2022-02-19 Thread liach
> Upon review of [8261407](https://bugs.openjdk.java.net/browse/JDK-8261407), > by design, duplicate initialization of ReflectionFactory should be safe as it > performs side-effect-free property read actions, and the suggesting of making > the `initted` field volatile cannot prevent concurrent

Re: RFR: 8261407: ReflectionFactory.checkInitted() is not thread-safe [v8]

2022-02-19 Thread liach
> Upon review of [8261407](https://bugs.openjdk.java.net/browse/JDK-8261407), > by design, duplicate initialization of ReflectionFactory should be safe as it > performs side-effect-free property read actions, and the suggesting of making > the `initted` field volatile cannot prevent concurrent

Re: RFR: 8261407: ReflectionFactory.checkInitted() is not thread-safe [v7]

2022-02-14 Thread Mandy Chung
On Mon, 14 Feb 2022 08:01:38 GMT, Peter Levart wrote: >> src/java.base/share/classes/jdk/internal/reflect/ReflectionFactory.java line >> 620: >> >>> 618: * The configurations exist as an object to avoid race conditions. >>> 619: * See bug 8261407. The object methods backed by indy

Re: RFR: 8261407: ReflectionFactory.checkInitted() is not thread-safe [v7]

2022-02-14 Thread Peter Levart
On Fri, 11 Feb 2022 21:44:04 GMT, Mandy Chung wrote: > Note that the object methods of this Config record are called via indy which > is > available to use after initPhase1. We can workaround that limitation by > implementing the object methods. "Note that the object methods of this Config

Re: RFR: 8261407: ReflectionFactory.checkInitted() is not thread-safe [v7]

2022-02-11 Thread Mandy Chung
On Fri, 11 Feb 2022 13:51:38 GMT, liach wrote: >> Upon review of [8261407](https://bugs.openjdk.java.net/browse/JDK-8261407), >> by design, duplicate initialization of ReflectionFactory should be safe as >> it performs side-effect-free property read actions, and the suggesting of >> making

Re: RFR: 8261407: ReflectionFactory.checkInitted() is not thread-safe [v7]

2022-02-11 Thread liach
On Fri, 11 Feb 2022 13:51:38 GMT, liach wrote: >> Upon review of [8261407](https://bugs.openjdk.java.net/browse/JDK-8261407), >> by design, duplicate initialization of ReflectionFactory should be safe as >> it performs side-effect-free property read actions, and the suggesting of >> making

Re: RFR: 8261407: ReflectionFactory.checkInitted() is not thread-safe [v6]

2022-02-11 Thread Peter Levart
On Fri, 11 Feb 2022 13:42:01 GMT, liach wrote: >> ...having suggested that rearrangement, perhaps the right way to do it is to >> enable some VM.isXXX queries themselves to be constant-foldable so that >> other callers too may benefit. Like this: >>

Re: RFR: 8261407: ReflectionFactory.checkInitted() is not thread-safe [v7]

2022-02-11 Thread Peter Levart
On Fri, 11 Feb 2022 13:51:38 GMT, liach wrote: >> Upon review of [8261407](https://bugs.openjdk.java.net/browse/JDK-8261407), >> by design, duplicate initialization of ReflectionFactory should be safe as >> it performs side-effect-free property read actions, and the suggesting of >> making

Re: RFR: 8261407: ReflectionFactory.checkInitted() is not thread-safe [v7]

2022-02-11 Thread liach
> Upon review of [8261407](https://bugs.openjdk.java.net/browse/JDK-8261407), > by design, duplicate initialization of ReflectionFactory should be safe as it > performs side-effect-free property read actions, and the suggesting of making > the `initted` field volatile cannot prevent concurrent

Re: RFR: 8261407: ReflectionFactory.checkInitted() is not thread-safe [v6]

2022-02-11 Thread liach
On Fri, 11 Feb 2022 08:25:16 GMT, Peter Levart wrote: >> src/java.base/share/classes/jdk/internal/reflect/ReflectionFactory.java line >> 685: >> >>> 683: instance = c = load(); >>> 684: } >>> 685: return c; >> >> If you do that the "old" way, you loose

Re: RFR: 8261407: ReflectionFactory.checkInitted() is not thread-safe [v6]

2022-02-11 Thread Peter Levart
On Fri, 11 Feb 2022 08:05:30 GMT, Peter Levart wrote: >> liach has updated the pull request incrementally with one additional commit >> since the last revision: >> >> Make config a pojo, move loading code into config class > >

Re: RFR: 8261407: ReflectionFactory.checkInitted() is not thread-safe [v6]

2022-02-11 Thread Peter Levart
On Thu, 10 Feb 2022 22:53:56 GMT, liach wrote: >> Upon review of [8261407](https://bugs.openjdk.java.net/browse/JDK-8261407), >> by design, duplicate initialization of ReflectionFactory should be safe as >> it performs side-effect-free property read actions, and the suggesting of >> making

Re: RFR: 8261407: ReflectionFactory.checkInitted() is not thread-safe [v5]

2022-02-10 Thread Mandy Chung
On Fri, 11 Feb 2022 02:08:27 GMT, liach wrote: >> Worth a try. Even the regular class, the constructor taking 5 fields isn't >> too bad to me. In a near future, I hope to remove the old core reflection >> implementation, `noInflation` and `inflationThreshold` will be removed and >> fewer

Re: RFR: 8261407: ReflectionFactory.checkInitted() is not thread-safe [v5]

2022-02-10 Thread liach
On Thu, 10 Feb 2022 22:49:38 GMT, Mandy Chung wrote: >> Can I just write the config class as a record, or does it generate too much >> boilerplate? Or is this class initialized too early to use records (such as >> indy is not yet ready)? > > Worth a try. Even the regular class, the

Re: RFR: 8261407: ReflectionFactory.checkInitted() is not thread-safe [v5]

2022-02-10 Thread Mandy Chung
On Thu, 10 Feb 2022 22:21:32 GMT, liach wrote: >> src/java.base/share/classes/jdk/internal/reflect/ReflectionFactory.java line >> 672: >> >>> 670: private final boolean disableSerialConstructorChecks; >>> 671: >>> 672: private Config(boolean getProperties) { >> >> I suggest

Re: RFR: 8261407: ReflectionFactory.checkInitted() is not thread-safe [v6]

2022-02-10 Thread liach
> Upon review of [8261407](https://bugs.openjdk.java.net/browse/JDK-8261407), > by design, duplicate initialization of ReflectionFactory should be safe as it > performs side-effect-free property read actions, and the suggesting of making > the `initted` field volatile cannot prevent concurrent

Re: RFR: 8261407: ReflectionFactory.checkInitted() is not thread-safe [v5]

2022-02-10 Thread liach
On Thu, 10 Feb 2022 19:45:24 GMT, Mandy Chung 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 seven additional commits >> since

Re: RFR: 8261407: ReflectionFactory.checkInitted() is not thread-safe [v5]

2022-02-10 Thread Mandy Chung
On Thu, 10 Feb 2022 02:24:43 GMT, liach wrote: >> Upon review of [8261407](https://bugs.openjdk.java.net/browse/JDK-8261407), >> by design, duplicate initialization of ReflectionFactory should be safe as >> it performs side-effect-free property read actions, and the suggesting of >> making

Re: RFR: 8261407: ReflectionFactory.checkInitted() is not thread-safe [v5]

2022-02-10 Thread Peter Levart
On Thu, 10 Feb 2022 13:36:11 GMT, liach 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 seven additional commits >> since the

Re: RFR: 8261407: ReflectionFactory.checkInitted() is not thread-safe [v5]

2022-02-10 Thread liach
On Thu, 10 Feb 2022 02:24:43 GMT, liach wrote: >> Upon review of [8261407](https://bugs.openjdk.java.net/browse/JDK-8261407), >> by design, duplicate initialization of ReflectionFactory should be safe as >> it performs side-effect-free property read actions, and the suggesting of >> making

Re: RFR: 8261407: ReflectionFactory.checkInitted() is not thread-safe [v5]

2022-02-09 Thread liach
> Upon review of [8261407](https://bugs.openjdk.java.net/browse/JDK-8261407), > by design, duplicate initialization of ReflectionFactory should be safe as it > performs side-effect-free property read actions, and the suggesting of making > the `initted` field volatile cannot prevent concurrent

Re: RFR: 8261407: ReflectionFactory.checkInitted() is not thread-safe [v4]

2022-02-09 Thread Mandy Chung
On Sat, 22 Jan 2022 00:05:49 GMT, liach wrote: >> Upon review of [8261407](https://bugs.openjdk.java.net/browse/JDK-8261407), >> by design, duplicate initialization of ReflectionFactory should be safe as >> it performs side-effect-free property read actions, and the suggesting of >> making

Re: RFR: 8261407: ReflectionFactory.checkInitted() is not thread-safe [v4]

2022-02-09 Thread liach
On Sat, 22 Jan 2022 00:05:49 GMT, liach wrote: >> Upon review of [8261407](https://bugs.openjdk.java.net/browse/JDK-8261407), >> by design, duplicate initialization of ReflectionFactory should be safe as >> it performs side-effect-free property read actions, and the suggesting of >> making

Re: RFR: 8261407: ReflectionFactory.checkInitted() is not thread-safe [v3]

2022-02-09 Thread Peter Levart
On Sat, 22 Jan 2022 00:00:18 GMT, liach 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

Re: RFR: 8261407: ReflectionFactory.checkInitted() is not thread-safe [v3]

2022-01-21 Thread liach
On Fri, 21 Jan 2022 23:51:02 GMT, liach wrote: >> Upon review of [8261407](https://bugs.openjdk.java.net/browse/JDK-8261407), >> by design, duplicate initialization of ReflectionFactory should be safe as >> it performs side-effect-free property read actions, and the suggesting of >> making

Re: RFR: 8261407: ReflectionFactory.checkInitted() is not thread-safe [v4]

2022-01-21 Thread liach
> Upon review of [8261407](https://bugs.openjdk.java.net/browse/JDK-8261407), > by design, duplicate initialization of ReflectionFactory should be safe as it > performs side-effect-free property read actions, and the suggesting of making > the `initted` field volatile cannot prevent concurrent

Re: RFR: 8261407: ReflectionFactory.checkInitted() is not thread-safe [v3]

2022-01-21 Thread liach
> Upon review of [8261407](https://bugs.openjdk.java.net/browse/JDK-8261407), > by design, duplicate initialization of ReflectionFactory should be safe as it > performs side-effect-free property read actions, and the suggesting of making > the `initted` field volatile cannot prevent concurrent

Re: RFR: 8261407: ReflectionFactory.checkInitted() is not thread-safe [v2]

2022-01-17 Thread liach
On Sun, 19 Dec 2021 06:51:45 GMT, liach wrote: >> Upon review of [8261407](https://bugs.openjdk.java.net/browse/JDK-8261407), >> by design, duplicate initialization of ReflectionFactory should be safe as >> it performs side-effect-free property read actions, and the suggesting of >> making

Re: RFR: 8261407: ReflectionFactory.checkInitted() is not thread-safe [v2]

2021-12-19 Thread Alan Bateman
On Sun, 19 Dec 2021 20:34:22 GMT, liach wrote: >> src/java.base/share/classes/jdk/internal/reflect/ReflectionFactory.java line >> 65: >> >>> 63: >>> 64: // volatile ensures static fields set before are published >>> 65: private static volatile boolean initted = false; >> >> I assume

Re: RFR: 8261407: ReflectionFactory.checkInitted() is not thread-safe [v2]

2021-12-19 Thread liach
On Sun, 19 Dec 2021 08:45:02 GMT, Alan Bateman wrote: >> liach has updated the pull request incrementally with one additional commit >> since the last revision: >> >> Just use volatile directly to ensure read order > > src/java.base/share/classes/jdk/internal/reflect/ReflectionFactory.java

Re: RFR: 8261407: ReflectionFactory.checkInitted() is not thread-safe [v2]

2021-12-19 Thread David Holmes
On Sun, 19 Dec 2021 06:51:45 GMT, liach wrote: >> Upon review of [8261407](https://bugs.openjdk.java.net/browse/JDK-8261407), >> by design, duplicate initialization of ReflectionFactory should be safe as >> it performs side-effect-free property read actions, and the suggesting of >> making

Re: RFR: 8261407: ReflectionFactory.checkInitted() is not thread-safe [v2]

2021-12-19 Thread David Holmes
On Sun, 19 Dec 2021 07:05:31 GMT, liach wrote: >> src/java.base/share/classes/jdk/internal/reflect/ReflectionFactory.java line >> 695: >> >>> 693: >>> 694: // ensure previous fields are visible before initted is >>> 695: Unsafe.getUnsafe().storeStoreFence(); >> >> Ensuring

Re: RFR: 8261407: ReflectionFactory.checkInitted() is not thread-safe [v2]

2021-12-19 Thread Alan Bateman
On Sun, 19 Dec 2021 06:51:45 GMT, liach wrote: >> Upon review of [8261407](https://bugs.openjdk.java.net/browse/JDK-8261407), >> by design, duplicate initialization of ReflectionFactory should be safe as >> it performs side-effect-free property read actions, and the suggesting of >> making

Re: RFR: 8261407: ReflectionFactory.checkInitted() is not thread-safe [v2]

2021-12-18 Thread liach
On Sun, 19 Dec 2021 05:56:55 GMT, David Holmes wrote: >> liach has updated the pull request incrementally with one additional commit >> since the last revision: >> >> Just use volatile directly to ensure read order > > src/java.base/share/classes/jdk/internal/reflect/ReflectionFactory.java

Re: RFR: 8261407: ReflectionFactory.checkInitted() is not thread-safe [v2]

2021-12-18 Thread liach
> Upon review of [8261407](https://bugs.openjdk.java.net/browse/JDK-8261407), > by design, duplicate initialization of ReflectionFactory should be safe as it > performs side-effect-free property read actions, and the suggesting of making > the `initted` field volatile cannot prevent concurrent

Re: RFR: 8261407: ReflectionFactory.checkInitted() is not thread-safe

2021-12-18 Thread David Holmes
On Sun, 19 Dec 2021 03:21:55 GMT, liach wrote: > Upon review of [8261407](https://bugs.openjdk.java.net/browse/JDK-8261407), > by design, duplicate initialization of ReflectionFactory should be safe as it > performs side-effect-free property read actions, and the suggesting of making > the

RFR: 8261407: ReflectionFactory.checkInitted() is not thread-safe

2021-12-18 Thread liach
Upon review of [8261407](https://bugs.openjdk.java.net/browse/JDK-8261407), by design, duplicate initialization of ReflectionFactory should be safe as it performs side-effect-free property read actions, and the suggesting of making the `initted` field volatile cannot prevent concurrent