-
Notifications
You must be signed in to change notification settings - Fork 6k
8255493: Support for pre-generated java.lang.invoke classes in CDS dynamic archive #3611
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
👋 Welcome back minqi! A progress list of the required criteria for merging this PR into |
Webrevs
|
Mailing list message from David Holmes on hotspot-dev: Hi Yumin, On 22/04/2021 8:42 am, Yumin Qi wrote:
Meta questions: Thanks, |
1 similar comment
Mailing list message from David Holmes on hotspot-dev: Hi Yumin, On 22/04/2021 8:42 am, Yumin Qi wrote:
Meta questions: Thanks, |
Thanks |
Mailing list message from David Holmes on hotspot-dev: On 22/04/2021 1:51 pm, Yumin Qi wrote:
Okay I think I get how these classes are generated and then stored in
Not following this part. How can you regenerate an existing class? I'm
Probably not but that is the problem I'm flagging. Is this list of Thanks, |
1 similar comment
Mailing list message from David Holmes on hotspot-dev: On 22/04/2021 1:51 pm, Yumin Qi wrote:
Okay I think I get how these classes are generated and then stored in
Not following this part. How can you regenerate an existing class? I'm
Probably not but that is the problem I'm flagging. Is this list of Thanks, |
Mailing list message from David Holmes on hotspot-dev: On 23/04/2021 7:18 am, Yumin Qi wrote:
There are two allocation sites in regenerate_holder_classes that use Cheers, |
1 similar comment
Mailing list message from David Holmes on hotspot-dev: On 23/04/2021 7:18 am, Yumin Qi wrote:
There are two allocation sites in regenerate_holder_classes that use Cheers, |
int len = _lambdaform_lines->length(); | ||
objArrayHandle list_lines = oopFactory::new_objArray_handle(vmClasses::String_klass(), len, CHECK); | ||
if (list_lines == nullptr) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's no need to check for null. If allocation fails, the CHECK in the previous line will force this function to return.
(same for the check on line 96).
…check for return NULL
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just had a couple of comments, otherwise looks ok to me but I have to confess that I don't know this code that well.
test/hotspot/jtreg/runtime/cds/appcds/dynamicArchive/TestDynamicRegenerateHolderClasses.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Just a few minor comments.
…lly deleted empty line
Please hold, there is more clean up need to do: including file headers, empty lines etc. |
…aDictionaryPrinter index
…dump) lines in static archive
_lambdaform_lines->append(line); | ||
} | ||
|
||
void LambdaFormInvokers::regenerate_holder_classes() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is no longer a TRAPS function then it should not be using CHECK macro:
objArrayHandle list_lines = oopFactory::new_objArray_handle(vmClasses::String_klass(), len, CHECK);
for (int i = 0; i < len; i++) {
Handle h_line = java_lang_String::create_from_str(_lambdaform_lines->at(i), CHECK);
If exceptions are not possible just use THREAD and comment why they are not possible. If they are possible then you need to use THREAD and handle them appropriately.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will rework the dynamic dump so it will be like static dump --- handle exceptions after returned. So will remove the all the checks in this file. Thanks.
…ions in LambdaFormInvokers::regenerate_holder_classes except for oom
The recent version change made DynamicArchive::dump take TRAPS, same as static dump. The design of LambdaFormInvokers::regenerate_holder_classes is that if the regeneration failed, it won't affect normal classes archived. So we have tests for wrong tag or line format in classlist to make sure dump archive. I changed to handle all exceptions except for OOM (which will be processed by caller if possible for the extreme case) in the function. |
CLEAR_PENDING_EXCEPTION; | ||
return; | ||
} | ||
HANDLE_IF_HAS_EXCEPTION; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of using a macro, I think the code can be more readable like this:
if (check_exception(THREAD)) {
return;
}
and check_exception
can contain the logic of HANDLE_IF_HAS_EXCEPTION
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I was think of that, if a function or macro and finally took macro.
void LambdaFormInvokers::regenerate_holder_classes() { | ||
#define HANDLE_IF_HAS_EXCEPTION \ | ||
if (HAS_PENDING_EXCEPTION) { \ | ||
if (!PENDING_EXCEPTION->is_a(vmClasses::OutOfMemoryError_klass())) { \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest adding a comment and error message to explain why we ignore the error:
// We may get an exception if the classlist contains an error (or an outdated entry generated by
// an older JDK).
if (DumpSharedArchive) {
log_error(cds)("Failed to generate LambdaForm holder classes. Is your classlist out of date?");
} else {
log_error(cds)("Failed to generate LambdaForm holder classes. Was the base archive generated with an outdated classlist?");
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean DumpSharedSpaces? So the message will be different between static (DumpSharedSpaces) and dynamic, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, because the errors can only be caused by bad input from the classlist, which is only used during static archive dump. The information collected during dynamic dump are correct, and cannot cause CDS.generateLambdaFormHolderClasses()
to fail.
CLEAR_PENDING_EXCEPTION; | ||
return; | ||
} | ||
HANDLE_IF_HAS_EXCEPTION; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this necessary? I think bad classlists will cause errors in the JavaCalls::call_static call above, but not here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is the original code, and it is necessary, KlassFactory::create_from_stream could throw exception.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What kid of exceptions will be thrown (other than OOM?) Do you mean CDS.generateLambdaFormHolderClasses()
can generate bad class files?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is after CDS.generateLambdaFormHolderClasses, we get the byte stream of a class --- we need create instanceklass of the class. The exception is from ClassParser ---- whatever the exception during the process.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ClassParser will throw an error only if OOM, or if the class is invalid. However, if the class is invalid, it means there's a bug inside generateLambdaFormHolderClasses() -- it's not supposed to generate an invalid class.
So we cannot just print a warning and ignore the error. The user may not notice the error message.
The archive dumping should fail so the user knows what has happened, and will hopefully report the error to us.
@@ -138,7 +140,6 @@ void LambdaFormInvokers::regenerate_holder_classes(TRAPS) { | |||
memcpy(buf, (char*)h_bytes->byte_at_addr(0), len); | |||
ClassFileStream st((u1*)buf, len, NULL, ClassFileStream::verify); | |||
reload_class(class_name, st, THREAD); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reload_class(class_name, st, THREAD);
should be reload_class(class_name, st, CHECK);
. If an error happens inside there, it means CDS.generateLambdaFormHolderClasses()
has generated a bad classfile, or we have hit an OOM. In either case, we should report the error back to the caller.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, it should be a check here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Just a small nit about comments.
@yminqi This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be:
You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 44 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. ➡️ To integrate this PR with the above commit message to the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The TRAPS/CHECK usages seem okay now.
Thanks,
David
@dholmes-ora Thanks for comment again. @calvinccheung @coleenp I need one more OK to push since this is not a trivial fix. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Latest version looks good. Just one nit.
Thanks,
Calvin
/integrate |
@yminqi Since your change was applied there have been 44 commits pushed to the
Your commit was automatically rebased without conflicts. Pushed as commit 8b37d48. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
Hi, Please review
When do dynamic dump, the pre-generated lambda form classes from java.lang.invoke are not stored in dynamic archive. The patch will regenerate the four holder classes,
"java.lang.invoke.Invokers$Holder",
"java.lang.invoke.DirectMethodHandle$Holder",
"java.lang.invoke.DelegatingMethodHandle$Holder",
"java.lang.invoke.LambdaForm$Holder"
which will include the versions in static archive and new loaded functions all together and stored in dynamic archive. New test case added.
(Minor change to PrintSharedArchiveAtExit, which the index is not consecutive)
Tests: tier1,tier2,tier3,tier4
Thanks
Yumin
Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/3611/head:pull/3611
$ git checkout pull/3611
Update a local copy of the PR:
$ git checkout pull/3611
$ git pull https://git.openjdk.java.net/jdk pull/3611/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 3611
View PR using the GUI difftool:
$ git pr show -t 3611
Using diff file
Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/3611.diff