On Tue, 8 Oct 2024 11:01:45 GMT, Andrew Dinn <ad...@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
