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

[Ioi Lam]

On Tue, 8 Oct 2024 11:01:45 GMT, Andrew Dinn <adinn at openjdk.org> wrote:

>> Ioi Lam has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   @adinn comments
>
> src/hotspot/share/cds/archiveBuilder.cpp line 234:
> 
>> 232:       _klasses->append(klass);
>> 233:       if (klass->is_hidden() && klass->is_instance_klass()) {
>> 234:         update_hidden_class_loader_type(InstanceKlass::cast(klass));
> 
> Can you explain why this 'update' is needed. Are the shared classloader type and classpath index not already set? Or do they need adjusting? Perhaps a comment would help.

Thanks for pointing this out. This code was necessary because `ClassLoader::record_result()` was not handling hidden classes. I fixed this by moving the logic from `update_hidden_class_loader_type()` to `ClassLoader::record_hidden_class()`.

> src/hotspot/share/cds/cdsConfig.cpp line 486:
> 
>> 484: bool CDSConfig::allow_only_single_java_thread() {
>> 485:   // See comments in JVM_StartThread()
>> 486:   return is_dumping_static_archive();
> 
> The test in `JVM_StartThread()` calls `CDSConfig::is_dumping_static_archive()`. Should it be updated to call `CDSConfig::allow_only_single_java_thread()`?

I updated `JVM_StartThread()`  to call `CDSConfig::allow_only_single_java_thread()`

> src/hotspot/share/cds/cdsHeapVerifier.cpp line 126:
> 
>> 124: 
>> 125:   // Integer for 0 and 1 are in java/lang/Integer$IntegerCache and are archived
>> 126:   ADD_EXCL("sun/invoke/util/ValueConversions",           "ONE_INT",                // E
> 
> At line 46 the example that explains how the verifier works discusses save and restore of field `Foo#archivedFoo`. I believe that field was supposed to be declared as `static` but it is actually declared as an instance field.

I edited the comment to add `static` to the declaration of `Foo#archivedFoo`.

> src/hotspot/share/cds/cdsHeapVerifier.cpp line 274:
> 
>> 272:     char* class_name = info->_holder->name()->as_C_string();
>> 273:     char* field_name = info->_name->as_C_string();
>> 274:     if (strstr(class_name, "java/lang/invoke/BoundMethodHandle$Species_") == class_name &&
> 
> Can we have a comment to explain this special case?

This check is no longer necessary, the Species_XXX classes are now added to the aot-init list in `AOTClassInitializer::can_archive_initialized_mirror()`. I've removed these two lines.

> src/hotspot/share/cds/classListParser.cpp line 609:
> 
>> 607:   // We store a pre-generated version of the lambda proxy class in the AOT cache,
>> 608:   // which will be loaded via JVM_LookupLambdaProxyClassFromArchive().
>> 609:   // This eliminate dynamic class generation of the proxy class, but we still need to
> 
> "This eliminate" --> "This eliminates"

Fixed

> src/hotspot/share/cds/heapShared.cpp line 893:
> 
>> 891: void KlassSubGraphInfo::check_allowed_klass(InstanceKlass* ik) {
>> 892:   if (CDSConfig::is_dumping_invokedynamic()) {
>> 893:     // We allow LambdaProxy classes in platform and app loaders as well.
> 
> Do we not want an assert here to describe the extra classes we are allowing?
> 
> Also, does the comment at line 1975 now need to be updated -- it refers to this check which has been widened.

I updated `KlassSubGraphInfo::check_allowed_klass()` to do proper checks for lambda proxy classes that are allowed. I also update the comment at at line 1975 to be consistent with `check_allowed_klass()`.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/21143#discussion_r1792659021
PR Review Comment: https://git.openjdk.org/jdk/pull/21143#discussion_r1792659000
PR Review Comment: https://git.openjdk.org/jdk/pull/21143#discussion_r1792658987
PR Review Comment: https://git.openjdk.org/jdk/pull/21143#discussion_r1792658960
PR Review Comment: https://git.openjdk.org/jdk/pull/21143#discussion_r1792658948
PR Review Comment: https://git.openjdk.org/jdk/pull/21143#discussion_r1792658912