RFR: 8280682: Refactor AOT code source validation checks

[Ashutosh Mehra]

On Wed, 5 Feb 2025 22:32:58 GMT, Calvin Cheung <ccheung at 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.

src/hotspot/share/cds/aotCodeSource.cpp line 762:

> 760:   }
> 761: 
> 762:   if (is_boot_classpath && runtime_css.has_next() && (need_to_check_app_classpath() || num_module_paths() > 0)) {

I am not sure I get what this block is for. Is it for the case where runtime boot cp has more entries than the dumptime boot cp, and it is checking if the extra entries really exist or they are just empty? If so, then `check_paths_existence` should only be checking the extra entries in the boot cp, not all of them.

Can you please explain this and probably add a comment as well to describe what this block is for.

src/hotspot/share/cds/aotCodeSource.cpp line 894:

> 892: // matched exactly.
> 893: bool AOTCodeSourceConfig::need_lcp_match(AllCodeSourceStreams& all_css) const {
> 894:   if (!need_lcp_match_helper(boot_start(), boot_end(), all_css.boot_cp()) ||

Can we reverse these conditions to make it easier to read?


  if (need_lcp_match_helper(boot_start(), boot_end(), all_css.boot_cp()) &&
      need_lcp_match_helper(app_start(), app_end(), all_css.app_cp())) {
    return true;
  } else {
    return false;
  }

src/hotspot/share/cds/aotCodeSource.cpp line 903:

> 901: 
> 902: bool AOTCodeSourceConfig::need_lcp_match_helper(int start, int end, CodeSourceStream& css) const {
> 903:   if (app_end() == boot_start()) {

I feel this block belongs to the caller `need_lcp_match`.

src/hotspot/share/cds/aotCodeSource.hpp line 213:

> 211: 
> 212:   // Common accessors
> 213:   int boot_start()                       const { return 1; }

Can we rename these methods to something like boot_start() -> boot_cp_start_index().
At the call site it makes it clear it is referring to the bootclasspath index, and not booting something :)

src/hotspot/share/cds/aotCodeSource.hpp line 234:

> 232:   // Functions used only during dumptime
> 233:   static void dumptime_init(TRAPS);
> 234:   static size_t estimate_size_for_archive() {

This method doesn't seem to be in use. Can this be removed?

src/hotspot/share/cds/filemap.cpp line 318:

> 316:   if (header()->has_full_module_graph() && has_extra_module_paths) {
> 317:     CDSConfig::stop_using_optimized_module_handling();
> 318:     log_info(cds)("optimized module handling: disabled because of extra module path(s) are specified");

typo: "disabled because ~of~ extra module path(s) are specified"

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/23476#discussion_r1953766439
PR Review Comment: https://git.openjdk.org/jdk/pull/23476#discussion_r1953732835
PR Review Comment: https://git.openjdk.org/jdk/pull/23476#discussion_r1953722869
PR Review Comment: https://git.openjdk.org/jdk/pull/23476#discussion_r1953489002
PR Review Comment: https://git.openjdk.org/jdk/pull/23476#discussion_r1953383815
PR Review Comment: https://git.openjdk.org/jdk/pull/23476#discussion_r1953768345