On Mon, 14 Apr 2025 15:23:07 GMT, Timofei Pushkin <tpush...@openjdk.org> wrote:
>> test/hotspot/jtreg/runtime/cds/appcds/customLoader/ClassFromClasspath.java
>> line 53:
>>
>>> 51: out.shouldContain("unreg CustomLoadee");
>>> 52: }
>>> 53: }
>>
>> For completeness, I think we should have a more complicated scenario:
>>
>> - load CustomLoadee in both the app loader and a custom loader
>> - load CustomLoadeeChild in the custom loader. Its super class should be the
>> one defined in the custom loader
>>
>> At run time, verify that CustomLoadeeChild is archived and its super class
>> is defined in the custom loader
>
> This case will work, I've added the test, however I found another case which
> won't be handled correctly, neither before nor after this change.
>
> This is the case you've suggested, it works fine both before and after the
> change.
>
> java/lang/Object id: 0
> CustomLoadee3 id: 1
> CustomLoadee3 id: 2 super: 0 source: a.jar
> CustomLoadee3Child id: 3 super: 2 source: a.jar
>
>
> However the below one doesn't work neither before nor after this change. At
> dump time, 2 is loaded instead of 1 as the super of 3 because 2, 3 are loaded
> by the same class loader and 1, 2 have the same name. It is possible to get
> such class list if 2 is loaded by a non-delegating unregistered loader while
> 3 is loaded by a different delegating one.
>
> # Difference with the previous: super of the last class
> java/lang/Object id: 0
> CustomLoadee3 id: 1
> CustomLoadee3 id: 2 super: 0 source: a.jar
> CustomLoadee3Child id: 3 super: 1 source: a.jar
>
>
> And the following case is working without this change but will not work with
> it. It is working now because there will be two different unregistered
> loaders for classes 2 and 3 (because they have the same source), however with
> this change there will be one and it will re-use 2 already loaded by it when
> loading 3.
>
> # Difference with the previous: source of the last class
> java/lang/Object id: 0
> CustomLoadee3 id: 1
> CustomLoadee3 id: 2 super: 0 source: a.jar
> CustomLoadee3Child id: 3 super: 1 source: b.jar
>
>
> Although the last case is a regression, I am not sure how much of a problem
> this is since in general having a registered super with the same name as
> another unregistered class is not supported anyway as shown by the second
> case.
Thanks for doing the investigation. I think we are always going to have some
corner cases that cannot be covered:
- For your second case, we would need a separate ClassLoader per class.
- However, that will result in the IllegalAccessError in the current PR
The problem is we are trying to reconstruct the classes without reconstructing
the ClassLoaders. A real solution would probably require updating the classlist
format.
However, future CDS improvements will be based on the AOT cache introduced in
JEP 483. As of JDK 25, we have changed the AOT cache configuration file from a
text file (classlist) to a binary file (see
https://github.com/openjdk/jdk/pull/23484) . Also, the unregistered classes are
saved from the training run, where we still have ClassLoader information (see
https://github.com/openjdk/jdk/pull/23926). Therefore, I think we should just
leave the old classlist format as is, and accept that certain unregistered
classes cannot be supported. I'll try add some test cases
([JDK-8354557](https://bugs.openjdk.org/browse/JDK-8354557)) for the AOT cache
using the test cases you identified above.
This PR address the IllegalAccessError problem and simplifies the
implementation of classlist handling. However, it causes incompatibility (in
your third case). I think we should change the error checking code in
`InstanceKlass* ClassListParser::load_class_from_source()` to print a warning
message and continue. These are the rare cases that we cannot support. Since
CDS is just a cache mechanism, it should be acceptable that certain classes are
not supported. We can use your second and third test cases to validate the
warning messages.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/24223#discussion_r2043259660
