On Fri, 14 Feb 2025 19:21:32 GMT, Calvin Cheung <cche...@openjdk.org> wrote:
>> This changset refactors CDS class paths and module paths validation code
>> into a new class `AOTCodeSource` and related class `AOTCodeSourceConfig`.
>> Code has been moved from filemap.[c|h]pp, classLoader.[c|h]pp, and
>> classLoaderExt.[c|h]pp to aotCodeSource.[c|h]pp. CDS dependencies have been
>> removed from `classLoader.cpp`. More refactoring could be done, such as
>> removing `classLoaderExt.cpp`, in a future RFE.
>>
>> Passed tiers 1 - 5 testing.
>
> Calvin Cheung has updated the pull request incrementally with one additional
> commit since the last revision:
>
> @iklam and @ashu-mehra comment
This is a very large change to try and digest. Is it really just a refactor?
I've made a few comments in places where it seems functionality may have been
changed as well.
src/hotspot/share/cds/aotCodeSource.cpp line 2:
> 1: /*
> 2: * Copyright (c) 2025, Oracle and/or its affiliates. All rights reserved.
If the code has been moved from other files, the current opinion/consensus is
that the first copyright year should be the oldest first year from all the
files from which the code was obtained.
src/hotspot/share/cds/aotCodeSource.cpp line 106:
> 104: for (const char* bootcp = Arguments::get_boot_class_path(); *bootcp
> != '\0'; ++bootcp) {
> 105: if (*bootcp == *os::path_separator()) {
> 106: ++ bootcp;
Nit (possibly pre-existing) - no space before/after unary operators
src/hotspot/share/cds/aotCodeSource.hpp line 125:
> 123: // during AOTCache creation are the same as when the AOTCache is used
> during runtime.
> 124: // Non-existent entries are recorded during AOTCache creation. Those
> non-existent entries
> 125: // must not exist during runtime.
Does this mean that if Foo.jar is on the classpath but does not in fact exist,
then we record it was on the classpath and require it to be on the classpath at
runtime, but also to still not exist?
src/hotspot/share/cds/aotCodeSource.hpp line 128:
> 126: //
> 127: // Some details on validation:
> 128: // - the boot classpath could be appended during runtime if there's no
> app classpath and
Suggestion:
// - the boot classpath can be appended to at runtime if there's no app
classpath and no
src/hotspot/share/cds/aotCodeSource.hpp line 130:
> 128: // - the boot classpath could be appended during runtime if there's no
> app classpath and
> 129: // module path specified when an AOTCache is created;
> 130: // - the app classpath could be appended during runtime;
Suggestion:
// - the app classpath can be appended to at runtime;
src/hotspot/share/cds/aotCodeSource.hpp line 131:
> 129: // module path specified when an AOTCache is created;
> 130: // - the app classpath could be appended during runtime;
> 131: // - the module path during runtime could be a superset of the one
> specified during AOTCache creation.
Suggestion:
// - the module path at runtime can be a superset of the one specified during
AOTCache creation.
src/hotspot/share/runtime/threads.cpp line 809:
> 807: vm_exit_during_initialization("ClassLoader::initialize_module_path()
> failed unexpectedly");
> 808: }
> 809: #endif
Not obvious where this functionality is now handled.
test/hotspot/jtreg/runtime/cds/appcds/BootClassPathMismatch.java line 243:
> 241: * No error - bootclasspath can be appended during runtime if no -cp
> is specified.
> 242: */
> 243: public void testBootClassPathAppend() throws Exception {
A refactoring should not be introducing new test cases. Did you refactor and
enhance?
test/hotspot/jtreg/runtime/cds/appcds/NonExistClasspath.java line 70:
> 68: .assertNormalExit();
> 69:
> 70: // Replace nonExistPath with another non-existent file in the CP,
> it should still work
Not at all clearr why these test cases have been removed?
-------------
PR Review: https://git.openjdk.org/jdk/pull/23476#pullrequestreview-2622538668
PR Review Comment: https://git.openjdk.org/jdk/pull/23476#discussion_r1959178211
PR Review Comment: https://git.openjdk.org/jdk/pull/23476#discussion_r1959126336
PR Review Comment: https://git.openjdk.org/jdk/pull/23476#discussion_r1959181514
PR Review Comment: https://git.openjdk.org/jdk/pull/23476#discussion_r1959181842
PR Review Comment: https://git.openjdk.org/jdk/pull/23476#discussion_r1959183865
PR Review Comment: https://git.openjdk.org/jdk/pull/23476#discussion_r1959184959
PR Review Comment: https://git.openjdk.org/jdk/pull/23476#discussion_r1959192451
PR Review Comment: https://git.openjdk.org/jdk/pull/23476#discussion_r1959193331
PR Review Comment: https://git.openjdk.org/jdk/pull/23476#discussion_r1959194900
