On Thu, 13 Feb 2025 03:55:50 GMT, Ashutosh Mehra <asme...@openjdk.org> wrote:
>> Calvin Cheung has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> @iklam and @ashu-mehra comment
>
> 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.
I added some comment:
// Check if the runtime boot classpath has more entries than the one stored
in the archive and if the app classpath
// or the module path requires validation.
if (is_boot_classpath && runtime_css.has_next() &&
(need_to_check_app_classpath() || num_module_paths() > 0)) {
// the check passes if all the extra runtime boot classpath entries are
non-existent
if (check_paths_existence(runtime_css)) {
log_warning(cds)("boot classpath is longer than expected");
return false;
}
}
Also fixed the `check_paths_existence()` method so it only checks the extra
entries.
> 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;
> }
Done.
> 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`.
Fixed.
> 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 :)
I renamed them as follows:
// Common accessors
int boot_cp_start_index() const { return 1; }
int boot_cp_end_index() const { return _boot_classpath_end; }
int app_cp_start_index() const { return boot_cp_end_index(); }
int app_cp_end_index() const { return _app_classpath_end; }
int module_path_start_index() const { return app_cp_end_index(); }
int module_path_end_index() const { return _module_end; }
> 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?
Removed. I also removed the `estimate_size_for_archive_helper()` method.
> 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"
Fixed.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/23476#discussion_r1956614255
PR Review Comment: https://git.openjdk.org/jdk/pull/23476#discussion_r1956614149
PR Review Comment: https://git.openjdk.org/jdk/pull/23476#discussion_r1956614064
PR Review Comment: https://git.openjdk.org/jdk/pull/23476#discussion_r1956613894
PR Review Comment: https://git.openjdk.org/jdk/pull/23476#discussion_r1956613789
PR Review Comment: https://git.openjdk.org/jdk/pull/23476#discussion_r1956614357
