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
