Re: RFR: 8256487: Handle disableEagerInitialization for archived lambda proxy classes
On Thu, 19 Nov 2020 16:09:41 GMT, Claes Redestad wrote: >> Hi Claes, >> >> Thanks for taking a look. >> >> So should I keep the following `!initialize` check in >> LambdaProxyClassArchive? >> 109 if (!loadedByBuiltinLoader(caller) || !initialize || >> 110 !CDS.isSharingEnabled() || isSerializable || >> markerInterfaces.length > 0 || additionalBridges.length > 0) >> 111 return null; >> If we keep the above code, I think we don't need to pass the `initialize` to >> `findFromArchive` and eventually to `JVM_LookupLambdaProxyClassFromArchive`. >> >> Let me know if the above is what you have in mind? >> >> thanks, >> Calvin > > Right, I'd drop that argument - I would go further and suggest making calls > to both `LambdaProxyClassArchive.register` and `LambdaProxyClassArchive.find` > conditional on `disableEagerInitialization` being `false` to avoid any > accidental mix-up and reduce complexity of these orthogonal features/concerns. Closing this pull request. I'll file another bug to address suggestions from @cl4es and @mlchung. - PR: https://git.openjdk.java.net/jdk/pull/1301
Re: RFR: 8256487: Handle disableEagerInitialization for archived lambda proxy classes
On Thu, 19 Nov 2020 19:42:51 GMT, Yumin Qi wrote:
>> Before this change, the setting of the
>> `jdk.internal.lambda.disableEagerInitialization` property was not captured
>> during dumping of lambda proxy classes. There's a workaround in
>> `LambdaProxyClassArchive.find`, it won't call `findFromArchive` if the above
>> property is set.
>>
>> This change adds handling of the
>> `jdk.internal.lambda.disableEagerInitialization` property, specifically:
>>
>> - remove the above workaround;
>>
>> - capture the setting of the property in the archive header during CDS dump
>> time;
>>
>> - during runtime when finding an archived lambda proxy class, the setting of
>> the property will be compared with the stored value in the archive header.
>> If the values don't match, the archived lambda proxy class won't be used.
>>
>> Tests:
>>
>> - [x] ran all cds tests locally on linux-x64
>>
>> - [x] ran the `hotspot_appcds_dynamic` test group with
>> `-Dtest.dynamic.cds.archive=true` on linux-x64
>>
>> - [x] mach5 tiers 1,2,3 (in progress)
>
> src/hotspot/share/memory/metaspaceShared.cpp line 76:
>
>> 74: #endif
>> 75:
>> 76: #include
>
> This include is strange, usually we do not include std head file in .cpp file.
The include is needed for strcasecmp and _stricmp.
If you do a grep like `find . -name "*.cpp" | xargs grep "#include <"`, you'll
find many .cpp files include std header files.
> src/hotspot/share/memory/metaspaceShared.cpp line 1821:
>
>> 1819: }
>> 1820:
>> 1821: void MetaspaceShared::set_disable_eager_init(const char* value) {
>
> strcasecmp is not platform dependent, why not use it for all? It does not
> need included.
According to https://en.wikipedia.org/wiki/C_string_handling, strcasecmp is for
POSIX, BSD, stricmp is for Windows.
> test/hotspot/jtreg/runtime/cds/appcds/dynamicArchive/LambdaEagerInit.java
> line 35:
>
>> 33: * @library /test/lib /test/hotspot/jtreg/runtime/cds/appcds
>> 34: * @compile
>> ../../../../../../jdk/java/lang/invoke/lambda/LambdaEagerInitTest.java
>> 35: * @build sun.hotspot.WhiteBox
>
> Do we need WB here?
Our current design is that every test under the `dynamicArchive` dir extends
`DynamicArchiveTestBase` which depends on `sun.hotspot.WhiteBox`.
-
PR: https://git.openjdk.java.net/jdk/pull/1301
Re: RFR: 8256487: Handle disableEagerInitialization for archived lambda proxy classes
On Thu, 19 Nov 2020 16:09:41 GMT, Claes Redestad wrote:
>> Hi Claes,
>>
>> Thanks for taking a look.
>>
>> So should I keep the following `!initialize` check in
>> LambdaProxyClassArchive?
>> 109 if (!loadedByBuiltinLoader(caller) || !initialize ||
>> 110 !CDS.isSharingEnabled() || isSerializable ||
>> markerInterfaces.length > 0 || additionalBridges.length > 0)
>> 111 return null;
>> If we keep the above code, I think we don't need to pass the `initialize` to
>> `findFromArchive` and eventually to `JVM_LookupLambdaProxyClassFromArchive`.
>>
>> Let me know if the above is what you have in mind?
>>
>> thanks,
>> Calvin
>
> Right, I'd drop that argument - I would go further and suggest making calls
> to both `LambdaProxyClassArchive.register` and `LambdaProxyClassArchive.find`
> conditional on `disableEagerInitialization` being `false` to avoid any
> accidental mix-up and reduce complexity of these orthogonal features/concerns.
I agree with Claes that this is a wrong move to archive lambda proxies even if
`disableEagerInitialization` is set. A simple fix would be:
private Class spinInnerClass() throws LambdaConversionException {
if (!disableEagerInitialization) {
if (LambdaProxyClassArchive.isDumpArchive()) {
...
}
// load from CDS archive if present
Class archiveClass = LambdaProxyClassArchive.find(targetClass,
samMethodName,
invokedType,
samMethodType,
implMethod,
instantiatedMethodType,
isSerializable,
markerInterfaces,
additionalBridges,
true);
if (archiveClass != null) return archiveClass;
}
return generateInnerClass();
}
-
PR: https://git.openjdk.java.net/jdk/pull/1301
Re: RFR: 8256487: Handle disableEagerInitialization for archived lambda proxy classes
On Wed, 18 Nov 2020 23:58:25 GMT, Calvin Cheung wrote:
> Before this change, the setting of the
> `jdk.internal.lambda.disableEagerInitialization` property was not captured
> during dumping of lambda proxy classes. There's a workaround in
> `LambdaProxyClassArchive.find`, it won't call `findFromArchive` if the above
> property is set.
>
> This change adds handling of the
> `jdk.internal.lambda.disableEagerInitialization` property, specifically:
>
> - remove the above workaround;
>
> - capture the setting of the property in the archive header during CDS dump
> time;
>
> - during runtime when finding an archived lambda proxy class, the setting of
> the property will be compared with the stored value in the archive header.
> If the values don't match, the archived lambda proxy class won't be used.
>
> Tests:
>
> - [x] ran all cds tests locally on linux-x64
>
> - [x] ran the `hotspot_appcds_dynamic` test group with
> `-Dtest.dynamic.cds.archive=true` on linux-x64
>
> - [x] mach5 tiers 1,2,3 (in progress)
src/hotspot/share/memory/metaspaceShared.cpp line 1821:
> 1819: }
> 1820:
> 1821: void MetaspaceShared::set_disable_eager_init(const char* value) {
strcasecmp is not platform dependent, why not use it for all? It does not need
included.
test/hotspot/jtreg/runtime/cds/appcds/dynamicArchive/LambdaEagerInit.java line
35:
> 33: * @library /test/lib /test/hotspot/jtreg/runtime/cds/appcds
> 34: * @compile
> ../../../../../../jdk/java/lang/invoke/lambda/LambdaEagerInitTest.java
> 35: * @build sun.hotspot.WhiteBox
Do we need WB here?
-
PR: https://git.openjdk.java.net/jdk/pull/1301
Re: RFR: 8256487: Handle disableEagerInitialization for archived lambda proxy classes
On Wed, 18 Nov 2020 23:58:25 GMT, Calvin Cheung wrote: > Before this change, the setting of the > `jdk.internal.lambda.disableEagerInitialization` property was not captured > during dumping of lambda proxy classes. There's a workaround in > `LambdaProxyClassArchive.find`, it won't call `findFromArchive` if the above > property is set. > > This change adds handling of the > `jdk.internal.lambda.disableEagerInitialization` property, specifically: > > - remove the above workaround; > > - capture the setting of the property in the archive header during CDS dump > time; > > - during runtime when finding an archived lambda proxy class, the setting of > the property will be compared with the stored value in the archive header. > If the values don't match, the archived lambda proxy class won't be used. > > Tests: > > - [x] ran all cds tests locally on linux-x64 > > - [x] ran the `hotspot_appcds_dynamic` test group with > `-Dtest.dynamic.cds.archive=true` on linux-x64 > > - [x] mach5 tiers 1,2,3 (in progress) src/hotspot/share/memory/metaspaceShared.cpp line 76: > 74: #endif > 75: > 76: #include This include is strange, usually we do not include std head file in .cpp file. - PR: https://git.openjdk.java.net/jdk/pull/1301
Re: RFR: 8256487: Handle disableEagerInitialization for archived lambda proxy classes
On Thu, 19 Nov 2020 02:08:52 GMT, Calvin Cheung wrote: >> I'm not sure if this is a good idea, TBH. The disableEagerInitialization >> setting is for native-image pre-generation purposes and the less CDS cares >> about it, the better. I'd prefer it if there's no trace of the property in >> hotspot sources. > > Hi Claes, > > Thanks for taking a look. > > So should I keep the following `!initialize` check in LambdaProxyClassArchive? > 109 if (!loadedByBuiltinLoader(caller) || !initialize || > 110 !CDS.isSharingEnabled() || isSerializable || > markerInterfaces.length > 0 || additionalBridges.length > 0) > 111 return null; > If we keep the above code, I think we don't need to pass the `initialize` to > `findFromArchive` and eventually to `JVM_LookupLambdaProxyClassFromArchive`. > > Let me know if the above is what you have in mind? > > thanks, > Calvin Right, I'd drop that argument - I would go further and suggest making calls to both `LambdaProxyClassArchive.register` and `LambdaProxyClassArchive.find` conditional on `disableEagerInitialization` being `false` to avoid any accidental mix-up and reduce complexity of these orthogonal features/concerns. - PR: https://git.openjdk.java.net/jdk/pull/1301
Re: RFR: 8256487: Handle disableEagerInitialization for archived lambda proxy classes
On Thu, 19 Nov 2020 00:50:52 GMT, Claes Redestad wrote: >> Before this change, the setting of the >> `jdk.internal.lambda.disableEagerInitialization` property was not captured >> during dumping of lambda proxy classes. There's a workaround in >> `LambdaProxyClassArchive.find`, it won't call `findFromArchive` if the above >> property is set. >> >> This change adds handling of the >> `jdk.internal.lambda.disableEagerInitialization` property, specifically: >> >> - remove the above workaround; >> >> - capture the setting of the property in the archive header during CDS dump >> time; >> >> - during runtime when finding an archived lambda proxy class, the setting of >> the property will be compared with the stored value in the archive header. >> If the values don't match, the archived lambda proxy class won't be used. >> >> Tests: >> >> - [x] ran all cds tests locally on linux-x64 >> >> - [x] ran the `hotspot_appcds_dynamic` test group with >> `-Dtest.dynamic.cds.archive=true` on linux-x64 >> >> - [x] mach5 tiers 1,2,3 (in progress) > > I'm not sure if this is a good idea, TBH. The disableEagerInitialization > setting is for native-image pre-generation purposes and the less CDS cares > about it, the better. I'd prefer it if there's no trace of the property in > hotspot sources. Hi Claes, Thanks for taking a look. So should I keep the following `!initialize` check in LambdaProxyClassArchive? 109 if (!loadedByBuiltinLoader(caller) || !initialize || 110 !CDS.isSharingEnabled() || isSerializable || markerInterfaces.length > 0 || additionalBridges.length > 0) 111 return null; If we keep the above code, I think we don't need to pass the `initialize` to `findFromArchive` and eventually to `JVM_LookupLambdaProxyClassFromArchive`. Let me know if the above is what you have in mind? thanks, Calvin - PR: https://git.openjdk.java.net/jdk/pull/1301
Re: RFR: 8256487: Handle disableEagerInitialization for archived lambda proxy classes
On Wed, 18 Nov 2020 23:58:25 GMT, Calvin Cheung wrote: > Before this change, the setting of the > `jdk.internal.lambda.disableEagerInitialization` property was not captured > during dumping of lambda proxy classes. There's a workaround in > `LambdaProxyClassArchive.find`, it won't call `findFromArchive` if the above > property is set. > > This change adds handling of the > `jdk.internal.lambda.disableEagerInitialization` property, specifically: > > - remove the above workaround; > > - capture the setting of the property in the archive header during CDS dump > time; > > - during runtime when finding an archived lambda proxy class, the setting of > the property will be compared with the stored value in the archive header. > If the values don't match, the archived lambda proxy class won't be used. > > Tests: > > - [x] ran all cds tests locally on linux-x64 > > - [x] ran the `hotspot_appcds_dynamic` test group with > `-Dtest.dynamic.cds.archive=true` on linux-x64 > > - [x] mach5 tiers 1,2,3 (in progress) I'm not sure if this is a good idea, TBH. The disableEagerInitialization setting is for native-image pre-generation purposes and the less CDS cares about it, the better. I'd prefer it if there's no trace of the property in hotspot sources. - PR: https://git.openjdk.java.net/jdk/pull/1301
RFR: 8256487: Handle disableEagerInitialization for archived lambda proxy classes
Before this change, the setting of the `jdk.internal.lambda.disableEagerInitialization` property was not captured during dumping of lambda proxy classes. There's a workaround in `LambdaProxyClassArchive.find`, it won't call `findFromArchive` if the above property is set. This change adds handling of the `jdk.internal.lambda.disableEagerInitialization` property, specifically: - remove the above workaround; - capture the setting of the property in the archive header during CDS dump time; - during runtime when finding an archived lambda proxy class, the setting of the property will be compared with the stored value in the archive header. If the values don't match, the archived lambda proxy class won't be used. Tests: - [x] ran all cds tests locally on linux-x64 - [x] ran the `hotspot_appcds_dynamic` test group with `-Dtest.dynamic.cds.archive=true` on linux-x64 - [x] mach5 tiers 1,2,3 (in progress) - Commit messages: - Merge branch 'master' into 8256487-disableEagerInit - 8256487: Handle disableEagerInitialization for archived lambda proxy classes Changes: https://git.openjdk.java.net/jdk/pull/1301/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=1301=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8256487 Stats: 263 lines in 15 files changed: 246 ins; 0 del; 17 mod Patch: https://git.openjdk.java.net/jdk/pull/1301.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1301/head:pull/1301 PR: https://git.openjdk.java.net/jdk/pull/1301
