On Fri, 4 Apr 2025 18:13:24 GMT, Timofei Pushkin <tpush...@openjdk.org> wrote:
>> src/java.base/share/classes/jdk/internal/misc/CDS.java line 446:
>>
>>> 444: protected Class<?> findClass(String name) throws
>>> ClassNotFoundException {
>>> 445: // Unregistered classes should be found in load(...),
>>> registered classes should be
>>> 446: // handeled by parent loaders
>>
>> This does seem like a good simplification, as we no longer need to check the
>> `currentClassName`, `currentSuperClass`, etc.
>>
>> We had a plan for supported multiple classes for the same name, but we never
>> go around doing it (even though the classlist format supports that with the
>> use of IDs).
>>
>> I am not sure if the current tests includes a case where a class name `Foo`
>> exists both in the classpath as well as in a custom loader. If not, I think
>> we should add such a case (to ensure that UnregisteredClassLoader never
>> delegates to the application class loader and finds the wrong `Foo`).
>
> Thank you for the review!
>
>> We had a plan for supported multiple classes for the same name, but we never
>> go around doing it (even though the classlist format supports that with the
>> use of IDs).
>
> With this approach having multiple unregistered classes with the same name is
> not possible (JVM will not allow this for a single class loader) so if you
> are still planning to support this in the near future it would be better to
> go with the approach originally suggested in the issue: continue using
> multiple unregistered class loaders but record class loader ID in class list
> and create only one archiving class loader per recorded ID. But that would be
> a larger change since it requires class list format to be changed.
>
>> I am not sure if the current tests includes a case where a class name Foo
>> exists both in the classpath as well as in a custom loader
>
> Pre-existing `ClassFromSystemModule` test seems to test the boot classpath
> case. I've also added a new test for the app classpath case.
We don't have immediate plans for supporting multiple classes of the same name.
I suspect any future enhancements would be available only through new "AOT"
workflow, where the training information, including the unregistered classes,
would be encoded in the binary AOT configuration file. Therefore, these future
enhancements will not affect this particular case (loading unregistered classes
from the classlist text file).
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/24223#discussion_r2029445262
