On Fri, 4 Apr 2025 05:58:47 GMT, Ioi Lam <ik...@openjdk.org> wrote:
>> Timofei Pushkin has updated the pull request incrementally with two
>> additional commits since the last revision:
>>
>> - Make getResource concurrent-friendly
>> - Don't use URLClassLoader
>
> 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.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/24223#discussion_r2029248783
