RFR: 8263421: Module image file is opened twice during VM startup

[Ioi Lam]

On Tue, 20 Apr 2021 17:44:44 GMT, Harold Seigel <hseigel at openjdk.org> wrote:

> Please review this fix for JDK-8263421.  The fix stores the JImageFile* in a static that gets initialized when lookup_vm_options() opens the module image file.  The static JimageFIle* is then used by setup_bootstrap_search_path_impl() when creating the ClassPathImageEntry for the module image file, avoiding having to re-open the module image file.
> 
> This fix moved the creation of the ClassPathImageEntry for the module image file from create_class_path_entry() to setup_bootstrap_search_path_impl() because create_class_path_entry() may try to do this multiple times, but the ClassPathImageEntry for the module image file is only actually created once.
> 
> The fix also removes some unused code related to module image file processing.
> 
> The changes were tested with Mach5 tiers 1 and 2 on Linux, Mac OS, and Windows, and Mach5 tiers 3-
> 5 on Linux x64.
> 
> Thanks, Harold

Looks good overall, but I think we should add some asserts to ensure safety.

src/hotspot/share/classfile/classLoader.cpp line 393:

> 391: ClassFileStream* ClassPathImageEntry::open_stream_for_loader(Thread* current, const char* name, ClassLoaderData* loader_data) {
> 392:   jlong size;
> 393:   JImageLocationRef location = (*JImageFindResource)(jimage(), "", get_jimage_version_string(), name, &size);

There's a subtle change:

* before: `_jimage` could never be NULL, since it's a private member of this `ClassPathImageEntry`
* after: `jimage()` could theoretically be NULL, if `ClassPathImageEntry::close_jimage()` is called, which happens only when:
  * `~ClassPathImageEntry()` is called. But this will never happen because we never delete `ClassPathImageEntry`;
  * `ClassLoader::close_jrt_image()` is called. This happens only on Linux inside `os::abort()`

When we call `jimage()` and expect it to be non-null, maybe we should use a new function `jimage_non_null()` like this:


JImageFile* ClassPathImageEntry::jimage_non_null() {
  assert(ClassLoader::has_jrt_entry(), "must be");
  assert(jimage() != NULL, "should have been opened by ClassLoader::lookup_vm_options "
                            "and remained throughout normal JVM lifetime");
  return jimage();
}


and we should change `~ClassPathImageEntry()` to assert that it's never called?

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

Changes requested by iklam (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/3591