Re: RFR: 8261407: ReflectionFactory.checkInitted() is not thread-safe [v5]
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 fields. > > I made a commit with the config class converted into a record. Apparently the > tests are passing, and I would assume it would be feasible. Should I apply it? > https://github.com/liachmodded/jdk/commit/8cf5af417a6f906e9fc0c878d60731d6f026b528 yes and I can take a closer it. Indy is ready to use very early during VM initialization before initPhase2 where the module system is initialized. AFAIU, indy is needed for the object methods for records i.e. `equals`, `hashCode`, and `toString`. Just record object instantiation and accessing its final fields don't use indy. - PR: https://git.openjdk.java.net/jdk/pull/6889
Re: RFR: 8261407: ReflectionFactory.checkInitted() is not thread-safe [v5]
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 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 fields. I made a commit with the config class converted into a record. Apparently the tests are passing, and I would assume it would be feasible. Should I apply it? https://github.com/liachmodded/jdk/commit/8cf5af417a6f906e9fc0c878d60731d6f026b528 - PR: https://git.openjdk.java.net/jdk/pull/6889
Re: RFR: 8261407: ReflectionFactory.checkInitted() is not thread-safe [v5]
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 to make this a static `Config.getInstance()` method to return a
>> `Config` instance and that should assert if this is called after the module
>> system is initialized. The `Config` constructor is a simple constructor
>> taking one parameter for each field and just set all fields to the parameter
>> value.
>
> 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 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 fields.
-
PR: https://git.openjdk.java.net/jdk/pull/6889
Re: RFR: 8261407: ReflectionFactory.checkInitted() is not thread-safe [v5]
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 the last revision:
>>
>> - use peter's model of separate data object
>> - Merge branch 'master' into 8261407-reflectionfactory
>> - Include the stable annotation
>> - 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
>
> 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 to make this a static `Config.getInstance()` method to return a
> `Config` instance and that should assert if this is called after the module
> system is initialized. The `Config` constructor is a simple constructor
> taking one parameter for each field and just set all fields to the parameter
> value.
Can I just write the config class as a record, or does it generate too much
boilerplate?
-
PR: https://git.openjdk.java.net/jdk/pull/6889
Re: RFR: 8261407: ReflectionFactory.checkInitted() is not thread-safe [v5]
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 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 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 last revision:
>
> - use peter's model of separate data object
> - Merge branch 'master' into 8261407-reflectionfactory
> - Include the stable annotation
> - 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
This looks good. I have a couple of small suggestions.
src/java.base/share/classes/jdk/internal/reflect/ReflectionFactory.java line
621:
> 619: java.lang.reflect.AccessibleObject) causes this class's to be
> 620: run, before the system properties are set up. */
> 621: private static Config config;
This can be made `@Stable` as this is initialized only once.
src/java.base/share/classes/jdk/internal/reflect/ReflectionFactory.java line
643:
> 641: */
> 642: private static final class Config {
> 643: private static final Config fallback = new Config(false);
Suggestion:
private static final Config DEFAULT = new Config(false);
I suggest to rename `fallback` to `DEFAULT`.
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 to make this a static `Config.getInstance()` method to return a
`Config` instance and that should assert if this is called after the module
system is initialized. The `Config` constructor is a simple constructor
taking one parameter for each field and just set all fields to the parameter
value.
-
PR: https://git.openjdk.java.net/jdk/pull/6889
Re: RFR: 8261407: ReflectionFactory.checkInitted() is not thread-safe [v5]
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 last revision:
>>
>> - use peter's model of separate data object
>> - Merge branch 'master' into 8261407-reflectionfactory
>> - Include the stable annotation
>> - 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
>
> Updated per suggestion of peter and mandy. Requesting another round of review.
Hi @liach ,
This looks good. Maybe we could even make these configuration parameters
constant-foldable so they are basically constants if/when JIT compiles the
code. 1st, you would have to mark the static `config` field as `@Stable` and
move the fast-path check above the check for `VM.isModuleSystemInited()` like
this:
private static @Stable Config config;
private static Config config() {
Config c = config;
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 Config.fallback;
}
config = c = new Config(true);
return c;
}
And that's basically it. The instance final fields in Config class are trusted
by VM since they live in trusted package `jdk.internal.reflect` (see:
s[rc/hotspot/share/ci/ciField.cpp:227](https://github.com/openjdk/jdk/blob/master/src/hotspot/share/ci/ciField.cpp#L227),
so JIT should constant-fold their values when it compiles the code given that
VM has probably already initialized the module system by that time and the
`config` field has a non-null reference.
-
PR: https://git.openjdk.java.net/jdk/pull/6889
Re: RFR: 8261407: ReflectionFactory.checkInitted() is not thread-safe [v5]
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 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 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 last revision: > > - use peter's model of separate data object > - Merge branch 'master' into 8261407-reflectionfactory > - Include the stable annotation > - 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 Updated per suggestion of peter and mandy. Requesting another round of review. - PR: https://git.openjdk.java.net/jdk/pull/6889
Re: RFR: 8261407: ReflectionFactory.checkInitted() is not thread-safe [v5]
> 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 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 last revision: - use peter's model of separate data object - Merge branch 'master' into 8261407-reflectionfactory - Include the stable annotation - 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 - Changes: - all: https://git.openjdk.java.net/jdk/pull/6889/files - new: https://git.openjdk.java.net/jdk/pull/6889/files/f237cf50..12086557 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=6889=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk=6889=03-04 Stats: 18301 lines in 782 files changed: 12300 ins; 3056 del; 2945 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
