On Tue, 6 Oct 2020 20:46:17 GMT, Yumin Qi <mi...@openjdk.org> 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<String> validateInputLines(String[] lines) {
> 140: ArrayList<String> list = new ArrayList<String>(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<String> lineStream = validateInputLines(lines);
It seems clearer to have `validateInputLines` do validation only and convert
this line into:
validateInputLines(lines);
Stream<String> 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: } catch (Exception e) {
> 198: e.printStackTrace();
Is this a debugging statement? If CDS swallows the exception thrown, I think
VM should emit the warning message and
print the stack trace if appropriate.
-------------
PR: https://git.openjdk.java.net/jdk/pull/193
