RFR: 8279366: CDS should allow alternative locations for JAR files in classpath [v2]

[Calvin Cheung]

On Mon, 10 Oct 2022 05:23:39 GMT, Ioi Lam <iklam at openjdk.org> wrote:

>> Calvin Cheung has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains four additional commits since the last revision:
>> 
>>  - not storing the common app path string in the archive header
>>  - Merge branch 'master' into 8279366-longest-common-prefix
>>  - remove trailing whitespace
>>  - 8279366: CDS should allow alternative locations for JAR files in classpath
>
> src/hotspot/share/cds/filemap.cpp line 261:
> 
>> 259:   set_base_archive_name_size((unsigned int)base_archive_name_size);
>> 260:   set_common_app_classpath_offset((unsigned int)common_app_classpath_offset);
>> 261:   set_common_app_classpath_size((unsigned int)common_app_classpath_size);
> 
> I think there's no need to record the actual `common_app_classpath` string into the `FileMapHeader`. It should be enough to just store the length of the longest common path prefix. E.g., at runtime, the function `FileMapInfo::check_paths_ignoring_common_path` currently only uses `strlen(dumptime_prefix)`.
> 
> Also, `longest_common_app_classpath` can just return the length instead of the string.

I've made the change. The changeset is smaller now.

> src/hotspot/share/cds/filemap.cpp line 895:
> 
>> 893:   for (int i = 1; i < num_paths; i++) {
>> 894:     chr = strrchr((char*)rp_array->at(i), *os::file_separator());
>> 895:     size_t curr_len = chr != NULL ? chr - rp_array->at(i) + 1 : strlen(rp_array->at(i)) + 1;
> 
> If `chr == NULL`, it means we have a file name only, with no file separator. In this case, there will be no common prefix, so this function can return NULL immediately.

Right. I don't need to use strrchr in my updated change.

> src/hotspot/share/cds/filemap.cpp line 915:
> 
>> 913:     *(lcp + j) = c;
>> 914:   }
>> 915: 
> 
> Need to check for this case as well:
> 
> 
> rp[0] = "/x/a/Foo.jar"
> rp[1] = "/x/aa/Bar.jar"
> 
> 
> I think the current code will incorrectly return "/x/a"

Fixed. I rewrote the function. It now returns the length.
I also added some test cases to test the above and refactored some code in the test cases.

> src/hotspot/share/cds/filemap.cpp line 942:
> 
>> 940:     const char* runtime_path = rp_array->at(i)  + strlen(runtime_prefix);
>> 941:     if (strcmp(dumptime_path, runtime_path) != 0) {
>> 942:       mismatch = true;
> 
> Can this be simplified as `return false` so you don't need to maintain the `mismatch` variable?

Fixed.

> src/hotspot/share/cds/filemap.cpp line 1079:
> 
>> 1077:         mismatch = check_paths_ignoring_common_path(j, shared_app_paths_len, rp_array,
>> 1078:                                                     (const char*)dumptime_prefix, runtime_prefix);
>> 1079:       }
> 
> For completeness, we should allow empty prefixes. For example:
> 
> dump: `-cp foo.jar:x/bar.jar`
> run: `-cp a/foo.jar:a/x/bar.jar`
> 
> or
> 
> dump: `-cp a/foo.jar:a/x/bar.jar`
> run: `-cp foo.jar:x/bar.jar`

My updated change doesn't have the condition check anymore.

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

PR: https://git.openjdk.org/jdk/pull/10615