Re: RFR: 8261407: ReflectionFactory.checkInitted() is not thread-safe [v6]
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: >> https://github.com/plevart/jdk/commit/e918ccc52bbc288f6721af5fa66d8f7a8cc880cf >> WDYT? > > I believe your patch to fold these methods is a good choice: for example, > `FileSystems.getDefault()` will be constant-foldable as a result. > For shutdown, the benefit may look negligible, but a consistency in style is > beneficial. > To make this more efficient, I recommend looking at the callers to > `VM.initLevel()` and replace with such boolean checks if possible: for > example, `ClassLoader.getSystemClassLoader` may be constant-foldable if its > default branch of switch on init level become a dedicated fast path. > > Since this change affects multiple components and beyond the reflection > factory itself, I don't think I will include it here; I will just use the > right arrangement. This belongs to another jbs ticket. Right, I wasn't suggesting to include that in the patch. Local rearrangement is OK anyway. Latest code looks good to me. - PR: https://git.openjdk.java.net/jdk/pull/6889
Re: RFR: 8261407: ReflectionFactory.checkInitted() is not thread-safe [v6]
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 the ability for JIT to constant-fold
>> the `instance` and by transitivity the Config instance fields, since the
>> check for `VM.isModuleSystemInited()` can't be elided. As suggested, the
>> fast-path check should be done 1st, like:
>>
>>
>> private static @Stable Config instance;
>>
>> private static Config instance() {
>> Config c = instance;
>> if (c != null) {
>> return c;
>> }
>>
>> // Defer initialization until module system is initialized so as
>> // to avoid inflation and spinning bytecode in unnamed modules
>> // during early startup.
>> if (!VM.isModuleSystemInited()) {
>> return DEFAULT;
>> }
>>
>> instance = c = load();
>> return c;
>> }
>
> ...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:
> https://github.com/plevart/jdk/commit/e918ccc52bbc288f6721af5fa66d8f7a8cc880cf
> WDYT?
I believe your patch to fold these methods is a good choice: for example,
`FileSystems.getDefault()` will be constant-foldable as a result.
For shutdown, the benefit may look negligible, but a consistency in style is
beneficial.
To make this more efficient, I recommend looking at the callers to
`VM.initLevel()` and replace with such boolean checks if possible: for example,
`ClassLoader.getSystemClassLoader` may be constant-foldable if its default
branch of switch on init level become a dedicated fast path.
Since this change affects multiple components and beyond the reflection factory
itself, I don't think I will include it here; I will just use the right
arrangement.
-
PR: https://git.openjdk.java.net/jdk/pull/6889
Re: RFR: 8261407: ReflectionFactory.checkInitted() is not thread-safe [v6]
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
>
> 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 the ability for JIT to constant-fold
> the `instance` and by transitivity the Config instance fields, since the
> check for `VM.isModuleSystemInited()` can't be elided. As suggested, the
> fast-path check should be done 1st, like:
>
>
> private static @Stable Config instance;
>
> private static Config instance() {
> Config c = instance;
> if (c != null) {
> return c;
> }
>
> // Defer initialization until module system is initialized so as
> // to avoid inflation and spinning bytecode in unnamed modules
> // during early startup.
> if (!VM.isModuleSystemInited()) {
> return DEFAULT;
> }
>
> instance = c = load();
> return c;
> }
...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:
https://github.com/plevart/jdk/commit/e918ccc52bbc288f6721af5fa66d8f7a8cc880cf
WDYT?
-
PR: https://git.openjdk.java.net/jdk/pull/6889
Re: RFR: 8261407: ReflectionFactory.checkInitted() is not thread-safe [v6]
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 the `initted` field volatile cannot prevent concurrent initialization
>> either; however, having `initted == true` published without the other
>> fields' values is a possibility, which this patch addresses.
>>
>> This simulates what's done in `CallSite`'s constructor for
>> `ConstantCallSite`. Please feel free to point out the problems with this
>> patch, as I am relatively inexperienced in this field of fences and there
>> are relatively less available documents. (Thanks to
>> https://shipilev.net/blog/2014/on-the-fence-with-dependencies/)
>
> 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
Changes requested by plevart (Reviewer).
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 the ability for JIT to constant-fold
the `instance` and by transitivity the Config instance fields, since the check
for `VM.isModuleSystemInited()` can't be elided. As suggested, the fast-path
check should be done 1st, like:
private static @Stable Config instance;
private static Config instance() {
Config c = instance;
if (c != null) {
return c;
}
// Defer initialization until module system is initialized so as
// to avoid inflation and spinning bytecode in unnamed modules
// during early startup.
if (!VM.isModuleSystemInited()) {
return DEFAULT;
}
instance = c = load();
return c;
}
-
PR: https://git.openjdk.java.net/jdk/pull/6889
Re: RFR: 8261407: ReflectionFactory.checkInitted() is not thread-safe [v6]
> 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 initialization either; > however, having `initted == true` published without the other fields' values > is a possibility, which this patch addresses. > > This simulates what's done in `CallSite`'s constructor for > `ConstantCallSite`. Please feel free to point out the problems with this > patch, as I am relatively inexperienced in this field of fences and there are > relatively less available documents. (Thanks to > https://shipilev.net/blog/2014/on-the-fence-with-dependencies/) 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 - Changes: - all: https://git.openjdk.java.net/jdk/pull/6889/files - new: https://git.openjdk.java.net/jdk/pull/6889/files/12086557..f32ff988 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=6889=05 - incr: https://webrevs.openjdk.java.net/?repo=jdk=6889=04-05 Stats: 138 lines in 1 file changed: 66 ins; 48 del; 24 mod Patch: https://git.openjdk.java.net/jdk/pull/6889.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6889/head:pull/6889 PR: https://git.openjdk.java.net/jdk/pull/6889
