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

[Calvin Cheung]

On 9/12/16, 3:20 PM, Ioi Lam wrote:
> 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).
It turns out changing only jdk/make/mapfiles/libzip/mapfile-vers is enough.
The other three files already contain ZIP_FreeEntry.

While testing my change, I noticed the ReadMappedEntry in 
ClassLoader::load_zip_library() is NULL:
   ReadMappedEntry = CAST_TO_FN_PTR(ReadMappedEntry_t, 
os::dll_lookup(handle, "ZIP_ReadMappedEntry"));

It will be used in ClassPathZipEntry::open_entry(). I did a search in 
the jdk/src/java.base/share/native/libzip and couldn't find the 
function. This is unrelated to the current change. I can file a separate 
bug if needed.
>
>    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
I've fixed the above.
>
> *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",

I will fix the above and send a new webrev after more testing.

thanks,
Calvin
>
> 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
>