RFR: 8255493: Support for pre-generated java.lang.invoke classes in CDS dynamic archive [v8]

[David Holmes]

On Tue, 27 Apr 2021 21:08:38 GMT, Yumin Qi <minqi at openjdk.org> wrote:

>> 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
>
> Yumin Qi has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Restore filemap.[ch]pp

Hi Yumin,

A few comments below - the exception issue still remains.

I can't really comment on the actual archiving logic here as I'm not familiar enough with the design or code.

Thanks,
David

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

> 372:   JavaThread* THREAD = JavaThread::current();
> 373:   log_info(cds, dynamic)("Regenerate lambdaform holder classes ...");
> 374:   LambdaFormInvokers::regenerate_holder_classes(THREAD);

You still have not addressed the exception problem here. This call can return early with an exception pending due to line 89 in the called method, which uses CHECK. That exception will escape up to the Java code after the VM operation completes.

Note: If `regenerate_holder_classes` is changed to suppress all exceptions then it should not be declared TRAPS and there is no need to pass THREAD (manifest [Java]Thread::current() internally as needed).

src/hotspot/share/cds/lambdaFormInvokers.cpp line 67:

> 65: void LambdaFormInvokers::append_filtered(char* line) {
> 66:   for (int k = 0; k < NUM_FILTER; k++) {
> 67:     if (strstr(line, filter[k]) != nullptr) {

Do you really want to match anywhere in the string? I'm assuming the line should start with the filter?

src/hotspot/share/cds/lambdaFormInvokers.cpp line 185:

> 183:       int len = (int)strlen(str);
> 184:       Array<char>* line = ArchiveBuilder::new_ro_array<char>(len+1);
> 185:       strcpy(line->adr_at(0), str); // copies terminating zero as well

Should use strncpy for safety (code checkers will flag this).

src/hotspot/share/classfile/systemDictionaryShared.cpp line 2277:

> 2275:   void do_value(const RunTimeSharedClassInfo* record) {
> 2276:     ResourceMark rm;
> 2277:     _st->print_cr("%4d: %s %s", ++_index, record->_klass->external_name(),

Not clear why the _index handling is changing.

src/hotspot/share/classfile/systemDictionaryShared.cpp line 2297:

> 2295:                       class_loader_name_for_shared(k));
> 2296:         k = k->next_link();
> 2297:         _index++;

Not clear why the _index handling is changing.

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

Changes requested by dholmes (Reviewer).

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