RFR(M): 8164011: --patch-module support for CDS

[Ioi Lam]

Hi Calvin,
*
**src/share/vm/classfile/classLoader.cpp*

    There's a memory leak here - the entry is not freed:

      322 bool ClassPathZipEntry::stream_exists(const char* name) {
      323   // enable call to C land
      324   JavaThread* thread = JavaThread::current();
      325   ThreadToNativeFromVM ttn(thread);
      326   // check whether zip archive contains name
      327   jint name_len, filesize;
      328   jzentry* entry = (*FindEntry)(_zip, name, &filesize, &name_len);
      329   return (entry != NULL) ? true : false;
      330 }

    I think you need to add a new function pointer like this, and call
    it if <entry> is not NULL.
       FreeEntry    = CAST_TO_FN_PTR(FreeEntry_t, os::dll_lookup(handle,
    "ZIP_FreeEntry"));

    But, ZIP_FreeEntry (in
    jdk/src/java.base/share/native/libzip/zip_util.c) is not exported.
    To export it, you need to edit all the files under
    jdk/make/mapfiles/libzip (grep for ZIP_FindEntry as an example).

    This line can be removed
    1416   bool found;

    and these lines:
    1444           found = e->stream_exists(file_name);
    1445           if (found) {
    can be changed to "if (e->stream_exists(file_name) {"

    Typo on line 1489: exploeded -> exploded

    This variable seems to be redundant:
    1504   bool found_patch_mod_entry = false;

    I think it's better to move 1523~1525 into here:

    1517 #if INCLUDE_CDS
    1518       if (is_in_patch_module(file_name)) {
                       tty->print_cr("Preload Warning: Skip archiving
    class %s found in --patch-module entry", class_name);
                       return NULL;
                    }
    1519 #endif

*test/runtime/modules/PatchModule/PatchModuleCDS.java**
*

    I think this line should be removed. High level description of the
    test should be included as @summary in the header.
    40         System.out.println("Test that --patch-module and
    -Xshare:dump are compatibable");

    Is mods/java.naming supposed to be a directory that doesn't exist? I
    don't see any code in the test that creates this. If so, it's better
    to use something like "java.naming=no/such/directory"
    46             "--patch-module=java.naming=mods/java.naming",

Thanks
- Ioi

On 9/12/16 11:01 AM, Calvin Cheung wrote:
> Hi,
>
> Please review the following change to allow the use of the 
> --patch-module option with CDS.
>
> bug: https://bugs.openjdk.java.net/browse/JDK-8164011
> webrev: http://cr.openjdk.java.net/~ccheung/8164011/webrev.00/
>
> During dump time, this change checks if a class can be found in the 
> jar entries of the _patch_mod_entries. If so, the class won't be 
> included in the shared archive. It currently does not support non-jar 
> entries in the _patch_mod_entries.
>
> Tested with JPRT and hotspot/runtime tests.
>
> thanks,
> Calvin