RFR: 8293336: AOT-linking of invokedynamic for lambda expression and string concat [v7]

[Ioi Lam]

On Wed, 16 Oct 2024 22:56:06 GMT, Ashutosh Mehra <asmehra at openjdk.org> wrote:

>> Ioi Lam has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Fixed typo in last commit; fixed header inclusion order
>
> src/hotspot/share/cds/heapShared.cpp line 675:
> 
>> 673:     const char* klass_name = info->klass_name;
>> 674:     for (; fields[i].valid(); i++) {
>> 675:       ArchivableStaticFieldInfo* f = &fields[i];
> 
> It looks like the pattern of nested for-loops is copied from HeapShared::archive_object_subgraphs()
> but this pattern doesn't seem to give any advantage here. If so can we replace it with just single for loop to make it easier to understand?

I changed it to a single loop as suggested.

> src/hotspot/share/classfile/systemDictionaryShared.cpp line 775:
> 
>> 773:   // Now, all hidden classes that have not yet been scanned must be marked as excluded
>> 774:   auto exclude_remaining_hidden = [&] (InstanceKlass* k, DumpTimeClassInfo& info) {
>> 775:     if (k->is_hidden() && !info.has_checked_exclusion()) {
> 
> Suggestion: Check for `has_checked_exclusion()` is not really required as the call to `SDS::check_for_exclusion()` does that. And the condition in `guarantee()` can be updated as `info.is_excluded() || info.is_required()`.

I changed the code to the following to make sure that `should_hidden_class_be_archived(k)` is mutually exclusive with `info.is_excluded()`.


  auto exclude_remaining_hidden = [&] (InstanceKlass* k, DumpTimeClassInfo& info) {
    if (k->is_hidden()) {
      SystemDictionaryShared::check_for_exclusion(k, &info);
      if (CDSConfig::is_dumping_invokedynamic()) {
        if (should_hidden_class_be_archived(k)) {
          guarantee(!info.is_excluded(), "Must be");
        } else {
          guarantee(info.is_excluded(), "Must be");
        }
      }
    }
  };


Unfortunately I can do this only for `CDSConfig::is_dumping_invokedynamic()`. In the "legacy CDS" mode of lambda support, we actually would return true from `should_hidden_class_be_archived(k)` and then later decide to exclude `k`.

> src/hotspot/share/oops/constantPool.cpp line 340:
> 
>> 338:     src_cp->iterate_archivable_resolved_references([&](int rr_index) {
>> 339:       keep_resolved_refs.at_put(rr_index, true);
>> 340:       });
> 
> Indentation seems off here

Fixed.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/21143#discussion_r1810025512
PR Review Comment: https://git.openjdk.org/jdk/pull/21143#discussion_r1810025031
PR Review Comment: https://git.openjdk.org/jdk/pull/21143#discussion_r1810025626