Re: RFR: 8247536: Support for pre-generated java.lang.invoke classes in CDS static archive [v12]
On Wed, 7 Oct 2020 17:53:30 GMT, Ioi Lam wrote: >> Yumin Qi has updated the pull request with a new target base due to a merge >> or a rebase. The pull request now contains >> 23 commits: >> - Added new separate function to CDS for logging species and modified the >> existing function to log lambda form invokers. >>Changed isDumpLoadedClassList to a reasonable name isDumpingClassList as >> read only in CDS. >> - Merge branch 'master' of https://github.com/openjdk/jdk into jdk-8247536 >> - Removed unused imports. >> - Fixed comments with correct class and method name in CDS, removed unused >> variables after last change. >> - Moved and renamed cdsGenerateHolderClasses from GenerateJLIClassesHelp to >> CDS as generateLambdaFormHolderClasses. Added >>input verification function in CDS before class generation. Added more >> test scenarios. Removed trailing unused ending >>words for output of lambda form trace line in case of DumpLoadedClassList. >> - Move the check work to java, restore code in VM. Modified test code >> according to the changes. The invoke name >>verififcation is not implemented since not all the holder class are >> processed, not all the functions of processed >>holder classes are added. For holder class with DirectMethodHandle in its >> name, only the name in the >>DMH_METHOD_TYPE_MAP keyset is added, ithe line with other names just gets >> skipped silently. This makes the verification >>on invoke names difficul, a name not in the keyset should not fail the >> test. Also add a boolean to >>cdsGenerateHolderClasses to indicate call path. >> - Remove trailing word of line which is not used in holder class >> regeneration. There is a trailing LF (Line Feed) so trim >>white spaces from both front and end of the line or it will fail method >> type validation. >> - In case of exception happens during reloading class, CHECK will return >> without free the allocated buffer for class >>bytes so moved the buffer allocation and freeing to caller. Also removed >> test 6 since there is not guarantee that we >>can give a signature which will always fail. Additional changes to >> GenerateJLIClassesHelper according to review >>suggestion. >> - Merge branch 'master' of https://github.com/openjdk/jdk into jdk-8247536 >> - Merge branch 'master' of https://git.openjdk.java.net/jdk into jdk-8247536 >> - ... and 13 more: >> https://git.openjdk.java.net/jdk/compare/82fe023b...f5584dcf > > Marked as reviewed by iklam (Reviewer). passed mach5 tier1-4 - PR: https://git.openjdk.java.net/jdk/pull/193
Re: RFR: 8247536: Support for pre-generated java.lang.invoke classes in CDS static archive [v12]
On Wed, 7 Oct 2020 21:49:24 GMT, Yumin Qi wrote:
>> src/java.base/share/classes/jdk/internal/misc/CDS.java line 144:
>>
>>> 142: String line = s.trim();
>>> 143: if (!line.startsWith("[LF_RESOLVE]") &&
>>> !line.startsWith("[SPECIES_RESOLVE]")) {
>>> 144: System.out.println("Wrong prefix: " + line);
>>
>> Should this throw an exception instead?
>
> This part is for check the format only, throw exceptions will lead more
> objects generated which should not be archived
> in shared heap. Since this is only called from VM, so decide not to throw
> exception here.
The exception object will not be automatically added to the shared heap, so
it's OK to throw exceptions here.
-
PR: https://git.openjdk.java.net/jdk/pull/193
Re: RFR: 8247536: Support for pre-generated java.lang.invoke classes in CDS static archive [v12]
On Wed, 7 Oct 2020 20:36:18 GMT, Mandy Chung wrote:
>> Yumin Qi has updated the pull request with a new target base due to a merge
>> or a rebase. The pull request now contains
>> 23 commits:
>> - Added new separate function to CDS for logging species and modified the
>> existing function to log lambda form invokers.
>>Changed isDumpLoadedClassList to a reasonable name isDumpingClassList as
>> read only in CDS.
>> - Merge branch 'master' of https://github.com/openjdk/jdk into jdk-8247536
>> - Removed unused imports.
>> - Fixed comments with correct class and method name in CDS, removed unused
>> variables after last change.
>> - Moved and renamed cdsGenerateHolderClasses from GenerateJLIClassesHelp to
>> CDS as generateLambdaFormHolderClasses. Added
>>input verification function in CDS before class generation. Added more
>> test scenarios. Removed trailing unused ending
>>words for output of lambda form trace line in case of DumpLoadedClassList.
>> - Move the check work to java, restore code in VM. Modified test code
>> according to the changes. The invoke name
>>verififcation is not implemented since not all the holder class are
>> processed, not all the functions of processed
>>holder classes are added. For holder class with DirectMethodHandle in its
>> name, only the name in the
>>DMH_METHOD_TYPE_MAP keyset is added, ithe line with other names just gets
>> skipped silently. This makes the verification
>>on invoke names difficul, a name not in the keyset should not fail the
>> test. Also add a boolean to
>>cdsGenerateHolderClasses to indicate call path.
>> - Remove trailing word of line which is not used in holder class
>> regeneration. There is a trailing LF (Line Feed) so trim
>>white spaces from both front and end of the line or it will fail method
>> type validation.
>> - In case of exception happens during reloading class, CHECK will return
>> without free the allocated buffer for class
>>bytes so moved the buffer allocation and freeing to caller. Also removed
>> test 6 since there is not guarantee that we
>>can give a signature which will always fail. Additional changes to
>> GenerateJLIClassesHelper according to review
>>suggestion.
>> - Merge branch 'master' of https://github.com/openjdk/jdk into jdk-8247536
>> - Merge branch 'master' of https://git.openjdk.java.net/jdk into jdk-8247536
>> - ... and 13 more:
>> https://git.openjdk.java.net/jdk/compare/82fe023b...f5584dcf
>
> src/java.base/share/classes/jdk/internal/misc/CDS.java line 144:
>
>> 142: String line = s.trim();
>> 143: if (!line.startsWith("[LF_RESOLVE]") &&
>> !line.startsWith("[SPECIES_RESOLVE]")) {
>> 144: System.out.println("Wrong prefix: " + line);
>
> Should this throw an exception instead?
This part is for check the format only, throw exceptions will lead more objects
generated which should not be archived
in shared heap. Since this is only called from VM, so decide not to throw
exception here.
> src/java.base/share/classes/jdk/internal/misc/CDS.java line 40:
>
>> 38: * indicator for dumping class list.
>> 39: */
>> 40: static public final boolean isDumpingClassList;
>
> what about making this a private static field and adding a public static
> `isDumpingClassList()` method (which was in
> the previous version).
That will have a name for two properties, if you that is OK, I will use the
previous version.
> src/java.base/share/classes/jdk/internal/misc/CDS.java line 155:
>
>> 153: System.out.println("Incorrecct number of items in
>> the line: " + parts.length);
>> 154: System.out.println("line: " + line);
>> 155: return null;
>
> I think these error cases should throw `IllegalArgumentException` and VM
> decides how to handle the exception.
Same reason as above.
> src/java.base/share/classes/jdk/internal/misc/CDS.java line 140:
>
>> 138: // return null for invalid input
>> 139: private static Stream validateInputLines(String[] lines) {
>> 140: ArrayList list = new ArrayList(lines.length);
>
> Nit: this can use diamond operatior like this: `new
> ArrayList<>(lines.length)`.
Will update.
> src/java.base/share/classes/jdk/internal/misc/CDS.java line 184:
>
>> 182: Objects.requireNonNull(lines);
>> 183: try {
>> 184: Stream lineStream = validateInputLines(lines);
>
> It seems clearer to have `validateInputLines` do validation only and convert
> this line into:
>
> validateInputLines(lines);
> Stream lineStream = Arrays.stream(lines);
Somewhere in the testing framework, the line in ExtraClassList was added '\f'
which needs trim, I did not dig into deep
the root cause so here just return a new List with ending white spaces trimmed
instead. I will file a bug for it and
when it fixed, we can do this way.
> src/java.base/share/classes/jdk/internal/misc/CDS.java line
Re: RFR: 8247536: Support for pre-generated java.lang.invoke classes in CDS static archive [v12]
On Tue, 6 Oct 2020 20:46:17 GMT, Yumin Qi wrote:
>> This patch is reorganized after 8252725, which is separated from this patch
>> to refactor jlink glugin code. The previous
>> webrev with hg can be found at:
>> http://cr.openjdk.java.net/~minqi/2020/8247536/webrev-05. With 8252725
>> integrated, the
>> regeneration of holder classes is simply to call the new added
>> GenerateJLIClassesHelper.cdsGenerateHolderClasses
>> function. Tests: tier1-4
>
> Yumin Qi has updated the pull request with a new target base due to a merge
> or a rebase. The pull request now contains
> 23 commits:
> - Added new separate function to CDS for logging species and modified the
> existing function to log lambda form invokers.
>Changed isDumpLoadedClassList to a reasonable name isDumpingClassList as
> read only in CDS.
> - Merge branch 'master' of https://github.com/openjdk/jdk into jdk-8247536
> - Removed unused imports.
> - Fixed comments with correct class and method name in CDS, removed unused
> variables after last change.
> - Moved and renamed cdsGenerateHolderClasses from GenerateJLIClassesHelp to
> CDS as generateLambdaFormHolderClasses. Added
>input verification function in CDS before class generation. Added more
> test scenarios. Removed trailing unused ending
>words for output of lambda form trace line in case of DumpLoadedClassList.
> - Move the check work to java, restore code in VM. Modified test code
> according to the changes. The invoke name
>verififcation is not implemented since not all the holder class are
> processed, not all the functions of processed
>holder classes are added. For holder class with DirectMethodHandle in its
> name, only the name in the
>DMH_METHOD_TYPE_MAP keyset is added, ithe line with other names just gets
> skipped silently. This makes the verification
>on invoke names difficul, a name not in the keyset should not fail the
> test. Also add a boolean to
>cdsGenerateHolderClasses to indicate call path.
> - Remove trailing word of line which is not used in holder class
> regeneration. There is a trailing LF (Line Feed) so trim
>white spaces from both front and end of the line or it will fail method
> type validation.
> - In case of exception happens during reloading class, CHECK will return
> without free the allocated buffer for class
>bytes so moved the buffer allocation and freeing to caller. Also removed
> test 6 since there is not guarantee that we
>can give a signature which will always fail. Additional changes to
> GenerateJLIClassesHelper according to review
>suggestion.
> - Merge branch 'master' of https://github.com/openjdk/jdk into jdk-8247536
> - Merge branch 'master' of https://git.openjdk.java.net/jdk into jdk-8247536
> - ... and 13 more:
> https://git.openjdk.java.net/jdk/compare/82fe023b...f5584dcf
src/java.base/share/classes/jdk/internal/misc/CDS.java line 40:
> 38: * indicator for dumping class list.
> 39: */
> 40: static public final boolean isDumpingClassList;
what about making this a private static field and adding a public static
`isDumpingClassList()` method (which was in
the previous version).
src/java.base/share/classes/jdk/internal/misc/CDS.java line 144:
> 142: String line = s.trim();
> 143: if (!line.startsWith("[LF_RESOLVE]") &&
> !line.startsWith("[SPECIES_RESOLVE]")) {
> 144: System.out.println("Wrong prefix: " + line);
Should this throw an exception instead?
src/java.base/share/classes/jdk/internal/misc/CDS.java line 155:
> 153: System.out.println("Incorrecct number of items in
> the line: " + parts.length);
> 154: System.out.println("line: " + line);
> 155: return null;
I think these error cases should throw `IllegalArgumentException` and VM
decides how to handle the exception.
src/java.base/share/classes/jdk/internal/misc/CDS.java line 140:
> 138: // return null for invalid input
> 139: private static Stream validateInputLines(String[] lines) {
> 140: ArrayList list = new ArrayList(lines.length);
Nit: this can use diamond operatior like this: `new ArrayList<>(lines.length)`.
src/java.base/share/classes/jdk/internal/misc/CDS.java line 184:
> 182: Objects.requireNonNull(lines);
> 183: try {
> 184: Stream lineStream = validateInputLines(lines);
It seems clearer to have `validateInputLines` do validation only and convert
this line into:
validateInputLines(lines);
Stream lineStream = Arrays.stream(lines);
src/java.base/share/classes/jdk/internal/misc/CDS.java line 178:
> 176: /**
> 177: * called from vm to generate MethodHandle holder classes
> 178: * @return @code { Object[] } if holder classes can be generated.
type: `s/@code { Object[]/{@code Object[]}`
src/java.base/share/classes/jdk/internal/misc/CDS.java line 198:
> 196: return retArray;
> 197: }
Re: RFR: 8247536: Support for pre-generated java.lang.invoke classes in CDS static archive [v12]
On Wed, 7 Oct 2020 17:48:41 GMT, Ioi Lam wrote:
>> Yumin Qi has updated the pull request with a new target base due to a merge
>> or a rebase. The pull request now contains
>> 23 commits:
>> - Added new separate function to CDS for logging species and modified the
>> existing function to log lambda form invokers.
>>Changed isDumpLoadedClassList to a reasonable name isDumpingClassList as
>> read only in CDS.
>> - Merge branch 'master' of https://github.com/openjdk/jdk into jdk-8247536
>> - Removed unused imports.
>> - Fixed comments with correct class and method name in CDS, removed unused
>> variables after last change.
>> - Moved and renamed cdsGenerateHolderClasses from GenerateJLIClassesHelp to
>> CDS as generateLambdaFormHolderClasses. Added
>>input verification function in CDS before class generation. Added more
>> test scenarios. Removed trailing unused ending
>>words for output of lambda form trace line in case of DumpLoadedClassList.
>> - Move the check work to java, restore code in VM. Modified test code
>> according to the changes. The invoke name
>>verififcation is not implemented since not all the holder class are
>> processed, not all the functions of processed
>>holder classes are added. For holder class with DirectMethodHandle in its
>> name, only the name in the
>>DMH_METHOD_TYPE_MAP keyset is added, ithe line with other names just gets
>> skipped silently. This makes the verification
>>on invoke names difficul, a name not in the keyset should not fail the
>> test. Also add a boolean to
>>cdsGenerateHolderClasses to indicate call path.
>> - Remove trailing word of line which is not used in holder class
>> regeneration. There is a trailing LF (Line Feed) so trim
>>white spaces from both front and end of the line or it will fail method
>> type validation.
>> - In case of exception happens during reloading class, CHECK will return
>> without free the allocated buffer for class
>>bytes so moved the buffer allocation and freeing to caller. Also removed
>> test 6 since there is not guarantee that we
>>can give a signature which will always fail. Additional changes to
>> GenerateJLIClassesHelper according to review
>>suggestion.
>> - Merge branch 'master' of https://github.com/openjdk/jdk into jdk-8247536
>> - Merge branch 'master' of https://git.openjdk.java.net/jdk into jdk-8247536
>> - ... and 13 more:
>> https://git.openjdk.java.net/jdk/compare/82fe023b...f5584dcf
>
> src/hotspot/share/prims/jvm.cpp line 3872:
>
>> 3870: JVM_ENTRY(jboolean, JVM_IsDumpingClassList(JNIEnv *env))
>> 3871: JVMWrapper("JVM_IsDumpingClassList");
>> 3872: return DumpLoadedClassList != NULL && classlist_file->is_open();
>
> For sanity, it's better to add `classlist_file != NULL`
done
-
PR: https://git.openjdk.java.net/jdk/pull/193
Re: RFR: 8247536: Support for pre-generated java.lang.invoke classes in CDS static archive [v12]
On Tue, 6 Oct 2020 20:46:17 GMT, Yumin Qi wrote:
>> This patch is reorganized after 8252725, which is separated from this patch
>> to refactor jlink glugin code. The previous
>> webrev with hg can be found at:
>> http://cr.openjdk.java.net/~minqi/2020/8247536/webrev-05. With 8252725
>> integrated, the
>> regeneration of holder classes is simply to call the new added
>> GenerateJLIClassesHelper.cdsGenerateHolderClasses
>> function. Tests: tier1-4
>
> Yumin Qi has updated the pull request with a new target base due to a merge
> or a rebase. The pull request now contains
> 23 commits:
> - Added new separate function to CDS for logging species and modified the
> existing function to log lambda form invokers.
>Changed isDumpLoadedClassList to a reasonable name isDumpingClassList as
> read only in CDS.
> - Merge branch 'master' of https://github.com/openjdk/jdk into jdk-8247536
> - Removed unused imports.
> - Fixed comments with correct class and method name in CDS, removed unused
> variables after last change.
> - Moved and renamed cdsGenerateHolderClasses from GenerateJLIClassesHelp to
> CDS as generateLambdaFormHolderClasses. Added
>input verification function in CDS before class generation. Added more
> test scenarios. Removed trailing unused ending
>words for output of lambda form trace line in case of DumpLoadedClassList.
> - Move the check work to java, restore code in VM. Modified test code
> according to the changes. The invoke name
>verififcation is not implemented since not all the holder class are
> processed, not all the functions of processed
>holder classes are added. For holder class with DirectMethodHandle in its
> name, only the name in the
>DMH_METHOD_TYPE_MAP keyset is added, ithe line with other names just gets
> skipped silently. This makes the verification
>on invoke names difficul, a name not in the keyset should not fail the
> test. Also add a boolean to
>cdsGenerateHolderClasses to indicate call path.
> - Remove trailing word of line which is not used in holder class
> regeneration. There is a trailing LF (Line Feed) so trim
>white spaces from both front and end of the line or it will fail method
> type validation.
> - In case of exception happens during reloading class, CHECK will return
> without free the allocated buffer for class
>bytes so moved the buffer allocation and freeing to caller. Also removed
> test 6 since there is not guarantee that we
>can give a signature which will always fail. Additional changes to
> GenerateJLIClassesHelper according to review
>suggestion.
> - Merge branch 'master' of https://github.com/openjdk/jdk into jdk-8247536
> - Merge branch 'master' of https://git.openjdk.java.net/jdk into jdk-8247536
> - ... and 13 more:
> https://git.openjdk.java.net/jdk/compare/82fe023b...f5584dcf
Marked as reviewed by iklam (Reviewer).
src/hotspot/share/classfile/lambdaFormInvokers.cpp line 133:
> 131: log_info(cds)("Class %s not present, skip", name);
> 132: return;
> 133: }
`assert(klass->is_instance_klass(), "Should be");` should be after the NULL
check of `klass`
src/hotspot/share/prims/jvm.cpp line 3872:
> 3870: JVM_ENTRY(jboolean, JVM_IsDumpingClassList(JNIEnv *env))
> 3871: JVMWrapper("JVM_IsDumpingClassList");
> 3872: return DumpLoadedClassList != NULL && classlist_file->is_open();
For sanity, it's better to add `classlist_file != NULL`
-
PR: https://git.openjdk.java.net/jdk/pull/193
Re: RFR: 8247536: Support for pre-generated java.lang.invoke classes in CDS static archive [v12]
> This patch is reorganized after 8252725, which is separated from this patch > to refactor jlink glugin code. The previous > webrev with hg can be found at: > http://cr.openjdk.java.net/~minqi/2020/8247536/webrev-05. With 8252725 > integrated, the > regeneration of holder classes is simply to call the new added > GenerateJLIClassesHelper.cdsGenerateHolderClasses > function. Tests: tier1-4 Yumin Qi has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 23 commits: - Added new separate function to CDS for logging species and modified the existing function to log lambda form invokers. Changed isDumpLoadedClassList to a reasonable name isDumpingClassList as read only in CDS. - Merge branch 'master' of https://github.com/openjdk/jdk into jdk-8247536 - Removed unused imports. - Fixed comments with correct class and method name in CDS, removed unused variables after last change. - Moved and renamed cdsGenerateHolderClasses from GenerateJLIClassesHelp to CDS as generateLambdaFormHolderClasses. Added input verification function in CDS before class generation. Added more test scenarios. Removed trailing unused ending words for output of lambda form trace line in case of DumpLoadedClassList. - Move the check work to java, restore code in VM. Modified test code according to the changes. The invoke name verififcation is not implemented since not all the holder class are processed, not all the functions of processed holder classes are added. For holder class with DirectMethodHandle in its name, only the name in the DMH_METHOD_TYPE_MAP keyset is added, ithe line with other names just gets skipped silently. This makes the verification on invoke names difficul, a name not in the keyset should not fail the test. Also add a boolean to cdsGenerateHolderClasses to indicate call path. - Remove trailing word of line which is not used in holder class regeneration. There is a trailing LF (Line Feed) so trim white spaces from both front and end of the line or it will fail method type validation. - In case of exception happens during reloading class, CHECK will return without free the allocated buffer for class bytes so moved the buffer allocation and freeing to caller. Also removed test 6 since there is not guarantee that we can give a signature which will always fail. Additional changes to GenerateJLIClassesHelper according to review suggestion. - Merge branch 'master' of https://github.com/openjdk/jdk into jdk-8247536 - Merge branch 'master' of https://git.openjdk.java.net/jdk into jdk-8247536 - ... and 13 more: https://git.openjdk.java.net/jdk/compare/82fe023b...f5584dcf - Changes: https://git.openjdk.java.net/jdk/pull/193/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=193=11 Stats: 567 lines in 21 files changed: 545 ins; 14 del; 8 mod Patch: https://git.openjdk.java.net/jdk/pull/193.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/193/head:pull/193 PR: https://git.openjdk.java.net/jdk/pull/193
