RFR: 8266764: [REDO] JDK-8255493 Support for pre-generated java.lang.invoke classes in CDS dynamic archive [v2]

[Ioi Lam]

On Tue, 11 May 2021 03:24:23 GMT, Yumin Qi <minqi at openjdk.org> wrote:

>> Hi, please review
>>   This is REDO for jdk-8255493, the integration of the fix caused ParallelLambdaLoadTest.java failed. 
>>   The old PR description:
>> ------------- old ------------
>> 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.
>> ------------ new -------------
>> The new fix  (incremental)
>> 1)  Added a lock to protect LambdaFormInvokers::_lambdaform_lines, at dynamic dump case, there are potentially concurrent issue the list is added from multiple threads at same time  regeneration of holder class is reading from the list.
>> 2) Added a new function to MetaspaceShared, MetaspaceShared::regenerate_lambdaforminvokers_holders which calls into LambdaFormInvokers::LambdaFormInvokers::regenerate_holder_classes, and handle exceptions, this way even the regeneration fails, no affect to normal dump process.
>> 3) Move the call to regenerate holder classes to MetaspaceShared::link_and_cleanup_shared_classes, this is before java shutdown hook execution, since after that, it is not safe to call into java. 
>> 4) Since change in 3), jcmd to dynamic dump, we need call MetaspaceShared::regenerate_lambdaforminvokers_holders before do dumping.
>> 
>> Tests: tier1,tier2,tier3,tier4
>>            local test on runtime/cds
>>            local test ParallelLambdaLoadTest.java in loops without crash.
>> 
>> Thanks
>> Yumin
>
> Yumin Qi has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Remove unused variables

Hi Yumin,

I applied your patch and ran ParallelLambdaLoadTest.java for about 400 iterations and I could no longer reproduce the crash in [JDK-8266765](https://bugs.openjdk.java.net/browse/JDK-8266765).

As I mentioned in some of the individual comments, we need to properly handle the exceptions that can happen inside `MetaspaceShared::link_and_cleanup_shared_classes`. I think we should change these places:


JVM_ENTRY_NO_ENV(void, JVM_BeforeHalt())
  // Link all classes for dynamic CDS dumping before vm exit.
  if (DynamicDumpSharedSpaces) {
-  MetaspaceShared::link_and_cleanup_shared_classes(THREAD);
+  DynamicArchive::prepare_for_dynamic_dumping_at_exit();
  }
...

void JavaThread::invoke_shutdown_hooks() {
...
  if (DynamicDumpSharedSpaces) {
-  MetaspaceShared::link_and_cleanup_shared_classes(this);
+  DynamicArchive::prepare_for_dynamic_dumping_at_exit();
  }


And add a new function like this to catch the exception. If `link_and_cleanup_shared_classes` has thrown an exception, we should not dump the dynamic archive anymore. (We are already do that with the static archive).


void DynamicArchive::prepare_for_dynamic_dumping_at_exit() {
  EXCEPTION_MARK;
  ResourceMark rm(THREAD);
  MetaspaceShared::link_and_cleanup_shared_classes(THREAD);
  if (HAS_PENDING_EXCEPTION) {
    log_error(cds)("ArchiveClassesAtExit has failed");
    log_error(cds)("%s: %s", PENDING_EXCEPTION->klass()->external_name(),
                   java_lang_String::as_utf8_string(java_lang_Throwable::message(PENDING_EXCEPTION)));
    // We cannot continue to dump the archive anymore.
    DynamicDumpSharedSpaces = false;
    CLEAR_PENDING_EXCEPTION;
  }
}


I would also suggest writing a new test case for the exception handling: run a program with something like `-Xmx64m -XX:ArchiveClassesAtExit`. The program should completely fill up the Java heap and exit. The dynamic archive should fail to dump due to OOM.

src/hotspot/share/cds/dynamicArchive.cpp line 363:

> 361: }
> 362: 
> 363: void DynamicArchive::dump(TRAPS) {

TRAPS is no longer needed since this function doesn't throw exceptions anymore.

src/hotspot/share/cds/metaspaceShared.cpp line 629:

> 627:                  java_lang_String::as_utf8_string(java_lang_Throwable::message(PENDING_EXCEPTION)));
> 628:     log_info(cds)("Regenerate lambdaform holder classes ...failed");
> 629:     CLEAR_PENDING_EXCEPTION; // Exceptions are ignored.

As we discussed in the [previous PR](https://github.com/openjdk/jdk/pull/3611#discussion_r623578292), we will get an exception here if we hit an OOM, or we have generated a bad classfile. In either case, the exception cannot be just ignored. It should be reported to the user and the dynamic dump should fail.

I think we don't need this function anymore. Instead, the logging like `log_info(cds)("Regenerate lambdaform holder classes ...");` can be moved into `regenerate_holder_classes()`. The  `HAS_PENDING_EXCEPTION` check should be removed.

src/hotspot/share/cds/metaspaceShared.cpp line 638:

> 636:   // Collect all loaded ClassLoaderData.
> 637:   ResourceMark rm;
> 638:   regenerate_lambdaforminvokers_holders(THREAD);

Should be `CHECK`.

src/hotspot/share/prims/jvm.cpp line 3689:

> 3687:   Handle file_handle(THREAD, JNIHandles::resolve_non_null(archiveName));
> 3688:   char* archive_name  = java_lang_String::as_utf8_string(file_handle());
> 3689:   MetaspaceShared::regenerate_lambdaforminvokers_holders(THREAD);

The `THREAD` should be `CHECK`.

-------------

Changes requested by iklam (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/3953